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

Closing a second buffer makes the cursor jump to the beginning of the first buffer #1186

Closed
JacobKochems opened this issue Nov 29, 2021 · 15 comments · Fixed by #1397
Closed
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug

Comments

@JacobKochems
Copy link

JacobKochems commented Nov 29, 2021

Reproduction steps

  1. open a file in hx
  2. navigate some lines down
  3. from within hx open a second file into the next buffer
    (switching back and forth with gn centers cursor in view)
  4. close second buffer
  5. cursor is back at the beginning of the first file
    (expected: cursor stays at downward position)

Environment

  • Platform: Linux (Debian 11)
  • Helix version: v0.5.0-165-g0dd2303
~/.cache/helix/helix.log
2021-11-29T17:16:41.041 helix_view::editor [ERROR] Failed to initialize the LSP for `source.rust` { IO Error: No such file or directory (os error 2) }
2021-11-29T17:16:48.097 helix_view::editor [ERROR] Failed to initialize the LSP for `source.rust` { IO Error: No such file or directory (os error 2) }
@JacobKochems JacobKochems added the C-bug Category: This is a bug label Nov 29, 2021
@sudormrfbin
Copy link
Member

/cc @cole-h

@cole-h
Copy link
Contributor

cole-h commented Nov 29, 2021

A not-so-short background on this feature, first:

Closing a document with :bc closes any views it's focused in. Originally, I had some "smart" switching ability, where instead of closing the view, the view would just switch its currently-focused document to something else (either the last accessed one, or the one that comes before it in the tree if there is no last accessed doc, or the one that comes after it in the tree if there are none that come before it, or a new document). However, somewhere along the line, we decided this wasn't what we wanted to do, and instead I went the route of always closing all views that have this document focused. The reason why closing views is a problem for this situation is because that's where the jumps (a tuple of (DocumentId, Selection), which holds where the cursor may be) are stored -- meaning closing the view loses all previously-held selections for that document. Now, the document itself has a HashMap of selections (mapping a ViewId to a Selection), but again: the view is closed, so its ViewId is no longer accessible / valid.

So basically, the problem can be traced back to the fact that we indiscriminately close views instead of doing some "smarts" in order to preserve the views.

I'll look into improving this.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Nov 30, 2021
@dead10ck
Copy link
Member

I think the issue described here is saying that views other than the one that was closed are moving to the top of their respective documents. In other words, if you have documents A, B, and C open, and you close C, both A and B scroll up to the top of the file for some reason.

@JacobKochems
Copy link
Author

JacobKochems commented Nov 30, 2021

I think the issue described here is saying that views other than the one that was closed are moving to the top of their respective documents. In other words, if you have documents A, B, and C open, and you close C, both A and B scroll up to the top of the file for some reason.

Yes, that is exactly the behavior I'm observing. Maybe I didn't described that clearly enough.

The behavior of centering the cursor in the view when switching back and forth between buffers is also somewhat unexpected but doesn't affect usability too much. Could this be a related issue?

@cole-h
Copy link
Contributor

cole-h commented Nov 30, 2021

That's exactly what I'm saying though (maybe it was me who wasn't clear; sorry) -- the current cursor position in a document is tied to the View itself by an ID, but we unconditionally close all views that have the document-to-be-closed focused. Then, we create a new view with the default Selection because the document's cursor location (which is a Selection) is tied to the View's ID, which is gone because we just closed that view...... It's not that we don't restore the cursor, it's that we can't (with the current implementation).

@dead10ck
Copy link
Member

Sorry, maybe I don't have a good understanding of the terminology. A "view" is just a window into a document right? So if you have a split buffer on the same document, that's two views right?

What we're saying is that the cursor is getting reset in a different document than the one that was just closed.

@cole-h
Copy link
Contributor

cole-h commented Nov 30, 2021

Just to clarify, then: do you mean that if you open documents A, B, and C in one single split (e.g. no C-w C-s and friends), closing e.g. C would reset the cursors in A and B? Or do you mean that opening documents A, B, and C in separate splits (e.g. with C-w C-s and friends) resets the cursors in A and B after closing C?

@dead10ck
Copy link
Member

The former. If you open documents A, B, and C in separate single split views, move the cursor down in A and B, then close C, the cursors in A and B get reset.

@cole-h
Copy link
Contributor

cole-h commented Nov 30, 2021

Right, OK. I'll do my best to explain in detail why this is happening (but basically know that: this is expected behavior with the current implementation).


(TL;DR: This is expected and we are indeed talking about the same thing.)

Every Document has a HashMap<ViewId, Selection> where it stores its selections, which as you might guess per-view (due to being keyed by the ViewId). The current implementation of :bc is to close all views with the current document focused:

let views_to_close = self
.tree
.views()
.filter_map(|(view, _focus)| {
if view.doc == doc_id {
Some(view.id)
} else {
None
}
})
.collect::<Vec<_>>();
for view_id in views_to_close {
self.close(view_id);
}

This means that if you have a helix instance that looks like:

,---------------.
|       A       | <-- ViewId(1)
|               |
|---------------|
|      *B*      | <-- ViewId(2)
|               |
|---------------|
|      *B*      | <-- ViewId(3)
|               |
`---------------'

(where *B* is the currently focused document)

If you were to execute :bc, ViewId(2) and ViewId(3) would be completely destroyed. Document "A" keeps its cursor position because its view was not closed.

Now, if you had a helix instance that looks like the following (assuming "A" was opened in the top split before switching to "B", and then helix was split a couple times):

,---------------.
|      *B*      | <-- ViewId(1)
|               | (document A was visible before document B was opened in this split)
|---------------|
|      *B*      | <-- ViewId(2)
|               |
|---------------|
|      *B*      | <-- ViewId(3)
|               |
`---------------'

and then ran :bc, all three of those Views would be deleted. Document "A" would have its cursor position reset to the beginning of the document. helix would then look like this:

,---------------.
|      *A*      | <-- ViewId(4)
|               |
`---------------'

This is the crux of the issue. Notice how document "A" is now a part of ViewId(4). It was previously in ViewId(1), but when document "B" was closed via :bc, all of the views it was focused in were also closed: the views indexed by ViewId(1), ViewId(2), and ViewId(3) have been removed.

Does that help explain a little bit?

@dead10ck
Copy link
Member

dead10ck commented Nov 30, 2021

We are not talking about a scenario where we open A, B, and C, and then change A's view to look at C. The scenario that's happening now is, say we open 3 files with hx A B C:

,---------------.
|     *A*       | <-- ViewId(1)
|               |
|---------------|
|      B        | <-- ViewId(2)
|               |
|---------------|
|      C        | <-- ViewId(3)
|               |
`---------------'

then scroll down A, switch to B, scroll down, switch focus to C

,---------------.
|       A       | <-- ViewId(1)
|               |
|---------------|
|       B       | <-- ViewId(2)
|               |
|---------------|
|      *C*      | <-- ViewId(3)
|               |
`---------------'

And then :bc. After this, views 1 and 2 have their cursors reset to the beginning.

Edit: unless I'm misunderstanding how these transitions happen. If you are in view 1 and then do a switch buffer to B, does it change to view 2, or does it swap the documents of view 1 and 2? And if you open 3 files like that, does it make 3 views, or just 1 view and keep the other files open in the background, and switching buffers just reuses view 1?

@JacobKochems
Copy link
Author

@dead10ck wrote:

Edit: unless I'm misunderstanding how these transitions happen. If you are in view 1 and then do a switch buffer to B, does it change to view 2, or does it swap the documents of view 1 and 2?

How I understood @cole-h's explanation it is the later.

@cole-h wrote:

Does that help explain a little bit?

I think so. But that leads me to another question:

Suppose we do: hx A and make a particular selection, say: vww.
Now we are in the state:

,-------.
|  *A*  | <-- ViewId(1)
`-------'

than we do: :open B which leads to:

,-------.
|  *B*  | <-- ViewId(1)
`-------'

than we do: gn or gp we have again:

,-------.
|  *A*  | <-- ViewId(1), including the previous selection
`-------'

How is the selection in A restored when the selection is tied to the ViewID and A and B share the same ViewID?

@cole-h
Copy link
Contributor

cole-h commented Nov 30, 2021

If you are looking at a single document A and open a new document B, document B replaces document A in that view (so, the previously-visible document A is now stored somewhere else). The view stays the same, but now document B is visible, and document A is "hidden".

Could you make make an asciinema or something? I can't reproduce this. I opened helix and then ran the following:

  1. :o A (and added a few lines to move the cursor around)
  2. C-w C-s
  3. :o B (and added a few lines to move the cursor around)
  4. C-w C-s
  5. :o C
  6. :bc (closes C)

and the cursors in A and B stayed the same.


How is the selection in A restored when the selection is tied to the ViewID and A and B share the same ViewID?

Each document has its own list of selections tied to the ViewId. Since the ViewId never actually changes in your scenario, there are no problems with restoring the selection. So, A stores its ViewId-Selection map in something that looks like this: { ViewId(1): SomeSelectionInA }, and B's looks like { ViewId(1): SomeSelectionInB }. When document B is closed, or you go back to document A with gn / gp, helix looks up the selections that match the view in this newly-visible document. So, helix finds "oh, document A has a selection of SomeSelectionInA for ViewId(1) -- great, let's do that".


Maybe something I didn't make clear is: if a document is visible in all views, then all views will be closed, and a new view will be created. This means this new view has a new ID, and as such, helix won't find any matching selections. ViewId(x+1) has never seen document N before, even though ViewId(x) had a valid selection.

@JacobKochems
Copy link
Author

Could you make make an asciinema or something?

asciicast

@cole-h
Copy link
Contributor

cole-h commented Nov 30, 2021

Yeah, OK, that's exactly what I was talking about, thanks. Closing textobject.rs closes the view that is holding textobject.rs as well as auto_pairs.rs. Then, we create a new View (with a new ID) and focus the remaining document, auto_pairs.rs (and the selection doesn't return because the ViewId is different).

@dead10ck
Copy link
Member

Ahhh, I see, so then yeah, doing hx A B C makes just one view, but puts the other files in the background, and switching buffers just reuses the current view and associates that one view with the other file. I take it that this means that the only time you ever have more than one view is when you do splits.

Sorry for the confusion, I'm still learning how everything works. Thanks for taking the time to explain!

the-mikedavis pushed a commit to the-mikedavis/helix that referenced this issue Apr 20, 2022
Previously, when closing buffer, you would loose cursor position in other docs.
Also, all splits where the buffer was open would be closed.

This PR changes the behavior, if the view has also other buffer
previously viewed it switches back to the last one instead of the view
being closed. As a side effect, since the views are persisted,
 the cursor history is persisted as well.

Fixes: helix-editor#1186
archseer pushed a commit that referenced this issue Apr 27, 2022
* feat(commands): better handling of buffer-close

Previously, when closing buffer, you would loose cursor position in other docs.
Also, all splits where the buffer was open would be closed.

This PR changes the behavior, if the view has also other buffer
previously viewed it switches back to the last one instead of the view
being closed. As a side effect, since the views are persisted,
 the cursor history is persisted as well.

Fixes: #1186

* Adjust buffer close behavior

* Remove closed documents from jump history

* Fix after rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants