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

[Super Editor] - Change all newline insertions to use EditRequests #2500

Merged
merged 8 commits into from
Jan 11, 2025

Conversation

matthew-carroll
Copy link
Contributor

@matthew-carroll matthew-carroll commented Jan 10, 2025

[Super Editor] - Change all newline insertions to use EditRequests

Problem

Clients need to be able to customize what happens when a newline is inserted. For example, one client, on mobile, would like to be able to insert a single newline in a code block, but have a second newline exit the code block.

PR Goal

This PR refactors existing code so that a newline insertion will always look something like this:

editor.executeRequests([
  InsertNewlineAtCaretRequest(),
]);

I also added a request for soft newlines (within a block):

editor.executeRequests([
  InsertSoftNewlineAtCaretRequest(),
]);

Existing Places Where Newlines Were Used

A number of key handlers called into newline insertions.

Our document IME handler called into newline insertions.

Previous newline behaviors that were deleted or are no longer used:

CommonEditorOperations.insertBlockLevelNewline()

TextDeltasDocumentEditor._insertNewlineInDeltas()

text.dart: _insertBlockLevelNewline()

Impact on key handlers

Previously, the newline insertion logic was run directly from key handlers. This allowed the key handlers to know whether the newline was actually inserted or not, and then the key handler could report continueExecution if the newline wasn't inserted.

After moving this behavior into Editor requests, a key handler doesn't know what exactly happened after sending the request. Therefore, the key handlers now always halt execution. If this blocks some other desired behavior then we'll have to look deeper into how we can handle halts vs continues.

Impact on DocumentRange

I implemented a isCollapsed query on DocumentRange.

Removed unused blockquote and list item commands

As I upgraded all uses that I could find of the existing newline behavior, I found a few public blockquote and list item behaviors that seemed to be unused. I don't know if those were left there for others to use, or if we just forgot to delete them. For now, I've deleted them.

Other Changes in this PR

I added a few things to AttributedText:

/// Returns the character or placeholder at offset zero.
Object get first => placeholders[0] ?? _text[0];

/// Returns the character or placeholder at the given [offset].
Object operator [](int offset) => placeholders[offset] ?? _text[offset];

/// Returns the character or placeholder at the end of this `AttributedText`.
Object get last => placeholders[length - 1] ?? _text[length - 1];

I discovered that after the recent placeholder work, there wasn't a convenient way to say "give me the character or placeholder at index X". These additions solve that.

Migration Guide

This PR introduces breaking changes. Use the following instructions when migrating to deal with this change.

Update all request -> command mappings. Previously, those mappings took a request and returned a command. Now they take an editor and a request, and return a command:

// Old way
(request) => return MyCommand(request.stuff);

// New way
(editor, request) => return MyCommand(request.stuff);

This change was made because the specific command that should run for a newline differs based on where the caret sits in the document. We could have stuffed all such decisions into one big command, but that would make it more difficult for clients to customize the behavior. Instead, we now allow these mappings to inspect the document and composer before choosing a relevant EditCommand.

Some requests no longer support const.: To ensure consistent new node IDs for undo/redo, some requests now automatically generate a node ID in their constructor, which means they can't be const. This is done so that we don't have to force clients to always pass a new node ID. Most clients probably won't know that they should essentially always pass Editor.createNodeId(). So if no node is provided, we generate one automatically in the request. To migrate code, remove the const on those request constructors.

Copy link
Collaborator

@angelosilvestre angelosilvestre left a comment

Choose a reason for hiding this comment

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

LGTM with a couple comments.

Comment on lines +2385 to +2389
if (!documentSelection.isCollapsed) {
// The selection is expanded. Delete the selected content.
executor.executeCommand(DeleteSelectionCommand(affinity: TextAffinity.downstream));
}
assert(context.composer.selection!.isCollapsed);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the selection contains only non-deletable nodes, we might still have an expanded selection after executing the DeleteSelectionCommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I always forget about non-deletable content. I added an early exit if any non-deletable nodes are selected and I added a few tests.

@@ -258,7 +258,7 @@ class _UpstreamInlineMarkdownParser {
// Start visiting upstream characters by visiting the first character
// and checking for possible syntaxes.
for (final parser in parsers) {
final markdownToken = parser.startWith(attributedText.text[offset], offset);
final markdownToken = parser.startWith(attributedText[offset] as String, offset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we know that this offset is a character, isn't it more clear to just use .text[offset] instead of cast it to a String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.text[offset] is an Object. That access is into the AttributedText, which might have a character or a placeholder at that offset.

@matthew-carroll
Copy link
Contributor Author

CC @miguelcmedeiros @Jethro87 @jmatth - You may want to do some spot checks on various text editing actions after this PR. I tried to keep changes to a minimum, but there are probably a number of edges cases around selection and newline insertion that may have been impacted.

@matthew-carroll matthew-carroll merged commit 55db7de into main Jan 11, 2025
13 of 16 checks passed
@matthew-carroll matthew-carroll deleted the 2497_move-ime-actions-to-requests branch January 11, 2025 05:26
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.

2 participants