[SuperEditor][Android] Honor handle builders (Resolves #1934) #2272

angelosilvestre · 2024-08-26T23:24:10Z

[SuperEditor][Android] Honor handle builders. Resolves #1934

SuperEditorAndroidControlsController exposes collapsedHandleBuilder and expandedHandlesBuilder, but neither of them are being used anywhere.

This PR changes the android touch interactor to honor the provided builders.

The drag handles involve various gesture callbacks to place/expand/collapse the selection, start/stop the blinking caret, show/hide the toolbar, among other things. It would be very difficult for developers to do that on their own.

Because of that, this PR introduces an object called DocumentHandleGestureCallbacks, which holds the gesture callbacks that should be attached to the drag handle. Developers would wrap the handle with a GestureDetector and set each property to its corresponding GestureDetector callback.

I needed to modify the expanded handles selection logic, because after these changes, when the user starts dragging the expanded handle, the pan update event is trigger before the user moves the drag handle to another character, so we couldn't determine if the user is dragging upstream or downstream. This breaks our logic of selecting by word or character.

The original ticket also mentions that we use magnifierBuilder and toolbarBuilder in different ways. For magnifierBuilder, we expect that developers will include the Follower in the returned widget, while in toolbarBuilder we wrap the returned widget with a Follower. Letting the developers attach the Follower to the tree seems more logical to me, because it would be able to customize the offset and anchors. But changing that now seems like a big breaking change. Should we do that?

matthew-carroll · 2024-08-26T23:45:15Z

@angelosilvestre is this PR both connecting the existing builders, and also modifying the tooling to build them? If so, can you split this into two PRs: one for fixing the existing bug, and another for whatever improvements you want to make?

angelosilvestre · 2024-08-27T00:01:51Z

@matthew-carroll I think that at least we should keep DocumentHandleGestureCallbacks in this PR. Otherwise it will be very impractical for anyone to use a custom handle, since the drag handle selection depends on the drag handle gesture callbacks.

angelosilvestre · 2024-08-27T23:00:13Z

@matthew-carroll Updated removing AndroidDefaultFollowingCollapsedHandle from this PR.

matthew-carroll · 2024-09-14T17:29:04Z

@angelosilvestre can you update the PR description so that it only describes the changes in this PR after you split this PR into two?

angelosilvestre · 2024-09-16T22:30:30Z

@matthew-carroll Updated the description.

matthew-carroll · 2024-09-22T20:12:46Z

@angelosilvestre since you focused in this PR around making it simpler and easier to create custom handles, can you add a code sample to the PR description that shows a minimum setup for a custom drag handle? That will probably help evaluate those decisions.

WRT the Follower issue, let's file a new issue for that and handle it later. We can make the breaking change when it seems like a good time to do that. We've got a lot of stuff in flight right now.

matthew-carroll · 2024-10-18T19:07:56Z

@angelosilvestre did you update the description? I see that my last comment was the last activity on this thread, so I wasn't sure if that happened, or not.

angelosilvestre · 2024-10-18T22:45:00Z

@angelosilvestre since you focused in this PR around making it simpler and easier to create custom handles, can you add a code sample to the PR description that shows a minimum setup for a custom drag handle? That will probably help evaluate those decisions.

In this PR I focused on just making the handle builder be honored. Making it simpler and easier would be moved to a separate PR, right?

matthew-carroll · 2024-10-18T22:54:34Z

I would defer to the earlier conversation in the thread. I think that's all the conversation we've had about this. What I don't know is whether your current PR description describes what's actually in this PR, or whether the description is still about the original PR, which included stuff that you later removed.

angelosilvestre · 2024-10-18T22:59:21Z

I updated the description.

matthew-carroll · 2024-10-18T23:16:38Z

I needed to modify the expanded handles selection logic, because after these changes, panning was triggering the pan update while the handle still focused the initially focused character, causing the selection strategy to be confused to determine if the user was dragging upstream or downstream.

I'm not sure what this means. Can you try to describe this in a clearer way?

angelosilvestre · 2024-10-18T23:33:05Z

Updated it again. Does it look better now?

matthew-carroll · 2024-10-18T23:36:45Z

Ok, now it reads:

I needed to modify the expanded handles selection logic, because after these changes, when the user starts dragging the expanded handle, the pan update event is trigger before the user moves the drag handle to another character, so we couldn't determine if the user is dragging upstream or downstream. This breaks our logic of selecting by word or character.

Wouldn't it be accurate to say if the user moves left, or up its an upstream selection, and if the user moves right or down then it's a downstream selection? There should probably be a little slop before making that choice, but we shouldn't have to wait for the handle to actually move for us to know the direction of the drag, right?

angelosilvestre · 2024-10-19T19:51:52Z

Wouldn't it be accurate to say if the user moves left, or up its an upstream selection, and if the user moves right or down then it's a downstream selection? There should probably be a little slop before making that choice, but we shouldn't have to wait for the handle to actually move for us to know the direction of the drag, right?

The change is basically adding that left/right move handling. Do we need to change the whole behavior on this PR?

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

matthew-carroll · 2024-10-20T18:39:11Z

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

-                key: DocumentKeys.upstreamHandle,
-                handleType: HandleType.upstream,
-                color: _controlsController!.controlsColor ?? Theme.of(context).primaryColor,
+  Widget _buildExpandedHandles() {


Do we need to change this from List<Widget> to Widget? I'm hesitant to add more layout widgets when not needed, because it increases the odds that at some point something isn't aligned correctly because of we've got a Stack in a Stack in a Stack, etc.

We can keep it as a list. Reverted.

matthew-carroll · 2024-10-20T18:42:35Z

super_editor/lib/src/default_editor/document_gestures_touch_android.dart

+
+        return Stack(
+          children: [
+            Follower.withOffset(


When you mentioned in the issue ticket that it's difficult for users to configure their own handles, did you include the Follower in your description of it being difficult? I.e., did you want to wrap the custom handle with a Follower so the user doesn't have to? Or is it better like this to have the user provide the Follower?

I think we should have the user provide the Follower, to have full control on the alignment, expansion, etc. We could add a default following handle to make it easier for those who don't need this kind of control.

super_editor/lib/src/infrastructure/platforms/mobile_documents.dart

matthew-carroll · 2024-10-20T18:46:36Z

super_editor/test/super_editor/mobile/super_editor_android_overlay_controls_test.dart

@@ -304,6 +305,91 @@ void main() {
      expect(SuperEditorInspector.isCaretVisible(), true);
    });

+    testWidgetsOnAndroid("allows customizing the collapsed handle", (tester) async {


Given that most of this PR is based on delegating gesture behavior, should this PR add a number of tests that use those gestures? Or is that fully covered by existing tests?

Also, why are we only testing for Android?

We currently have test ensuring that dragging the expanded handle changes the selection. Should we add a test for the collapsed handle? We have properties for handle customization on Android only.

Seems like tests for the collapsed handle would be as important as for expanded handles.

matthew-carroll · 2024-10-20T18:47:44Z

super_editor/test/super_editor/supereditor_test_tools.dart

@@ -309,6 +309,18 @@ class TestSuperEditorConfigurator {
    return this;
  }

+  /// Configures the [SuperEditor] to use the given [builder] as its android collapsed handle builder.
+  TestSuperEditorConfigurator withAndroidCollapsedHandleBuilder(DocumentCollapsedHandleBuilder? builder) {


Why only for Android?

The original ticket is only about Android. We don't have handle builders for iOS.

matthew-carroll · 2024-11-16T20:30:56Z

super_editor/lib/src/infrastructure/platforms/mobile_documents.dart

@@ -23,12 +23,24 @@ class DocumentKeys {
 /// The [handleKey] is used to find the handle in the widget tree for various purposes,
 /// e.g., within tests to verify the presence or absence of the handle.
 ///
+/// The [gestureDelegate] hold event handles that should be attached to the gesture recognizer


This paragraph is confusing - I'm not sure what exactly it's trying to say.

"The [gestureDelegate] hold event handles". First, it sounds like "hold" -> "holds". Beyond that, I don't know what it means to "hold event handles"...do you mean "handlers"? If so, the delegate doesn't "hold event handlers", a delegate "defines handlers for gesture events that occur on a given handle".

"For example, set [gestureDelegate.onTap] to the handle's onTap event."

I don't know what this is telling me to do. How do I set a method on the handle's onTap? Is this a statement about what I need to do as a reader, or a description of what is happening behind the scenes? I can't tell.

matthew-carroll · 2024-11-16T20:32:35Z

super_editor/lib/src/infrastructure/platforms/mobile_documents.dart

+/// The [gestureDelegate] hold event handles that should be attached to the gesture recognizer
+/// attached to the handle. For example, set [gestureDelegate.onTap] to the handle's `onTap` event.
+///
+/// Use [shouldShow] to fade in/out the handle entrance/exit, for example, using an [AnimatedOpacity]


This paragraph is also unclear. When reading this, I can't really tell what you want me to do, or what Super Editor does. I generally understand that something is going on with fading, but there seems to be like 3 distinct details that are smushed into one statement and it's confusing.

matthew-carroll · 2024-11-16T20:36:54Z

super_editor/lib/src/infrastructure/platforms/mobile_documents.dart

+/// These callbacks are intended to make it easier for developers to customize
+/// the drag handles, without having to re-implement the gesture logic.
+///
+/// To use it, for example, wrap the handle in a `GestureDetector` and pass


Can you replace this last sentence with a minimal code example showing this in use?

angelosilvestre requested a review from matthew-carroll August 26, 2024 23:40

matthew-carroll reviewed Oct 20, 2024

View reviewed changes

angelosilvestre added 3 commits November 13, 2024 16:15

[SuperEditor][Android] Honor handle builders (Resolves #1934)

861eded

PR updates

0ef71c7

PR updates

a309fc8

angelosilvestre force-pushed the 1934_android-handle-builders-unused branch from 1074a91 to a309fc8 Compare November 13, 2024 19:22

angelosilvestre requested a review from matthew-carroll November 13, 2024 19:26

matthew-carroll reviewed Nov 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperEditor][Android] Honor handle builders (Resolves #1934) #2272

[SuperEditor][Android] Honor handle builders (Resolves #1934) #2272

angelosilvestre commented Aug 26, 2024 •

edited

Loading

matthew-carroll commented Aug 26, 2024

angelosilvestre commented Aug 27, 2024

angelosilvestre commented Aug 27, 2024

matthew-carroll commented Sep 14, 2024

angelosilvestre commented Sep 16, 2024

matthew-carroll commented Sep 22, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 18, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 18, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 18, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 19, 2024

matthew-carroll Oct 20, 2024

angelosilvestre Nov 13, 2024

matthew-carroll Oct 20, 2024

angelosilvestre Nov 13, 2024

matthew-carroll Oct 20, 2024

angelosilvestre Nov 13, 2024

matthew-carroll Nov 16, 2024

matthew-carroll Oct 20, 2024

angelosilvestre Nov 13, 2024

matthew-carroll Nov 16, 2024

matthew-carroll Nov 16, 2024

matthew-carroll Nov 16, 2024

[SuperEditor][Android] Honor handle builders (Resolves #1934) #2272

Are you sure you want to change the base?

[SuperEditor][Android] Honor handle builders (Resolves #1934) #2272

Conversation

angelosilvestre commented Aug 26, 2024 • edited Loading

matthew-carroll commented Aug 26, 2024

angelosilvestre commented Aug 27, 2024

angelosilvestre commented Aug 27, 2024

matthew-carroll commented Sep 14, 2024

angelosilvestre commented Sep 16, 2024

matthew-carroll commented Sep 22, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 18, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 18, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 18, 2024

matthew-carroll commented Oct 18, 2024

angelosilvestre commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre commented Aug 26, 2024 •

edited

Loading