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

Don't merge history entries from different editors #2873

Merged
merged 3 commits into from
Aug 24, 2022
Merged

Conversation

acywatson
Copy link
Contributor

@acywatson acywatson commented Aug 23, 2022

Merging history entries from different editors can lead to a situation where the initial state update of a nested editor is merged with a change from the parent editor, leading to the EditorState with containing just the parent editor change never going onto the undo stack.

Example is an ImageNode that eagerly renders a nested editor caption with initial state set to undefined. This causes the following series of events:

  1. editor.update -> insertImageNode
  2. nestedEditor.update -> initializeEditorState
  3. LexicalHistory updateListener (1) -> applyChange
  4. HISTORY_PUSH -> put state before image node insertion onto undo stack, set historyEntry.current to new state with image inserted.
  5. LexicalHistory updateListener (2) -> applyChange
  6. HISTORY_MERGE (because of tag) -> don't push anything onto undo stack, set historyEntry.current to nestedEditor state.

This means the EditorState where the ImageNode exists is never put onto the undo stack, so if you delete the ImageNode subsequently, you can't undo and get it back.

To fix this, we only merge two entries if the editors are the same. The downside of pushing instead of mergin is that updates 1 and 2 are treated separately on the history stack, so the user perceives one action - inserting an image, but when they undo, they need to undo twice to actually get back to the state without the image (the first undo restores the same state from the stack - it gets put there because the nested editor change now causes a HISTORY_PUSH instead of a merge.

Instead, we might be able to just discard attempted merges from different editors.

@vercel
Copy link

vercel bot commented Aug 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Aug 23, 2022 at 9:10PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Aug 23, 2022 at 9:10PM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2022
if (isSameEditor) {
return HISTORY_MERGE;
}
return DISCARD_HISTORY_CANDIDATE;
Copy link
Contributor

@fantactuka fantactuka Aug 23, 2022

Choose a reason for hiding this comment

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

Should it discard only if it's a new editor state (prevEditorState.isEmpty()) and otherwise push?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isEmpty would actually return false in this case, since the rich text plugin is putting a ParagraphNode under the RootNode, which makes the _nodeMap size 2, if i'm not mistaken.

This should only impact entries that were marked for merging in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's fine, my concern is discarding is completely ignores update, while history-merge assumed that it'll remain in history stack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merge actually doesn't put it on the undo stack, right? Instead, you just roll forward to the next state and an undo would restore the previous state the change was merged into. Now, we're saying "if there's an update from a different editor that you were planning to merge, ignore it instead"

Another option might be to add a different tag?

Copy link
Member

Choose a reason for hiding this comment

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

In this case shouldn't it be HISTORY_PUSH, or am I misreading this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case shouldn't it be HISTORY_PUSH, or am I misreading this?

Well, the trade off with turning a merge into a push is that, in the case that prompted this change, it will cause state created by the editor.update in the initializeEditorState of the nested composer to be on the stack as well. This should ideally be ignored, because otherwise the user has to press undo twice to remove the inserted image. It doesn't make sense that they would need to hit undo once for the initial state of the caption, then again to undo the image insertion.

However, it still works, and maybe it's the more correct solution if we can tolerate multiple "undos" to undo what should be perceived as a single merged change.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, it still works, and maybe it's the more correct solution if we can tolerate multiple "undos" to undo what should be perceived as a single merged change.

I'd be fine with it compared to a potential state loss in history stack

Copy link
Member

@zurfyx zurfyx left a comment

Choose a reason for hiding this comment

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

The fix itself LGTM! Seems like it was the original intention by looking at the PR diff but that the original author didn't get quite right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants