Make blinking Placeholder's Nucleus and ForeColor customizable in both CaretStates #167
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
==========================================
+ Coverage 86.71% 86.82% +0.11%
==========================================
Files 157 157
Lines 11200 11275 +75
==========================================
+ Hits 9712 9790 +78
+ Misses 1488 1485 -3
Continue to review full report at Codecov.
|
Happypig375
reviewed
Sep 28, 2020
Happypig375
reviewed
Sep 28, 2020
| @@ -7,9 +8,12 @@ namespace CSharpMath.Display { | |||
| /// over the whole string. We use KernedGlyph objects instead of Glyphs to | |||
| /// allow us to set kern on a per-glyph basis.</summary> | |||
| public class AttributedGlyphRun<TFont, TGlyph> where TFont : FrontEnd.IFont<TGlyph> { | |||
| public AttributedGlyphRun(string text, IEnumerable<TGlyph> glyphs, TFont font, bool isPlaceHolder = false) { | |||
| public AttributedGlyphRun(string text, IEnumerable<TGlyph> glyphs, TFont font, bool isPlaceHolder = false, Color? foreColor = null) { | |||
There was a problem hiding this comment.
isPlaceHolder and foreColor can probably be replaced with one placeholderColor
There was a problem hiding this comment.
The placeholder has a default Color null. If the argument placeholderColor is null, there is no way to distinguish between "placeholder with default color" vs "is no placeholder'.
Happypig375
requested review from
Happypig375,
FoggyFinder and
charlesroddie
Happypig375
reviewed
Sep 28, 2020
Happypig375
reviewed
Sep 28, 2020
Seems okay to me. @FoggyFinder @charlesroddie? |
Looks fine |
Colour OK. Is the blinking option needed? The original rationale (#164 (comment)) doesn't include blinking. |
- Rename Placholder's "ForeColor" to "Color". - Use property initializer GlyphInfo.Foreground. - Restore "readonly field" instead of property getter for LaTeXSettings.Dummy. - Use the name parts "Resting" and "Active" in the placeholder setting names (instead of "Hiding" and "FullShow" which are related to the caret but do not fit a blinking placeholder). - Use variable name "placeholder" instead of "ph".
Also: verify more in LaTeXSettings_Placeholder_IsNewInstance.
…ble (#167) Also: in the MockTests class, verify that AttributedGlyphRun sets the GlyphInfo.Foreground to null (default color).
|
|
Use Assert.Equal inside Assert.All's action parameter |
|
About 9925952: I may have misinterpreted the test. |
FoggyFinder
reviewed
Sep 30, 2020
|
The commit datetimes of my latest 2 commits are not correct. I committed after FoggyFinder's review. The reason for incorrect commit datetimes is probably: I have my personal development environment inside a virtual machine and I suspect that when the host computer goes to sleep then the time of the virtual machine does not continue... A bit weird to see a commit that processes feedback earlier in the timeline than the feedback... |
Happypig375
reviewed
Sep 30, 2020
Happypig375
reviewed
Sep 30, 2020
Happypig375
reviewed
Sep 30, 2020
|
@Happypig375 Updating the time of my VM is very easy, so I will do that from now on. I'm sorry for the confusing commit times - c2a95a2 has not yet been reviewed. |
Happypig375
reviewed
Oct 3, 2020
Happypig375
approved these changes
Oct 4, 2020
|
Thank you for your contribution! |
Thank you for your guidance :) |
Happypig375
added
Resolution/Implemented
|
Edit: looking at the code in this PR, it wasn't responsible for blinking placeholders. |
FoggyFinder
added a commit
that referenced
this pull request
Jun 21, 2021
… customizable (#164) * Add TextColor property to BaseButton and make MathInputButton's color customizable The main goal of this commit is to make keyboard colors customizable (per keyboard key) to fit an app's color theme(s). For example, a dark keyboard background with light button texts. CSharpMath.Forms: - Refactor BaseButton's constructor. - Add TextColor property to BaseButton and a bindable TextColorProperty. - Remove the predefined Red color from MathInputButton.InputToLaTeX(input) for "Backspace" and "Clear". CSharpMath.Forms.Example (MathKeyboard.xaml): - Use the TextColorProperty in the Style of "Backspace" and "Clear", setting it to Red. * Add a few ButtonTests and refactor out use-once functions from BaseButton Notes: - I guessed that CSharpMath.Forms needs a Test project of its own. - It seems to deviate from the current coding style to have such a class, but I needed to make sure that the unit test MathInputButtonsHaveBackTextColorByDefault could detect the "phantom" and thus I added a static class LatexHelper that contains the temporary implementation of a "fake" vphantom. I also added the method SetColor(latex,Xamarin.Forms.Color). * Fix typo in unit test name MathInputButtonsHaveBlackTextColorByDefault * ButtonTests: use [ClassData] attribute, use NotNull<T>() extension, include in slnf - Instead of IEnumerable<MathInputButton> TheMathInputButtons, use the [ClassData] attribute, introducing TestHelper.ComplexClassData<T> that can pass classes instead of ValueType data. - Instead of using the ! (null-forgiving) operator, introduce and use NotNull<T>() extension that throws a Xunit.Sdk.NotNullException if the object is null and otherwise returns the not-null object. (Note: the namespace deviates from the folder structure, so that it is available everywhere in the test project without the need to add a using statement.) - Improve logic in Assert of AllMathInputButtonsHaveLatexContent. - Add CSharpMath.Forms.Tests to CSharpMath.CrossPlatform.slnf. Maybe the newly created classes can be used in other parts of the solution as well, but I don't feel comfortable deciding that. Most of the code is very compact and adding classes feels like ignoring that coding style. Also, I am not very confident that those classes I have created meet the expected standards. I therefore put them "close to where they are used". * Update CSharpMath.CrossPlatform.slnf (suggested commit)) Co-authored-by: Hadrian Tang <hadrianwttang@outlook.com> * ButtonTests: use the [MemberData] attribute and MathKeyboardInput enum for individual testcases + do some cleaning - NotNull.cs: remove pragma directives and unused namespace usings. - Rename LatexHelper's vphantom to phantom, as the intention is to also add a tiny bit of horizontal spacing. - ButtonTests: use the [MemberData] attribute and remove TestHelpers.ComplexClassData. Use the MathKeyboardInput enum as method parameter type, since only xunit-serializable types will result in individual test cases. - Remove unused linked file from CSharpMath.Forms.Tests.csproj. - Restore deleted space from commit 4a56307, and remove another white line. (I committed the suggestion to see its result: I didn't understand why the space character seemed to be removed at the start of the line and I thought it had something to do with my Visual Studio settings. Now I know that the suggested change was removing the white line and that the removed space character was not the intention.) * Rename and move file from TestHelpers/NotNull.cs to /Extensions.cs * Add ButtonBase.TextColorProperty Xaml Tests to CSharpMath.Forms.Tests Notes: - I changed the namespace of Extensions.NotNull and made the class partial, so that it can be linked from any test project. - I am still in doubt about what is the right folder/project structure for the tests. Long description of this last comment: -- The name Xaml refers to the markup language and if code behind stuff should be included in that project, then maybe "UI" is a better name than "Xaml". -- The class names are very broad: for example, "Test" seems to say that that is the main file that should include all tests. But that is set up only for tests that are shared between Avalonia and Xamarin.Froms. When thinking about creating a file that is for testing Xamarin.Forms only, I bump into the fact that one file is called TestXamarinForms.cs but that file has a completely different purpose than being the container for unit tests. -- Postponing the decision to restructure - or actually awaiting to hear your preferences - I used external links. I wouldn't mind making the change or even thinking up a new the structure, but without that request I won't do that of course (as a newbie/guest to the project). * Add MathInputButton_Command unit test and do the suggested cleaning - Add MathInputButton_Command unit test. - The global XML Namespace is only needed for the outermost node. - Move a refactored NotNullExtension into ButtonTests.cs. * MathInputButton_Command test: use the MathInputButton's Keyboard getter * MathInputButton_Command test: no variable is needed anymore for the MathKeyboard instance * Revert "MathInputButton_Command test: no variable is needed anymore for the MathKeyboard instance" This reverts commit 132e19f. * Revert "MathInputButton_Command test: use the MathInputButton's Keyboard getter" This reverts commit fe0f460. * Add MathButton unit tests that check that the image color is correctly set in the StreamImageSource Probably needless to say: if ever the font changes, new images can created by adding the following method temporarily to the unit test and copying the result into the test project's "files/buttons" folder: static void StreamToFile(Stream s, FileInfo f) { using (var fs = f.Create()) { s.CopyTo(fs); }} * Attempt/test 1: also reference SkiaSharp from the CSharpMath.Forms.csproj * Attempt/test 2: also reference SkiaSharp from CSharpMath.Forms.Test.csproj * Attempt/test 3: Reference SkiaSharp & SkiaSharp.Views.Forms from test project (note: my previous commit message stated that I included SkiaSharp, but it was SkiaSharp.Views.Forms) * Revert "Attempt/test 3: Reference SkiaSharp & SkiaSharp.Views.Forms from test project (note: my previous commit message stated that I included SkiaSharp, but it was SkiaSharp.Views.Forms)" This reverts commit b43a974. * Revert "Attempt/test 2: also reference SkiaSharp from CSharpMath.Forms.Test.csproj" This reverts commit 350fc9b. * Revert "Attempt/test 1: also reference SkiaSharp from the CSharpMath.Forms.csproj" This reverts commit 8c486dd. * Update SkiaSharp and SkiaSharp.View.Forms to 2.80.2 in all referencing projects Introduces unwanted side effects: - Warning NU1605 Detected package downgrade: Xamarin.Forms from 4.6.0.772 to 4.3.0.908675. - A reference to a dll from CSharpMath.Forms.Tests, because somehow referencing the NuGet package wasn't enough. This seems to be a known bug: mono/SkiaSharp#1393. * Add libSkiaSharp.dll to test project CSharpMath.Forms.Tests Trying out a variation of workaround 3 from: mono/SkiaSharp#1041 (comment) * Revert "Add libSkiaSharp.dll to test project CSharpMath.Forms.Tests" This reverts commit 593d677. * Revert "Update SkiaSharp and SkiaSharp.View.Forms to 2.80.2 in all referencing projects" This reverts commit 31cc12a. * Customize buttons for MathKeyboardInput even more - Make the button display Latex related to the enum values of MathKeyboardInput overridable. (Not the math that may be displayed after pressing the button, but only the appearance of the button itself.) - Use a TextButton as a base class for MathInputButton. Then you have more options than when using a MathButton. (See the Example project, MyMathInputButton.cs where just the text "space" is used.) - Add another button type related to MathKeyboardInput (but unrelated to LaTeX): ImageSourceMathInputButton. This may help people be creative. (See the Example project, the Clear button now appears as a flame: Controls/ImageSourceMathInputButtons/flame.png.) - Add a unit tests: --- ImageSourceMathInputButton_InputProperty_KeyboardProperty_and_Command --- MathInputButton_KeyboardProperty * Make blinking Placeholder's Nucleus and ForeColor customizable in both CaretStates (#167) * Make blinking Placeholder's Nucleus and ForeColor customizable in both CaretStates * Renaming of Placeholder-related variables + refactor (#167) - Rename Placholder's "ForeColor" to "Color". - Use property initializer GlyphInfo.Foreground. - Restore "readonly field" instead of property getter for LaTeXSettings.Dummy. - Use the name parts "Resting" and "Active" in the placeholder setting names (instead of "Hiding" and "FullShow" which are related to the caret but do not fit a blinking placeholder). - Use variable name "placeholder" instead of "ph". * Add unit tests for customizable placeholder (#167) * Disable parallelization of customizable placeholder unit tests Also: verify more in LaTeXSettings_Placeholder_IsNewInstance. * Add unit test AllCustomizablePlaceholderPropertiesAreResetOnCaretVisible (#167) Also: in the MockTests class, verify that AttributedGlyphRun sets the GlyphInfo.Foreground to null (default color). * Unit test CustomizedPlaceholderBlinks: test complete cycle * Fix failing unit test CaretTimerResetsOnKeyPress * Use Assert.All instead of Assert.True(enumerable.All(pred)) * Revert "Fix failing unit test CaretTimerResetsOnKeyPress" This reverts commit 9925952. * Replace Assert.NotEqual + replace hardcoded strings by constants * Refactoring and cleaning (of customizable placeholder tests and more) * Placeholder tests: use Assert.NotSame and async Task * MathInputButtons with customized placeholders: what you see is what you get in the output In case MathInputButtons have the same TextColor as the output, the placeholders can be customized by only setting one or more of the LaTeXSetting properties PlaceholderActiveNucleus, PlaceholderRestingNucleus, PlaceholderActiveColor, PlaceholderRestingColor. In case of different keyboard colors than output colors then the MathInputButton properties PlaceholderActiveColor and/or PlaceholderRestingColor can be used to override the LaTeXSettings. Those two properties use a weird hack: BindableProperties cannot have nullable value types it seems. Nobody will ever use a transparent placeholder, so that is save to use as a replacement for null. The color black should be available, in case the TextColor is not black. This commit reverts the adding of MyMathInputButton of commit 94ebda2. The Example project's Editor tab is updated to have a "Change appearance" button on the EditorPage that does a round trip through 3 themes that show different Clear buttons, different colors on the keyboard keys and output, different placeholders on the keyboard and in the output. A large number of testcases has been added. It does not cover everything however. The PlaceholderColorsProperties_MathInputButton expects a weird LaTeX string (you see "{}" at index 24): @"\(\color{blue}{\square }{}^{\color{green}{■}}\)" * Correct comment * MathInputButton: only perform the placeholder color logic if defaults are overridden + refactor - At ButtonDraw: only perform the placeholder color logic if the placeholder properties (of LaTeXSettings or of MathInputButton) are not the default values (null). - Introduce NullableColorBindablePropertyHelper for nullable color workaround. * Comment out 2 ButtonTests that have Linux failures * Use FactAttribute.Skip * Delete NullableColorBindablePropertyHelper (I am afraid I did not try the right things before - it just works) * Revert "Delete NullableColorBindablePropertyHelper (I am afraid I did not try the right things before - it just works)" This reverts commit da8b291. * Use "foreach" instead of Xamarin.Forms.Internals.ForEach * First call SetButtonsTextColor and then SetClearButtonImageSource * Update CSharpMath.Forms.Example/CSharpMath.Forms.Example/Controls/MathKeyboard.xaml.cs Co-authored-by: FoggyFinder <FoggyFinder@yandex.ua> * Add unit tests MathButtonTextColorCanChangeMultipleTimes and MathInputButtonTextColorCanChangeMultipleTimes Also: - Skip only on Linux via "FactSkipLinux(reason)" instead of a general Fact(Skip = reason). - Move some helper methods into a separate file ButtonTestsHelper.cs. - Rename png used in unit tests, using more specific names. * Introduce interface IButtonDraw for bindablePropertyChanged - Move ButtonDraw from TextColor setter to TextColorProperty's propertyChanged event. - If calling ButtonDraw after casting to a base class, the subclass' override of ButtonDraw() is not executed, but via the IButtonDraw interface it is. - Move NullableColorBindablePropertyHelper to its own file. - ButtonTestsHelper refactor: extract method imageButton.ImageSourceAsStream(). Example project: - use PlaceholderBlinks setting in the thrid theme. - call ButtonDraw during each theme change (this is officially only needed for going from theme 1 to theme 2, but may also fix a not-understood bug). * Shut CSharpMath.Ios.Tests * add lock for now * Create and use method SubStringCount() instead of Regex.Matches().Count * Customized placeholder colors: add unit test Co-authored-by: Hadrian Tang <hadrianwttang@outlook.com> Co-authored-by: FoggyFinder <FoggyFinder@yandex.ua>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In pull request #164, posting #164 (comment), the idea came up to make the placeholder customizable in the keyboard output. While PR #164 is concerned with the display of the keyboard buttons themselves, this PR is about the math keyboard output.
Currently the placeholder has two possible appearances: a full black square and an empty square with a black border.
This pull request makes the Nucleus and the ForeColor of the placeholder customizable in both CaretStates.
An example of usage: instead of an empty square with a black border in the 'hiding' CaretState, you could choose a full square with another Color (gray, for instance).
An example test:
In CSharpMath.Forms.Example\CSharpMath.Forms.Example\EditorPage.xaml.cs
add the following lines at the top of the constructor of the EditorView:
Open the Example project and go to the Editor tab. Click the fraction button. The output is:
where the square in the numerator blinks, LightGray/Black.
I am willing to write unit tests after some form of preliminary approval of this pull request.
(And of course I will fix mistakes and refactor if needed.)