Skip to content
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] Add tap handler to add an empty paragraph when tapping below the end of document (Resolves #2395) #2397

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

angelosilvestre
Copy link
Collaborator

@angelosilvestre angelosilvestre commented Nov 5, 2024

[SuperEditor] Add tap handler to add an empty paragraph when tapping below the end of document. Resolves #2395

Currently, tapping below the end of the document places the caret at the end of the last selectable component.

This PR modifies the ContentTapDelegate to expose the DocumentLayout, so apps can query the information necessary to make decisions.

This PR also introduces a SuperEditorAddEmptyParagraphTapHandler, so apps can choose to add an empty paragraph to the end of the document instead.

This is a breaking change, but resolving it is pretty simple:

-TapHandlingInstruction onTap(DocumentPosition tapPosition) {
+TapHandlingInstruction onTap(DocumentTapDetails details) {
+  final tapPosition = details.documentLayout.getDocumentPositionNearestToOffset(details.layoutOffset);
+  if (tapPosition == null) {
+    return TapHandlingInstruction.continueHandling;
+  }

Also, now multiple factories are allowed, which is also a breaking change, solvable with:

SuperEditor(
- contentTapDelegateFactory: superEditorAddEmptyParagraphTapHandlerFactory,
+ contentTapDelegateFactories: [superEditorAddEmptyParagraphTapHandlerFactory],
)

@matthew-carroll
Copy link
Contributor

This is a breaking change, but resolving it is pretty simple.

When creating a breaking change with a known solution, please demonstrate that change in the PR so that readers can come here and copy the fix.

final DocumentPosition position;

/// Whether the gesture occurred above the first node in the document.
final bool isGestureAboveStartOfDocument;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely far too specific to this one particular goal. There could be 100 different queries that apps might want. We'd end up with 100 properties.

Can you please propose an alternate approach? Can we pass a reference to the document layout and let the implementer check whatever they want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a alternative approach, placing the checks directly on the handler.

@matthew-carroll
Copy link
Contributor

I'm still curious about the concept of passing the document layout. Can you think of any reason why we wouldn't want to pass the layout with the event?

We're currently reporting the "tap position" in terms of the tapped document node. But this is already a bit of overreach on our end. What if the handler wants to find the "nearest position"? Or, if we're currently looking up the nearest position, what if the handler only wants to consider positions that are actually touched?

So I'm wondering, what if we pass the document layout and the global offset? Can you think of any downsides from that? Any hurdles that might get in our way?

@angelosilvestre
Copy link
Collaborator Author

Can you think of any downsides from that? Any hurdles that might get in our way?

I don't have anything against passing the document layout.

Are you saying we should remove tapPosition and let implementers obtain this information directly from the DocumentLayout?

@matthew-carroll
Copy link
Contributor

Are you saying we should remove tapPosition and let implementers obtain this information directly from the DocumentLayout?

Yeah, I think we can leave it up to the client to decide what info they wanna pull out of the layout.

@angelosilvestre
Copy link
Collaborator Author

@matthew-carroll Updated.

@@ -1723,3 +1729,82 @@ class SuperEditorLaunchLinkTapHandler extends ContentTapDelegate {
return null;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this comment here, but it's related to the delegate, itself.

Should we switch to a chain of responsibility so that multiple delegates can be provided? If we don't, and a developer wants multiple situations to be considered, then that developer will have to implement his own chain of responsibility.

Is there any reason that it's important for us to have only a single delegate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to multiple delegates. We would already need multiple delegates for the spellcheck PR.

@@ -1723,3 +1729,82 @@ class SuperEditorLaunchLinkTapHandler extends ContentTapDelegate {
return null;
}
}

SuperEditorAddEmptyParagraphTapHandler superEditorAddEmptyParagraphTapHandlerFactory(SuperEditorContext editContext) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably doesn't belong in super_editor.dart - we should create a more appropriate source file for such things, or add it to a related existing file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@angelosilvestre angelosilvestre force-pushed the 2395_tap-handler-add-empty-paragraph-to-end branch from 66f8179 to 5e89455 Compare November 18, 2024 15:46
@matthew-carroll
Copy link
Contributor

@miguelcmedeiros would you like to sign off on this PR before we merge it? I think it generally looks good to me, but would be good for you to verify that it works for you.

Copy link
Collaborator

@miguelcmedeiros miguelcmedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matthew-carroll matthew-carroll merged commit f027d60 into main Nov 20, 2024
13 of 14 checks passed
@matthew-carroll matthew-carroll deleted the 2395_tap-handler-add-empty-paragraph-to-end branch November 20, 2024 18:40
github-actions bot pushed a commit that referenced this pull request Nov 20, 2024
matthew-carroll pushed a commit that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SuperEditor] - Add empty paragraph at the end of document when tapping on the empty area below it
3 participants