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

Restore selections on undo/redo #1006

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

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Nov 21, 2018

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 optimizations

This 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 an enum, like:

enum SelectionState {
    Single(SelRegion),
    Multi(Vec<SelRegion>),
}

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 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 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 the EditType through to the view. Maybe this means we could just use EditType instead of InsertDrift? 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

  • I have responded to reviews and made changes where appropriate.
  • I have tested the code with cargo test --all / ./rust/run_all_checks.
  • I have updated comments / documentation related to the changes I made.
  • I have rebased my PR branch onto xi-editor/master.

@lord
Copy link
Member

lord commented Nov 23, 2018

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.

@cmyr
Copy link
Member Author

cmyr commented Nov 23, 2018

Yea, The EditType (and undo grouping in general) has always seemed like a bit of a wart, not sure what the best fixup should be. Documentation certainly helps.

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
@cmyr cmyr changed the title [WIP] Restore selections on undo/redo Restore selections on undo/redo Nov 23, 2018
@cmyr
Copy link
Member Author

cmyr commented Dec 12, 2018

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.

@jansol jansol added the on hold label Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants