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
Restore selections on undo/redo #1006
base: master
Are you sure you want to change the base?
Conversation
d6aead9
to
0c6ba66
Compare
Sounds good to me! Probably actually even cleaner than having a hard coded map of which edits types have which drifts, but also maybe worth documenting at some point how each of the edit types work, since otherwise it could be confusing about what exactly is the difference between them? Unless somebody reads through the code closely. |
Yea, The |
With this patch, we now keep track of selection state as we create and modify undo groups, which lets us restore selections to their state immediately before or after a given edit (edit group) during undo/redo. I had trouble figuring out exactly what state we wanted to store and how we wanted to represent that. The first version of this just had a single state per undo group; we would always save the last selection state for a given undo group, updating it on movement events. This didn't work. The problem, I think, is that we have different expectations for undo versus redo. As an example, let's say we've just opened a file with the contents, 'hello'. In our view, this looks like, `hello|` where `|` is the cursor. We enter a space and 'world!'; our view looks like `hello world!|`. We use the arrows to select 'world!' (our view looks like `hello [|world!]`) and type 'dear friend'. We now have two undo groups, corresponding to `hello world!` and `hello dear friend`. (We don't include the state of the document when it was opened, which can't be undone.) If we undo, at this stage, we want everything to go back to the state it was in before the last edit; i.e., `hello [|world!]`. If we undo _again_, we should have `hello|`, our starting state. However, what should now happen on _redo_? If we store a single state per edit, we would end up being back at `hello [|world!]`, which I don't think is right; we want `hello world!|`, the state immediately after the 'world!' edit. This means that (at least in some cases) we need to store two separate states; the state to restore on undo (which is the state immediately before the new edit group was crated) and also the state to restore on redo, which is the state immediately after the last edit in the edit group. closes xi-editor#458, closes xi-editor#765 related xi-editor#326, xi-editor#808
0c6ba66
to
56750fd
Compare
This is currently blocked on some correctness issues when handling async edits to an existing undo group; shouldn't be impossible to get right but my focus has been elsewhere. |
With this patch, we now keep track of selection state as we create and
modify undo groups, which lets us restore selections to their state
immediately before or after a given edit (edit group) during undo/redo.
closes #458, closes #765
related #326, #808
possible
Selection
optimizationsThis involves cloning and stashing selections. This feels kind of gross. A few optimizations occurred to me while doing this work. The simplest is that
Selection
should store it's state in anenum
, like:which would mean we only need to allocate in the relatively uncommon multi-selection case.
With or without that change, we could also use an
Arc<Vec<SelRegion>>
, which would at least be more space efficient when we know that every selection will end up cloned at least once.Implementation notes:
I had trouble figuring out exactly what state we wanted to store and how we wanted to represent that. The first version of this just had a single state per undo group; we would always save the last selection state for a given undo group, updating it on movement events.
This didn't work. The problem, I think, is that we have different expectations for undo versus redo. As an example, let's say we've just opened a file with the contents, 'hello'. In our view, this looks like,
hello|
where|
is the cursor. We enter a space and 'world!'; our view looks likehello world!|
. We use the arrows to select 'world!' (our view looks likehello [|world!]
) and type 'dear friend'. We now have two undo groups, corresponding tohello world!
andhello dear friend
. (We don't include the state of the document when it was opened, which can't be undone.)If we undo, at this stage, we want everything to go back to the state it was in before the last edit; i.e.,
hello [|world!]
. If we undo again, we should havehello|
, our starting state. However, what should now happen on redo? If we store a single state per edit, we would end up being back athello [|world!]
, which I don't think is right; we wanthello world!|
, the state immediately after the 'world!' edit.This means that (at least in some cases) we need to store two separate states; the state to restore on undo (which is the state immediately before the new edit group was created) and also the state to restore on redo, which is the state immediately after the last edit in the edit group.
This is a quick stab at #458; at the very least it needs some cleanup/docs/clone-removal. Mostly wanted to open an early PR so other people could play around with it. To whit:@nangtrongvuon I'm curious if this solves the last problems with #849?@lord So for this to work I basically have to thread theEditType
through to the view. Maybe this means we could just useEditType
instead ofInsertDrift
? In any case i have some larger thoughts about the fundamental problem with trying to get "selection adjustment after edit" right; will try to write that up soon.Review Checklist
cargo test --all
/./rust/run_all_checks
.