Add TextColor property to BaseButton and make MathInputButton's color customizable #164
Conversation
… 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.
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 86.71% 87.09% +0.38%
==========================================
Files 157 159 +2
Lines 11200 11261 +61
==========================================
+ Hits 9712 9808 +96
+ Misses 1488 1453 -35
Continue to review full report at Codecov.
|
|
Not a fan of use-once functions. @FoggyFinder? |
|
Also would be nice if some new tests related to this change are added to CSharpMath.Xaml.Tests. |
Yep, as well |
|
OK, I will restore that and make some unit tests.
Op wo 16 sep. 2020 09:15 schreef FoggyFinder <[email protected]>:
… Not a fan of use-once functions. @FoggyFinder
<https://github.com/FoggyFinder>?
Yep, as well
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#164 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHVGU6X5NYQCY6PN6RKGSM3SGBQZ7ANCNFSM4RN5WWJA>
.
|
…tton 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).
|
Oops, I didn't read "CSharpMath.Xaml.Tests". While trying to find out where to write the unit tests, I was in doubt about the "Xaml" meaning. I thought that test project was meant for tests more related to the xml. I have created a test project "CSharpMath.Forms.Tests" in the "Xamarin.Forms" folder.
|
…nclude 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".
Co-authored-by: Hadrian Tang <[email protected]>
…m 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.)
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).
|
I really appreciate the feedback you are providing. This is the first pull request I've ever done, and I am surprised about the quick responses and thorough reviews, providing kind and helpful suggestions to make the code compact and clean. You also care about the code quality even more than I'd expected - commenting about a white line for example and using |
There was a problem hiding this comment.
It's because I am a victim of dead air in pull requests...
kostub/iosMath#142
LayoutFarm/Typography#192
fsharp/fslang-design#442
MicrosoftDocs/visualfsharpdocs#326
And you will most definitely face similar scenarios as well in the future.
Regarding code style, thanks - less code means fewer chances of bugs.
- Add MathInputButton_Command unit test. - The global XML Namespace is only needed for the outermost node. - Move a refactored NotNullExtension into ButtonTests.cs.
…athKeyboard instance
|
Maybe I should only have committed "Add MathInputButton_Command unit test and do the suggested cleaning 99417ce". |
Yep, still the same |
- 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).
|
No luck @Happypig375 @charlesroddie any idea what's going on? Does theme changing work correctly for you? |
|
Core Tests results:
The test fail errors are not related to this PR. Test Core fail reason:
Test iOS fail reason:
|
|
@FoggyFinder I suspect that it is solved now via commit 486ac0f and I hope that calling ButtonDraw during each theme change in the example project is acceptable. Officially it is only needed to go from theme 1 to theme 2. |
|
@FoggyFinder @SymboLinker I'll test this PR tomorrow |
@Happypig375 It's more than half a year later now. Could you please share why this PR has not been tested / has not gotten any response? I would like to know that, so that I can do better next time (in other public repositories). |
|
It's just me focusing on other projects than this. I'll just merge this. |
|
@FoggyFinder Just missing your approval |
I'm doing some testing. I totally newbie with Regex so getting help with those who know at least something (pretty much to my opinion). Expect feedback / commit with fix proposal or. at least. tests that are failing within 1-2 days. |
|
|
Exactly. I thought "Aha. Now I realize why it behaves like this. And then - I can write simple Regex to fix it quickly. Well it ended with debug why a regex doesn't work properly in some case and then another regex and so on until the point when I totally no idea what the regex pattern means itself". Indeed I prefer not to use Regex and not only I'm very bad with them. But...you've been waiting for so long...my intention was to fix it ASAP, merge in and then, probably, rewrite without Regex. |
|
The issue with display (like I saw brown color despite the theme is black) is the fact that button.TextColor = color;
button.PlaceholderRestingColor = placeholderRestingColor;
button.PlaceholderActiveColor = placeholderActiveColor;
button.ButtonDraw(/* If the LaTeXSettings change but the button's appearance properties don't, there's no event that causes the execution of this method. */);So when we store "original" latex in a variable it might be previously mutated LaTeX that actually contains color etc attributes. |
|
I'm not a big fan of lock either but it was much easier than applying odd regex to prevent latex corruption. |
|
EDIT: It surprises me that calling a synchrounous method multiple times synchrounously would require a lock to work. I guess that - for example - a part of a painting method is asynchrously without exposing that in its signature. I’m glad that you’ve found a fix. Thanks for investing the time :) |
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 87.34% 87.68% +0.33%
==========================================
Files 156 161 +5
Lines 11329 11576 +247
==========================================
+ Hits 9895 10150 +255
+ Misses 1434 1426 -8
Continue to review full report at Codecov.
|
FoggyFinder
added
the
Resolution/Implemented
|
Thanks @Happypig375 and @FoggyFinder! |
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:
CSharpMath.Forms.Example (MathKeyboard.xaml):