prosemirror/prosemirror-history
1
0
Fork 0
Code Pull requests Activity

Fix redoing after append transaction #3

Closed
RichieAHB wants to merge 1 commit from rahb/fix-append-redos into master
pull from: rahb/fix-append-redos
merge into: prosemirror:master
Conversation 7 Commits 1 Files changed 2 +33 -6
RichieAHB commented 2018-07-12 17:00:58 +02:00 (Migrated from github.com)
Copy link

This is a proposed solution to https://github.com/ProseMirror/prosemirror/issues/819, it also requires https://github.com/ProseMirror/prosemirror-state/pull/13 for it to work (although won't break if this patch isn't present i.e. a user is using a prosemirror-state without prior to that hypothetical version)

N.B. the new test here naturally won't pass without the change to prosemirror-state.

This is a proposed solution to https://github.com/ProseMirror/prosemirror/issues/819, it also requires https://github.com/ProseMirror/prosemirror-state/pull/13 for it to work (although won't break if this patch isn't present i.e. a user is using a `prosemirror-state` without prior to that hypothetical version) N.B. the new test here naturally won't pass without the change to `prosemirror-state`.
marijnh (Migrated from github.com) reviewed 2018-07-19 21:14:39 +02:00
src/history.js
@ -266,2 +266,3 @@
} else if ((appended || tr).getMeta("addToHistory") !== false) {
} else if (tr.getMeta("addToHistory") !== false) {
// Group transforms that occur in quick succession into one event.
let historyRoot = rootTr && !!rootTr.getMeta(historyKey);
marijnh (Migrated from github.com) commented 2018-07-19 21:14:38 +02:00
Copy link

I'd like to keep the behavior where transactions appended to a non-history root transaction aren't added to history either. What is the reason for this change?

I'd like to keep the behavior where transactions appended to a non-history root transaction aren't added to history either. What is the reason for this change?
marijnh commented 2018-07-19 21:20:04 +02:00 (Migrated from github.com)
Copy link

I do see the problem here, but I'm not sure yet what the right solution is. I suspect this patch makes it possible for the history to become corrupted (you preserve the old undone stack, but that is no longer aligned with the current document, since the transaction changed that). I'll think about it more when I'm back from my holiday.

I do see the problem here, but I'm not sure yet what the right solution is. I suspect this patch makes it possible for the history to become corrupted (you preserve the old `undone` stack, but that is no longer aligned with the current document, since the transaction changed that). I'll think about it more when I'm back from my holiday.
RichieAHB commented 2018-07-19 21:25:26 +02:00 (Migrated from github.com)
Copy link

Yeah I realised this but thought I’d leave the straw man up as I think it
makes the problem a bit clearer than I put it in words. A plugin could do
anything to the state, I guess the lazy assumption by me here is that it
probably shouldn’t, but it actually isn’t a sound assumption. Enjoy your
holiday!
On Thu, 19 Jul 2018 at 20:20, Marijn Haverbeke notifications@github.com
wrote:

I do see the problem here, but I'm not sure yet what the right solution
is. I suspect this patch makes it possible for the history to become
corrupted (you preserve the old undone stack, but that is no longer
aligned with the current document, since the transaction changed that).
I'll think about it more when I'm back from my holiday.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ProseMirror/prosemirror-history/pull/3#issuecomment-406386007,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABk12y_i8WG2CYdd905T9WiocMYgHN0-ks5uINvkgaJpZM4VNGxL
.

Yeah I realised this but thought I’d leave the straw man up as I think it makes the problem a bit clearer than I put it in words. A plugin could do anything to the state, I guess the lazy assumption by me here is that it probably shouldn’t, but it actually isn’t a sound assumption. Enjoy your holiday! On Thu, 19 Jul 2018 at 20:20, Marijn Haverbeke <notifications@github.com> wrote: > I do see the problem here, but I'm not sure yet what the right solution > is. I suspect this patch makes it possible for the history to become > corrupted (you preserve the old undone stack, but that is no longer > aligned with the current document, since the transaction changed that). > I'll think about it more when I'm back from my holiday. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/ProseMirror/prosemirror-history/pull/3#issuecomment-406386007>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABk12y_i8WG2CYdd905T9WiocMYgHN0-ks5uINvkgaJpZM4VNGxL> > . >
RichieAHB (Migrated from github.com) reviewed 2018-07-20 11:04:30 +02:00
src/history.js
@ -266,2 +266,3 @@
} else if ((appended || tr).getMeta("addToHistory") !== false) {
} else if (tr.getMeta("addToHistory") !== false) {
// Group transforms that occur in quick succession into one event.
let historyRoot = rootTr && !!rootTr.getMeta(historyKey);
RichieAHB (Migrated from github.com) commented 2018-07-20 11:04:30 +02:00
Copy link

As far as I can see this is the same thing. A transaction's appendedTransaction can only be itself (if at all). So appended is either falsey or === tr, unless there's somewhere else that appendedTransaction is being set that I've missed?

As far as I can see this is the same thing. A transaction's `appendedTransaction` [can only be itself](https://github.com/ProseMirror/prosemirror-state/blob/545244357dbc12cc1f340cd3f8ad75dc7472a803/src/state.js#L131) (if at all). So `appended` is either falsey [or `=== tr`](https://github.com/RichieAHB/prosemirror-history/blob/22f773640ce79c91f328d14ef20de8361b2fb7f0/src/history.js#L263), unless there's somewhere else that `appendedTransaction` is being set that I've missed?
marijnh (Migrated from github.com) reviewed 2018-07-20 11:27:50 +02:00
src/history.js
@ -266,2 +266,3 @@
} else if ((appended || tr).getMeta("addToHistory") !== false) {
} else if (tr.getMeta("addToHistory") !== false) {
// Group transforms that occur in quick succession into one event.
let historyRoot = rootTr && !!rootTr.getMeta(historyKey);
marijnh (Migrated from github.com) commented 2018-07-20 11:27:50 +02:00
Copy link

A transaction's appendedTransaction can only be itself (if at all).

Right, but that's a bug (the behavior as you fixed it in the other PR was what I was trying to implement there)

> A transaction's appendedTransaction can only be itself (if at all). Right, but that's a bug (the behavior as you fixed it in the other PR was what I was _trying_ to implement there)
RichieAHB commented 2018-07-20 11:29:51 +02:00 (Migrated from github.com)
Copy link

Ah right - I did wonder! Either way it can wait 👍
On Fri, 20 Jul 2018 at 10:27, Marijn Haverbeke notifications@github.com
wrote:

@marijnh commented on this pull request.

In src/history.js
https://github.com/ProseMirror/prosemirror-history/pull/3#discussion_r203987231
:

if (tr.steps.length == 0) {
return history

  • } else if ((appended || tr).getMeta("addToHistory") !== false) {
  • } else if (tr.getMeta("addToHistory") !== false) {

A transaction's appendedTransaction can only be itself (if at all).

Right, but that's a bug (the behavior as you fixed it in the other PR was
what I was trying to implement there)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ProseMirror/prosemirror-history/pull/3#discussion_r203987231,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABk122dnCgbVU9A_n-f53oZXfrn_3JXOks5uIaKXgaJpZM4VNGxL
.

Ah right - I did wonder! Either way it can wait 👍 On Fri, 20 Jul 2018 at 10:27, Marijn Haverbeke <notifications@github.com> wrote: > *@marijnh* commented on this pull request. > ------------------------------ > > In src/history.js > <https://github.com/ProseMirror/prosemirror-history/pull/3#discussion_r203987231> > : > > > if (tr.steps.length == 0) { > return history > - } else if ((appended || tr).getMeta("addToHistory") !== false) { > + } else if (tr.getMeta("addToHistory") !== false) { > > A transaction's appendedTransaction can only be itself (if at all). > > Right, but that's a bug (the behavior as you fixed it in the other PR was > what I was *trying* to implement there) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/ProseMirror/prosemirror-history/pull/3#discussion_r203987231>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABk122dnCgbVU9A_n-f53oZXfrn_3JXOks5uIaKXgaJpZM4VNGxL> > . >
marijnh commented 2018-07-23 17:08:17 +02:00 (Migrated from github.com)
Copy link

Closing in favor of 2a94750

Closing in favor of 2a94750

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
prosemirror/prosemirror-history!3
No description provided.