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

Implement undo/redo #216

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Implement undo/redo #216

wants to merge 30 commits into from

Conversation

gkubisa
Copy link
Contributor

@gkubisa gkubisa commented Jun 21, 2018

  • Undo/redo for locally submitted operations/snapshots. It works with OT types that implement invert or applyAndInvert.
  • submitSnapshot for submitting snapshots, instead of operations. 2 snapshots are compared to generate an operation. It requires diff or diffX to work.
  • Users can choose, if an operation should be undoable or not.
  • No empty undo/redo operations, if the OT type supports isNoop.
  • Customizable undo stack size limit.
  • Undo operations can be composed, if OT type supports compose or composeSimilar.
  • Customizable time limit for composing operations.
  • Fix-up operations which work with undo/redo.
  • skipNoop option for submitOp and submitSnapshot to skip all processing on empty operations. Requires isNoop in the OT type to work.
  • Undo/redo across multiple documents.
  • Multiple undo managers can be created to manage multiple undo/redo stacks.
  • Filtering which operations should be recorded by each undo manager using the source option.

PS. I'll open a separate PR to update https://github.com/ottypes/docs with the new functions.

The problem was that unsubscribe re-added the doc to the connection.
Now the doc is removed from the connection after unsubscribe.
Additionally, we're no longer waiting for the unsubscribe response
before executing the callback. It is consistent with Query,
unsubscribe can't fail anyway and the subscribed state is updated
synchronously on the client side.
@coveralls
Copy link

coveralls commented Jun 21, 2018

Coverage Status

Coverage increased (+0.07%) to 96.563% when pulling a788f1e on Teamwork:undo-and-redo into 762e05d on share:master.

@ericyhwang
Copy link
Contributor

@nateps comments from PR review meeting:

  • Connection level undo stack might be better in some cases, like if you're changing 3 docs at once.
  • Could have an undo stack manager, used by the connection.

@ericyhwang : You might still want to undo for a specific doc locally, instead of across whole connection.
@gkubisa : Undo manager could be provided which docs to track. I'll experiment with making it so the undo/redo doc can either be across all ops from the client or just ones from specific docs.

@gkubisa
Copy link
Contributor Author

gkubisa commented Jul 16, 2018

Re managing undo/redo for groups of documents (for example to support UIs backed by multiple documents), I don't think that this feature should be a part of ShareDB because it is at a higher level of abstraction than the rest of the library - ShareDB deals with docs and queries rather than UIs. Additionally, different projects might have different requirements for group undo and it could be hard to cover all cases.

So, I'd suggest we just extend this PR slightly to make it possible to easily implement group undo/redo in the client code or separate projects. I think that all we need is to:

  • support an additional option called meta for submitOp, submitSnapshot, undo and redo. That option would associate arbitrary metadata with an operation, which would be available only locally.
  • document undoStack and redoStack as public, read-only variables.

I think that would be enough to support any group undo scheme. Below is just the simplest case - undo across 2 docs.

let seq = 0
const doc1 = connection.get('dogs', 'fido')
const doc2 = connection.get('dogs', 'toby')
const docs = [ doc1, doc2 ]

function undo() {
  const doc = docs.reduce((doc, nextDoc) => {
    if (!nextDoc.canUndo()) return doc
    if (!doc) return nextDoc
    if (nextDoc.undoStack[nextDoc.undoStack.length - 1].meta > doc.undoStack[doc.undoStack.length - 1].meta) return nextDoc
    return doc
  }, null)
  if (doc) doc.undo()
}

function redo() {
  const doc = docs.reduce((doc, nextDoc) => {
    if (!nextDoc.canUndo()) return doc
    if (!doc) return nextDoc
    if (nextDoc.redoStack[nextDoc.redoStack.length - 1].meta < doc.redoStack[doc.redoStack.length - 1].meta) return nextDoc
    return doc
  }, null)
  if (doc) doc.redo()
}

doc1.submitOp(op1, { undoable: true, meta: seq++ })
doc2.submitOp(op2, { undoable: true, meta: seq++ })
doc1.submitOp(op3, { undoable: true, meta: seq++ })
doc1.submitOp(op4, { undoable: true, meta: seq++ })

undo(); undo(); undo(); undo();
redo(); redo(); redo(); redo();

It basically associates sequence numbers with operations applied to different docs. To undo the latest op, call undo on the document which has the op with the largest seq number on the undoStack. To redo the latest op, call redo on the document which has the op with the smallest seq number on the redoStack.

This simple example could be extended to support overlapping doc groups, undoing multiple docs at once, dynamically adding and removing docs from groups, etc.

Would that be ok?

@gkubisa
Copy link
Contributor Author

gkubisa commented Jul 16, 2018

I think that the existing source option could be used instead of adding a new meta option.

@gkubisa
Copy link
Contributor Author

gkubisa commented Jul 16, 2018

The op event handler takes 3 params now. Maybe we could refactor it to take a single "event" object instead?

@gkubisa
Copy link
Contributor Author

gkubisa commented Jul 20, 2018

@nateps @ericyhwang I've updated this feature to allow undo/redo across multiple documents. It's also possible to create multiple undo managers to manage multiple undo/redo stacks. The operations recorded by each undo manager can be filtered by the source option of ops. Could you review it, please?

@curran
Copy link
Contributor

curran commented Nov 7, 2018

@gkubisa I'm curious, do you have a working interactive demo of this feature? For example with CTRL+Z mapped to the undo manager? I'd like to try out this feature, but it looks like a lot of work to set up a demo. Just thought I'd ask if there's one already available to start from before I embarked on the work. Thanks!

@gkubisa
Copy link
Contributor Author

gkubisa commented Nov 7, 2018

Sorry, I don't have any open source demo, however, it should be possible to adapt the ShareDB-QuillJS demo reasonably quickly.

Improving user experience by composing similar operations on the undo stack (eg typing consecutive letters of a word) could be a little more tricky but also doable. At the moment you'd do that using the composeSimilar function but I think it's flawed design and I'd like to provide a better way of controlling which operations should be composed on the undo stack in the future.

PS. If you want to just test out ShareDB undo/redo, you could register a free Teamwork Projects account (I work for Teamwork.com), enable collaborative editing in Settings -> Beta Program and try it out on an existing rich-text notebook. Collaborative Editing will be officially released in the next few weeks.

@curran
Copy link
Contributor

curran commented Nov 8, 2018

Great idea to try Teamwork! That's what I'm really interested in, just testing out the interactions and feeling out the behavior. Thanks!

@gkubisa
Copy link
Contributor Author

gkubisa commented Jan 30, 2019

Undo/redo has worked for us well in Teamwork for the last few months, however, I have noticed some problems with it.

It seems that in some cases the user intention might not be preserved correctly when operations are transformed on the undo/redo stacks. For example:

  1. A remote user types in a paragraph.
  2. A local user removes that paragraph.
  3. The changes sync up.
  4. The local user undoes his operation but the remote user's changes are not restored.

I'm not sure yet, if it can be solved by adjusting my type definition, or if the current design of the undo/redo feature is flawed. I'll post a message here once I find the answer, however, other than that I'm not likely to have time to work on this PR. Feel free to make any tweaks and merge it, or just close it.

FYI I have recently started working on SyncOT - a library similar to ShareDB but optimised for a different use case. It's designed with Presence, Undo/Redo and Offline Mode in mind from the start and does away with any cross-document dependencies (queries, multi-document undo/redo, etc).

@curran
Copy link
Contributor

curran commented Jul 22, 2020

Suggest to close as stale.

@curran curran mentioned this pull request Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants