Add placeholder blinks setting

[SymboLinker]

In issue #175 a feature request was done for stopping the blinking of the placeholder (but still have a blinking carret if it moves to a spot without a placeholder).

The proposed pull request #176 just removes the blinking of the placeholder and does not leave it as an option.

This pull request adds a setting LaTeXSetting.PlaceholderBlinks that keeps the default value true and does not break any existing unit tests.

Tests are added

• to make sure that the setting for a non-blinking placeholder works
• to document how it should work
• to make sure that it will not be removed in a whim

@charlesroddie Please verify that the effect of changing the "PlaceholderBlinks"-setting results in the behaviour you like.

[codecov-io]

Codecov Report

Merging #177 into master will increase coverage by 0.01%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
+ Coverage   87.32%   87.34%   +0.01%     
==========================================
  Files         156      156              
  Lines       11314    11329      +15     
==========================================
+ Hits         9880     9895      +15     
  Misses       1434     1434              

Impacted Files	Coverage Δ
CSharpMath.Forms/MathKeyboardExtensions.cs	0.00% <0.00%> (ø)
CSharpMath.Rendering/FrontEnd/MathKeyboard.cs	10.34% <0.00%> (ø)
CSharpMath.Editor.Tests/CaretTests.cs	100.00% <100.00%> (ø)
CSharpMath.Editor/MathKeyboard.cs	89.02% <100.00%> (-0.12%)	⬇️
CSharpMath/Atom/LaTeXSettings.cs	98.75% <100.00%> (+<0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 476beb0...944e307. Read the comment docs.

[SymboLinker]

O, I made a stupid mistake. The caret does not blink anymore if the setting is used. I will copy what charlesroddie proposed.

[charlesroddie]

I don't understand the enum

  public enum MathKeyboardCaretState : byte {
    Hidden,
    TemporarilyHidden,
    ShownThroughPlaceholder,
    Shown
  }

Is anyone able to document this enum assuming that it makes sense @Happypig375 @SymboLinker @FoggyFinder ?

It seems a little strange to me that, if PlaceholderBlinks, then the path is ShownThroughPlaceholder -> TemporarilyHidden -> Shown -> Hidden -> Shown -> Hidden ... . This suggests to me that ShownThroughPlaceholder is not a valid case. But it's only a guess as I don't understand the meanings.

[Happypig375]

TemporarilyHidden -> Shown
should be
TemporarilyHidden -> ShownThroughPlaceholder

So this may be a bug.

[Happypig375]

Shown: Caret is drawn. Invalid for placeholders. Will disappear when blink timer fires.
ShownThroughPlaceholder: Caret is not drawn. Placeholder is activated. Will disappear when blink timer fires.
TemporarilyHidden: Caret is not drawn. Placeholder is deactivated. Will reappear when blink timer fires.
Hidden: Caret is not drawn. Placeholder is deactivated. Will not reappear when blink timer fires.

[Happypig375]

Trying to set to Shown when caret is a placeholder will be automatically set to ShownThroughPlaceholder instead.

[charlesroddie]

Thanks. Is this better than having only the cases Hidden/TemporarilyHidden/Shown and having Shown display as a cursor or active placeholder depending on the position?

Wouldn't removing the ShownThroughPlaceholder case remove two possibilities for invalid data combinations (Shown and in a placeholder, ShownThroughPlaceholder and not in a placeholder)?

[SymboLinker]

Maybe this would be more clear:

  public enum MathKeyboardCaretState : byte {
    Hidden,
    TemporarilyHidden,
    TemporarilyShown,
  }

and then as charlesroddie suggested

display as a cursor or active placeholder depending on the position?

We could also re-add a Shown enum value that means "don't ever disappear, not even if the timer elapses".

[charlesroddie]

I've pushed a change to this branch c89ac9e which:

• Removes ShownThroughPlaceholder as per Add placeholder blinks setting #177 (comment)
• Adjusts the CaretState setter to actually set it to the requested value!
• Avoids triggering RedrawRequested when the CaretState is set to its current value [Edit: this was a mistake, now reverted]

[charlesroddie]

@SymboLinker your first idea wouldn't be more clear because placeholders won't be TemporarilyShown (except in the case where you had set PlaceholderBlinks to true) and the second idea requires an extra case and enables options (non-blinking cursor) which would be better expressed as a setting.

[charlesroddie]

Can't merge as there is a problem with the cursor showing:

Investigating

[charlesroddie]

Fixed

[charlesroddie]

Thank you for adjusting the tests.

@Happypig375 Looks like @SymboLinker 's VS is also adding a nuget.config file not just mine. I guess it doesn't matter a lot whether we keep it or gitignore it or find out why it's happening? Anyone with an opinion?

[SymboLinker]

Good defaults are extremely important (...) .

Thinking about defaults with such avidity has given me a thought that I will keep in mind for some time. Thanks for this possibly useful insight. This led me to read the following again:

because a large block blinks which is already easy to identify, unlike the cursor where a small line blinks which is harder to identify

Yes, nice. Those subtle things are what LaTeX is about.

Because you mentioned "what others do" first, I concluded too quickly that the argumention for a request for changing the default was weak. I should not have overlooked the second part.

And here I am, ditching the "I decided not to work on...".

[SymboLinker]

(...) also adding a nuget.config file not just mine. I guess it doesn't matter a lot whether we keep it or gitignore it or find out why it's happening? Anyone with an opinion?

Sometimes a NuGet.Config should be included, for example if CSharpMath would go use NightlyPackages as a dependency. That is not the case (yet), so that seems no reason to be against ignoring it now.
However, in my case today was the first time that a NuGet.Config was generated. That may solve the question how it was generated so that that can be stopped. Could ed72190 have done a change that causes it?

[Happypig375]

@charlesroddie If you don't add unit tests testing your desired behaviour, then there is no guarantee that it will continue to function correctly aside from manual testing. And why test it manually instead of automatically?

[SymboLinker]

I suspect this can be tested by calling the Keyboard's DrawCaret method with a Mock<ICanvas> and verify that mock.StartNewPath has not been called. I will look into that later today.

[SymboLinker]

The merge conflict cannot be solved by me. If the changes of cfeb157 are OK, then the MathKeyboardCaretState enum should be deleted in the merge.

[charlesroddie]

If the changes of cfeb157 are OK

They make sense to me

[charlesroddie]

Thanks @SymboLinker !