-
Notifications
You must be signed in to change notification settings - Fork 133
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
Export Undo/Redo functions #100
base: main
Are you sure you want to change the base?
Conversation
Export Undo/Redo functions Editor.History is available to save/restore modification history Fixes: https://todo.sr.ht/~eliasnaur/gio/438 Signed-off-by: Fabien Jansem fabien@jansem.eu.org
Export Undo/Redo functions Editor.History is available to save/restore modification history Fixes: https://todo.sr.ht/~eliasnaur/gio/438 Signed-off-by: Fabien Jansem fabien@jansem.eu.org
Please keep one PR per change. It's hard to follow the review history across multiple PRs. You can either squash and force-push or add fixup commits on top of older commits and squash when we're ready to submit your change to main. |
// nextIdx is the index within the history data of the next modification. | ||
// This is only not len(data) immediately after undo operations occur. | ||
// It is framed as the "next" value to make the zero value consistent. | ||
nextIdx int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still stuck with nextIdx
in your proposed change. Why can't modification
be self-contained? I imagine calling it Edit
and defined something like:
type Edit struct {
// rng is the range to be replaced with text.
rng key.Range
text string
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, but I don't understand your request.
nextIdx is the nextHistoryIdx that existed before my PR. I simply moved it to the History structure, which allows it to be saved along with the modifications (I changed the name because History was becoming redundant).
I don't see how it can be deleted without losing the boundary between undo and redo when undo was the last action performed by the user in the editor. If you don't save it, then when you restore modifications editor will consider that the redo's are undo's, which doesn't correspond to what's in the text. (that's my understanding from the existing comment associated with nextHistoryIdx which I also moved into the history).
Maybe I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eliasnaur we need a way to track the boundary between changes that have been applied and changes that can be applied. If you undo three changes, the final three modifications are what should happen when you "redo" three times. That's what the index is for. We could eliminate it by creating two slices (one for undo and one for redo), but we'd be copying elements between them pretty often. Other than that, I don't know how to efficiently eliminate the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we may have different views of what makes History useful. I've posted on the issue tracker to clarify my understanding of your proposal.
0d543a0
to
1686874
Compare
67c77c9
to
46cc311
Compare
f8029f2
to
026d3f9
Compare
3d36537
to
74ccc9c
Compare
Export Undo/Redo functions
Editor.History is available to save/restore modification history
Fixes: https://todo.sr.ht/~eliasnaur/gio/438
Signed-off-by: Fabien Jansem fabien@jansem.eu.org