Repository navigation
Delete swipe: horizontal-right deselect after a vertical-only swipe - #686
Merged
Merged
Conversation
Owner
|
Oh thats what you meant earlier |
Contributor
Author
|
Did you try it, what do you think? Need some fix or? |
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.
Related: issue #467 (follow-up comment about vertical delete swipe).
Problem
After a purely vertical delete swipe (selection grows line by line upwards), swiping right on the delete key did nothing. The user could only shorten the selection on the reached line if the first swipe had been diagonal (i.e. it had also moved left), because rightward movement could only undo a previous leftward movement.
Root cause
KeyboardActionListenerImplkeeps the delete-swipe selection as a line index plus a horizontal offset, not as a real selection start:deleteSwipeLineIndexis changed by vertical steps.deleteSwipeCharOffset(char mode) /deleteSwipeWordIndex(word-by-word mode) is changed by horizontal steps.Both horizontal values were clamped with
coerceAtLeast(0). After a vertical-only swipe they are0, so a rightward step (stepsX > 0) was clamped back to0and no selection change happened.Chosen approach
Allow the horizontal value to become negative while the selection base is on a line above the swipe start. Negative means "selection start is to the right of the line start", i.e. already selected text on that line is deselected again. This is the same behavior the diagonal swipe already had, now also available after a vertical-only swipe.
The alternative (making the vertical selection itself more precise) is not reliable, because line starts are estimated (
getLineBoundariesBackwardswraps long lines at ~40-50 chars at whitespace) and do not match the editor's real visual lines.Changes
All in
app/src/main/java/helium314/keyboard/keyboard/KeyboardActionListenerImpl.kt:onMoveDeletePointerChars2DdeleteSwipeCharOffsetis no longer0but-(selectionAnchor - lineBase), whereselectionAnchoris the selection start when the swipe began andlineBaseis the start of the currently used line. The selection start therefore can never move to the right of where the swipe started, so text that was selected before the swipe is never deselected.lineIndex == 0)lineBase == selectionAnchor, so the bound is0and behavior is identical to before.0(the selection is re-anchored at the start of the new line).onMoveDeletePointerWords2DdeleteSwipeWordIndex: negative values select the n-th word boundary to the right of the line start, limited to the boundaries that exist between the line start and the swipe start. Same reset to0on a line change.New helper
getWordBoundariesForwards(text, textEndOffset, fromOffset)in the companion object. ReusesgetWordBoundariesBackwards, filters positions> fromOffsetand reverses them, so selecting (left) and deselecting (right) step over exactly the same positions.Small refactor: the repeated "line base" expression became
deleteSwipeLineBase(), anddeleteSwipeSelectionAnchor()was added. No behavior change from the refactor itself.Tests:
WordDeleteSwipeTest.ktgets 4 new tests forgetWordBoundariesForwards(ordering, empty cases, consistency with the backward boundaries, non-zero text offset).What is intentionally not changed
KeyCode.FORWARD_DELETE/FORWARD_DELETE_WORD): handled inonCodeInput, not in the delete-swipe code. Untouched.PointerTracker.kt, release notes v4.2.9-beta5):PointerTracker.ktis not modified. It still zeroes accidental upwardstepsYduring a rightward swipe beforeonMoveDeletePointeris called. SincestepsXis computed independently of that filter, the two changes do not interact.ClipboardHistoryView,HandwritingView) use the 1-argumentonMoveDeletePointer(steps), which passesstepsY = 0. They can never leave line index 0, where the new lower bound is0, so their behavior is unchanged.0as before, so swiping right on the delete key without having selected anything still does nothing.Resulting behavior
Not verified
./gradlew testDebugUnitTest(at leastWordDeleteSwipeTest) and try the gesture on a device in both char mode and word-by-word mode, including a long wrapped line.Suggested release note: "Delete swipe: after a vertical swipe, swiping right now deselects text on the reached line (per character or per word)."