Refactor the way 2SCM support multiple attributes. Enable it for code. #7577
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
as the multi-attribute caret handling needs to be orchestrated by TwoStepCaretMovement class. Avoid exposing implementation details into API for no specific reason.
This was referenced Jul 8, 2020
jodator
reviewed
Jul 14, 2020
This was referenced Jul 22, 2020
jodator
approved these changes
Jul 22, 2020
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.
Suggested merge commit message (convention)
Other (typing, basic-styles): Enable two-step caret movement for Code, refactor the way it handles multiple attributes. Closes #6722. Closes #7578.
MINOR BREAKING CHANGE (typing):
TwoStepCaretHandlerclass was removed.Additional information
1. Behavioral change #7578
I think the current behavior in regards of moving between different attributes is counter-intuitive.
<i>foo[]</i><b>bar</b>moves to the<i>foo</i><b>[]bar</b><a href="f">foo[]</a><a href="b">bar</a>moves to the<a href="f">foo</a>[]<a href="b">bar</a>As a user, I find this inconsistency confusing.
You are able to type in between the same attribute but different values, but not between two different attributes.
If it would be inconsistent, I 'd rather make it the other way around. If it's the same attr, I'd just move between attribute values, not insert a character without any value. Then, as switching between entire attrs drops the first attribute anyway, I could expect writing attr-free here.
Needless to mention, that such inconsistency requires way more complicated code.
2. Tests
I do not want to duplicate tests from
TwoStepCaretMovementandInlineHighLighttoCodeEditing. I was considering having extensive tests for 2SCM and highlight alone, then tests thatCodeEditinguses these APIs correctly and some basic high-level tests for Code.Currently, with
TwoStepCaretMovementandLinkEditing, it's set up in the wat 2SCM has extensive tests, Link Editing has only ~two tests for basics, but no tests that it actually uses 2SCM or inline highlight API.The idea was dismissed at https://cksource.slack.com/archives/C03URJ97S/p1593637492006600.
3. right-to-left
Previously there was only 1 test for RTL case, which was testing that some private method is called. With this PR, I replace it with 1 test for basic use-case behavior, closer to the user perspective. If we want to have full RTL coverage, I'd rather refactor the tests to generate the scenarios. I consider it out of the scope of this PR.