expand_selection_around

[dead10ck]

This adds a new feature: expand_selection_around. With this command, one starts with an initial arbitrary selection, and then the expand_selection_around acts just like expand_selection, except that it splits the new expanded selection around the initial selection. All the while, it remembers what the initial selection was so that you can continue to expand any number of times around this initial selection. Additionally, shrinking works exactly the same way, except that once you get back down to the first expansion, the last shrink in the stack will revert back to your initial selection.

Implementation

The existing expand_selection and shrink_selection commands are implemented by keeping a list of Selections, which they push and pop off of like a stack.

In order to implement this feature, these "object selections" are generalized into a register of namespaced lists of Selections. Currently these are keyed by hardcoded static strings; this felt cleaner than having special struct fields. Theoretically, other commands that could make use of tracking lists of Selections could use this register in the future too, without specialized members.

Anyway, this particular feature is implemented by keeping 3 distinct Selection histories:

• expand: this serves the role of the existing object selections: it remembers your final selections at each step of the way, so that you can shrink back down to your previous selections
• expand_around_base: this remembers your initial "base" selection before running the expand_selection_around command
• parents: this tracks the entire span of the parent nodes as you are traversing up the syntax tree

The feature is accomplished by essentially pretending internally that we only have one selection: the entire span of a parent node. Each time we expand_selection_around again, this parent node is used to keep traveling up the tree. But before we actually set the final resulting selection, we take the expand_around_base selection and split the parent span into two around the base.

For this, it was necessary to add Selection:: without, which computes the set difference of two Selections.

Incidentally, along the way, I discovered the same bug reported in #6092, so this includes the same fix as, and could possibly supersede, #6093.

Additionally, I also noticed all the bugs that @pascalkuthe mentioned in #6088 (review) and #6087, i.e. the current object selections don't track all the context that they should: they can end up getting used on the wrong document, view, and could end up getting totally out of sync with any text edits that happen between an expansion and a shrink.

As @pascalkuthe mentioned, fixing this in the general case would be somewhat complex, as it would need to track any text edits and possibly associate views with selection histories, as mentioned in #6088 (comment). How this would ultimately end up interacting with the changes in this PR are unclear.

But in any case, I addressed these issues in a simple, but very aggressive way: the object selections are now cleared any time a view's selection is set, except during these expand and shrink commands. This required a breaking change in Document::set_selection: it has to take a mutable reference to the entire View now in order to clear the object selections. This is a simple way to ensure that the object selections don't get out of sync with view switches or document edits, since it's likely that every command that could cause a problematic divergence in the object selections also itself sets a selection, plus many that wouldn't. This includes normal mode movements of any kind, for example.

Consequences of this include:

• One can no longer do an expand, then move around, and then a shrink, and have it still go back to what you had before the expand. One could argue that this is a bug anyway.
• One can no longer do an expand, switch buffers, then switch back to the first buffer, then shrink back to the selection one had before the first expand. One could argue that this is a regression, as in fix #6087 #6088. Personally, my opinion is that this is an ok trade-off, since the object selection history was originally intended to allow for consecutive invocations of expansions and shrinks, which is preserved, though others might disagree.
• It makes the diffs quite noisy. For this reason, I very intentionally laid out the commits neatly so that this can easily be reviewed commit-by-commit.

Also, this PR changes the behavior of shrink_selection when there was no previous expansion. Currently, the first child is selected. However, this is often not very useful at all, since the first child often something like an open brace or parentheses. Instead, the behavior is changed to select the first child that is strictly contained within the span of the current node (credit and thanks to @pascalkuthe again for this idea!). The command is much more useful this way.

@matoous may be interested in this PR as it builds upon their original implementation of the expand and shrink commands.

Based on top of #6156 since it makes use of some of the integration testing improvements
Fixes #6092
Supersedes #6093
Fixes #6087

[pascalkuthe]

Loving the new/improved motions very useful 👍

About the object selection: I think you need to discard that when any transaction is applied too (since apply_impl does not call set_selection AFAIK). Changing all calls to apply to also take a view might be a really invasive change. I have been sort of thinking for a while now that anything view specific that needs to be updated/interact with the document should be stored on the document in a HashMap<ViewId, T>. We might even consider introducing a new ViewData strucut (where selection would just be one field) so we don't endup with 20 HashMaps in Document. This would also allow object_selection to persist across view changes. I agree with you that it's not worth putting in a ton of effort to enable it since its a nieche usecase but since we actually might endup with less changes by implementing that I think it would be worth considering.

Thus change would also be interesting for virtual text/folds since that is yet anthor instance of view & document specific data that needs to be updated in apply_impl (so again could be stored on a generic ViewData struct).

Furthermkre for conceal/fold in particular it would actually be really neat if we centralized all selection changes into a single function so we can hook into it more easily. Perhaps that could be done here since you essentially need the same thing: clear all object_selections whenever the selection changes in any way (with a few exceptions)

[pascalkuthe]

I marked this as waiting on PR for now until #6156 is merged

[dead10ck]

@pascalkuthe

About the object selection: I think you need to discard that when any transaction is applied too (since apply_impl does not call set_selection AFAIK). Changing all calls to apply to also take a view might be a really invasive change.

Yeah, this is true, but because of the last sentence, I figured it would be better to make a concrete improvement to the situation to start with before tackling it in the general case, since that's a much larger change.

We might even consider introducing a new ViewData strucut (where selection would just be one field) so we don't endup with 20 HashMaps in Document.

This is a really interesting suggestion. Let me look through the code again and try to conceptualize this more concretely.

[dead10ck]

I have been sort of thinking for a while now that anything view specific that needs to be updated/interact with the document should be stored on the document in a HashMap<ViewId, T>. We might even consider introducing a new ViewData strucut (where selection would just be one field) so we don't endup with 20 HashMaps in Document. This would also allow object_selection to persist across view changes. I agree with you that it's not worth putting in a ton of effort to enable it since its a nieche usecase but since we actually might endup with less changes by implementing that I think it would be worth considering.

Yeah, after looking through it again, I think you are right; this would actually be less invasive, since we wouldn't have to take a whole View in the set_selection to be able to clear the previous selections. And I believe it would also allow retaining the selection history across document switches (I wasn't able to find anywhere in the stack where set_selection is called. And it would easily allow us to clear all object selections whenever we apply transactions to the text of a Document.

I'll try out this change.

[dead10ck]

Mentioned in chat, but I implemented the above suggestion, and the diff is a lot cleaner, and it covers more corner cases to boot 🎉

[dead10ck]

Ok, I've rebased this. Additionally, as was discussed in chat, I changed the behavior of shrink_selection slightly from the original change in this PR. As before, if there was previously an expand_selection, running shrink_selection simply goes back to what the previous selection was.

In the case where there is no previous selection history from doing an expand_selection, the behavior is now changed so that it will:

• Find the descendant of the range
• Recursively descend the syntax tree in a breadth-first traversal
• We search for the first child node found in this walk which is contained within the original range, where at least one of the ends of the original range moves inside.

The only thing I'd like to do is write some unit tests for the recursive walker. Integration tests pass, but focused unit tests would be better.