New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SuperEditor][Android] Hide expanded drag handles after deleting the selected text (Resolves #1936) #1951
Conversation
…selected text (Resolves #1936)
@@ -655,7 +656,38 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt | |||
widget.dragHandleAutoScroller.value!.ensureOffsetIsVisible(extentOffsetInViewport); | |||
} | |||
|
|||
void _onDocumentChange(_) { | |||
void _onDocumentChange(DocumentChangeLog changeLog) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial thought is that this a backwards place to recognize such a situation. We have various Android-specific widgets and configurations for the UI. The UI is where the user's interaction originates. Is there not a much simpler location in the Android-specific UI where we can recognize this situation, rather than try to reconstruct our knowledge of it after the fact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, even though we can hide the controls in the cut
handler, for the cases where the user presses the backspace button, or type when the selection is expanded, the IME will generate deltas that will modify the document, without any user interaction with the app. The user interaction in this case is only with the software keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the handles rebuild for the new collapsed selection, doesn't the Android interactor know that the user isn't currently dragging a handle, and therefore knows that we should collapse the handles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understood where you are suggesting to write this code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angelosilvestre can you please frame that as a question? Where are you getting confused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthew-carroll I'll take a look at the existing code to frame a question.
@angelosilvestre are you waiting for me on this? It looks like I had an open question. |
@matthew-carroll I modified the implementation as you suggested. |
@@ -1363,6 +1370,23 @@ class SuperEditorAndroidControlsOverlayManagerState extends State<SuperEditorAnd | |||
@visibleForTesting | |||
bool get wantsToDisplayMagnifier => _controlsController!.shouldShowMagnifier.value; | |||
|
|||
void _onDocumentChange(_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this only moved the problem I identified before. The problem is that you're trying to identify a user interaction through a document change. My original question, which is still my question, is why can't you identify the user interaction when the user interaction happens, instead of after the fact?
This issue says its about "deleting the selected text". I assume that involves an expanded selection, then some kind of keyboard interaction, then a collapsed selection.
Doesn't this widget already know when the selection is expanded, and when the selection is collapsed? Why can't the widget recognize that the selection has collapsed and then hide the expanded handles, without worrying about document changes at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…selected text (Resolves superlistapp#1936) (superlistapp#1951)
[SuperEditor][Android] Hide expanded drag handles after deleting the selected text. Resolves #1936
On Android with an expanded selection, performing any action that causes text to be deleted, such as cutting, deleting, pasting or typing to replace the text, keeps the expanded handles visible. Trying to drag those handles throws an exception:
The expanded handles are kept visible because of #1754, where we changed the expanded handles to be allowed to be visible even with a collapsed selection. However, this should happen when the user is dragging the handles, not when the selection collapses due to a deletion.
Looking at other Android apps, it seems that the expected behavior is that no handle should be visible after deleting the selected text, not even the collapsed drag handle.
This PR changes the Android touch interactor to hide the expanded handles and any other controls when text is deleted, causing the selection to collapse.
We could also implement this by adding a listener directly to the editor and then listening for selection change events. If a better solution would be to find a way to make
shouldShowExpandedHandles
to befalse
if the user is not dragging then I can dig into that.On iOS this issue doesn't happen, because the expanded handles are never visible when the selection is collapsed. I added the same test I added for android.