fix #6087

[gibbz00]

Explained there: #6087

[dead10ck]

Would it be simpler just to clear the object selections any time we switch the view to a different document?

[gibbz00]

Wouldn't that be a regression? The entire feature is just thrown away because of a simple buffer switch. And it's not like this is very complicated right?

[dead10ck]

What do you mean thrown away? If I recall correctly, the original use case was to allow successive expand / shrink calls to remember the whole inner selection, rather than just the first child.

By tracking the selection history per document like this, it would be enriching in that you could switch documents and still retain the previous selections for a document you switch back to. But the use case would be something like:

1. Expand n times
2. Switch documents, do other things
3. Go back to the first document
4. Shrink back to smaller whole subtree selections by shrinking n - m times

Is this a common use case for you?

[gibbz00]

I really don't understand what you're trying say. Is there anything wrong with this PR? Why should the shrink to previous textobject selection stop working if an expand_selection is done on another document?

[pascalkuthe]

I think you identified a larger problem with the object_selection than what was fixed here. It's not just that one document can contaminate another documents object_selections it's also that object_selections are not updated on edits so if you used Alt-i somewhereits possible to randomly get a completly nonsensical result. In general all of these bugs are just a symtom of a much larger problem the object_selections sticks around forever so if you are inside a a rust function in an impl block and do alt-o once to select the current word (or call) and then use mat to select the entire implt block then doing alt-i puts you right back at the word (rather then the first TS child). This was actually raised in the original PR: #1340 (comment). Even weirder the history is never clear so you get weird behaviour like the following:

fn foo(){
   let bar = 1;
   let foo = 2;
}

if you expand the selection to the foo statement first, the do something else, comeback and expand the selection on the bar statement and then select the entire function and shrink_selection the cursor will select the bar statement. If you repeat the expierent and the foo statement second.

I am not entirely sure what the best way to solve this should be but we clearly need to clear the object_selections history much more agressively. The easiest solution would be to just clear the history if:

• the selection changes (and it's not caused by expand_selection or shrink_selection)
• the document changes
• the visible document changes

Alternatively we could treat object_selections closer to a real selection and store it on the document (and therefore loose the third rule). In theory we could even map object_selections trough edits (like we do normal selections) and therefore also loose the third rule, I added a comment with moreA details.

The problem is that this regresses cases where we might select something else go back with c-o and then shrink the selection again. We might just want to store object_selections on the jumplist to account for those cases.

Btw. I also think that shrink_selection needs a ton of improvements in case there isn't a history. It will often select the top of the file in rust for example. I think that shrink_selection should only ever decrease the size of the selection and never select anything outside of it. We probably need to look for the first child that is fully contained in the selection instead of just using the first child. That should be a separate PR tough

[pascalkuthe]

Storing per document data on the view is not a good idea. If the document is closed that data is essentially leaked and it also preculdes you from updating data on edits. If we are going to be storing this per-view and per-document then this should be stored on the Document as HashMap<ViewId,..> just like selections are and inserted cleared in remove_view and ensure_view_init

[dead10ck]

I am not entirely sure what the best way to solve this should be but we clearly need to clear the object_selections history much more agressively.

Yeah, I'm actually working on a feature right now that I started a while ago, but I'm picking back up, and this feature also involves changes to the object selections. I came to the same discovery about how messed up the object selections can become through writing integration tests for it. I decided to solve it by clearing all object selection history any time a selection is set on a view. It's aggressive, but I think it fixes the problems.

It does regress the feature, but I think the only way to solve the problems and retain history through more operations than exclusively expand_selection is by making it significantly more complicated, i.e. tracking all text mutations in them, etc., like you mentioned.