Remove blinking placeholder #176
Conversation
|
We have too many tests |
|
Yes, obviously we should have zero tests. |
|
Zero is too few |
|
We shouldn't test things that aren't required for correctness |
|
Current tests take a lot of current behavior and test for them. So the only thing they do is to make making changes slower. |
|
Tests catch unintentional changes in behaviour, i.e. bugs. Therefore, correctness is maintained. |
|
Change tests to ensure the new behaviour is maintained |
|
I don't want to be Xamarin.Forms where regressions happen all the time |
It's not possible to test for intentionality in the changes. For example tests failed here but I intended the changes. So this psychological aspect of testing is not valid. As far as possible tests should test for correct behaviour and it should be clear why it should pass. |
|
If it's intentional, then change tests to match the new behaviour. Any code can cause action at a distance by e.g. mutating global variables, and tests ensure that this does not happen. |
|
The tests here were easy to adjust (partly because I know that tests tend to be deletable in this repo and shouldn't be given too much respect) but in other places they place significant obstacles to doing PRs. Someone making a PR encouters a failed test, and then it's not clear whether the test indicates a problem in the PR's logic. And it's not clear whether it's his responsibility to get the tests to pass by changing the logic, or his responsibility to get the tests to pass by changing the tests, or @Happypig375 's responsibility to get the tests to pass by changing the tests. |
|
Refactors which produce the same output won't need test changes. You are asking for a change in behaviour. Any PR that ask for changes in behaviour must also change tests to match the new behaviour. |
|
And I am under no obligation to accept any pull request that potentially introduces a regression by deleting tests. |
|
You can make the appearance of the placeholder's 2 states equal via Atom.LaTeXSettings by customizing the so called "active" and "passive" placeholder nuclues. (Assuming you use the default color black, you don't need to do anything with the color.) |
|
@SymboLinker no changing PlaceholderRestingNucleus also changes how an unselected placeholder looks. |
|
If some users like blinking placeholder let allow them to use it. |
|
Does one of us need blinking placeholders? The fact that the main latex editors out there don't have it suggest that it shouldn't be there even as a setting, but if there is strong demand from one of us here we could have a setting. If the only reason is that someone in future might possibly use it, that's too weak. |
I am a user of it. Within a month or so I will publish an update for my app that includes a math keyboard with blinking placeholder. (I won't name the app, as I think it's unappropriate to advertise it this way.) |
|
I added a setting PlaceholderBlinks for you @SymboLinker . But now it wouldn't be invalid to have tests (testing correct application of application the setting). Not essential as the logic is extremely trivial but @Happypig375 might require. I don't like the complexity but can I leave the testing part of this feature to you (possibly having to write tests) and @Happypig375 (possibly requiring them)? Otherwise we can have this without the PlaceholderBlinks setting (since it improves the default), and add an issue to add the setting which should be doable in the timeframe you give. |
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
==========================================
- Coverage 87.33% 87.29% -0.04%
==========================================
Files 156 156
Lines 11312 11280 -32
==========================================
- Hits 9879 9847 -32
Misses 1433 1433
Continue to review full report at Codecov.
|
This reverts commit ab05e8f.
|
I've reverted the PlaceholderBlinks so that this is in an I think mergeable state. @Happypig375 can you indicate what tests if any you would need to have a PlaceholderBlinks setting and then @SymboLinker if it's worth doing those tests to get this setting in? If tests are needed, and it's worth writing them, then revert the revert and do the tests. Thanks. |
|
Superseded by #177 |
Happypig375
added
Resolution/Superceded
Word, MathQuill, and LyX don't have blinking placeholders.
They are visually unattractive and not useful UX because a large block blinks which is already easy to identify, unlike the cursor where a small line blinks which is harder to identify.