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

feat: ignore transactions which were not added to history #4

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phikes
Copy link

@phikes phikes commented Aug 4, 2020

prosemirror-history reads a metadata field addToHistory and doesn't
allow to undo changes if a transactions sets it to false. I think its
reasonable that this plugin ignores these transactions as well as they
are not undoable by the undo command anyways.

`prosemirror-history` reads a metadata field `addToHistory` and doesn't
record changes if a transactions sets it to `false`. I think its
reasonable that this plugin ignores these transactions as well as they
are not undoable by the undo command anyways.
@marijnh
Copy link
Member

marijnh commented Aug 4, 2020

I think its reasonable that this plugin ignores these transactions as well as they are not undoable by the undo command anyways.

Why?

@phikes
Copy link
Author

phikes commented Aug 4, 2020

If your questions targets the reasoning why the plugin should ignore transactions with addToHistory set to false the reasoning is in the second part of the sentence. My scenario is as follows:

  • an input rule transaction happens
  • our state is configured to dispatch other transactions which are "behind the scenes" transactions (in that case they adjust UUIDs) and as such are set to be ignored in the history
  • these transactions overwrite the input rules plugin's state so that the original input rule transactions cannot be undone anymore by the undoInputRule command

@phikes
Copy link
Author

phikes commented Aug 4, 2020

Btw since this is probably a breaking change I could also add a configuration option to inputRules so it's opt-in behaviour:

export function inputRules({rules, onlyTransactionsInHistory = false}) {

@marijnh
Copy link
Member

marijnh commented Aug 4, 2020

Ah, right, you're trying to avoid clearing the undo logic when something like a collaborative change comes in. But as it is, this patch isn't correct—if, for example, a non-history change inserts or deletes content before the last inputrules change, the positions in that stored change no longer align with the current document.

@phikes
Copy link
Author

phikes commented Aug 5, 2020

Thanks for the response! Makes total sense. I am fairly new to Prosemirror and am trying to figure out how to solve the problem you mentioned. As soon as there is a change that affects not only the position of an original input rule change, but also maybe it's contents and that change is not added to history I think we could not recover. As such I'd think of two options:

  • Make the ignoring bit opt-in (see above; in our case we can safely ignore the non-history changes e.g.)
  • Adjust the transaction in the state in the input rules plugin as long as we are only shifting positions and not changing the content it describes (in addition to it having addToHistory !== false)

@marijnh
Copy link
Member

marijnh commented Aug 5, 2020

Look into position mapping—it is probably possible to update the undoable data in a safe way.

@phikes
Copy link
Author

phikes commented Aug 5, 2020

This is what I thought about, however we would still need to discard the transactions as soon as another transaction comes along which modifies inside of the (mapped) range of the original transaction, no?

So in pseudo-code something like this?

stored = tr.getMeta(this)
if (stored) return stored

if (tr didNotChangeAnything in prev's content) { return prevWithMappedPositions }

return null // content changed inside of the original input rule tr or new input rule tr

@marijnh
Copy link
Member

marijnh commented Aug 5, 2020

we would still need to discard the transactions as soon as another transaction comes along which modifies inside of the (mapped) range of the original transaction, no?

This is the same case as step rebasing for history or collab editing. You can map steps through anything. Sometimes the result will be null (the changed content was deleted) and sometimes the step can't be applied anymore (maybeStep will return failure), so this becomes a bit more tricky to manage (probably don't want to store the plain transaction anymore, but rather an array of steps), but should be doable.

@phikes
Copy link
Author

phikes commented Aug 5, 2020

Thanks for the response, really appreciate it. I will see if I can make sense out of it and build it into my patch.

@phikes phikes marked this pull request as draft August 5, 2020 11:22
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