[SuperEditor] Add more specific SelectionChangeTypes (Resolves #2367)#2589
Open
angelosilvestre wants to merge 2 commits into
Open
[SuperEditor] Add more specific SelectionChangeTypes (Resolves #2367)#2589angelosilvestre wants to merge 2 commits into
angelosilvestre wants to merge 2 commits into
Conversation
Collaborator
Author
|
@JostSchenck Could you please try this PR to see if it works for you? |
Contributor
|
@angelosilvestre Thanks a lot. Trying it out now, but will respond to the issue for the context there. |
Contributor
|
@angelosilvestre This PR solves my problem, thank you very much. |
|
|
||
| /// Place the caret based on a desire to move the previous caret position upstream or downstream. | ||
| pushCaret, | ||
| /// Place the caret based on a desire to move the previous caret position downstream. |
Contributor
There was a problem hiding this comment.
I think I/we made these descriptions too verbose to begin with. There are so many words that it actually confuses the meaning.
Let's go with: "Move the caret one unit downstream."
Please attempt to adjust the other descriptions to have similar concision for the sake of clarity.
Contributor
|
@angelosilvestre @matthew-carroll Is there a chance this will be merged in the near future? |
This file contains hidden or 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
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.
[SuperEditor] Add more specific SelectionChangeTypes (Resolves #2367)
Currently, when moving the caret using up or down arrow, the editor generates a selection change event with the type
SelectionChangeType.collapseSelection. This makes it difficult for reactions to determine if the selection moved up or down.Also, our
SelectionChangeTypes for pushing the caret or extent don't carry the direction of the selection change.This PR removes
SelectionChangeType.pushCaretandSelectionChangeType.pushExtentin favor of more specific types:SelectionChangeType.pushDownstreamSelectionChangeType.pushCaretUpstreamSelectionChangeType.pushCaretDownSelectionChangeType.pushCaretUpSelectionChangeType.pushExtentDownstreamSelectionChangeType.pushExtentUpstreamSelectionChangeType.pushExtentDownSelectionChangeType.pushExtentUpThis is a breaking change, but I think it's much more safer to generate a compilation error instead of keeping
SelectionChangeType.pushCaretUpand possibly silently failing at runtime.The upgrade should be straightforward:
Before
After:
On Android, we are always generating a selection change type of
SelectionChangeType.pushCaret. This PR changes it toSelectionChangeType.placeCaret, but it might be better to let that decision to the touch interactor instead.On web, things are a bit trickier. We handle selection changes by IME deltas, so we don't have information about the intent of the selection change. Should we compare the new selection, with the selection that is expected when the user presses up/down/left/right arrow to determine what is the correct selection change type?