feat(commands): better handling of buffer-close #1397
Conversation
|
(fixed some typos in the commit message by amending) |
|
\cc @cole-h |
There was a problem hiding this comment.
I feel like it may make sense to turn the last_accessed_doc into a tree of last accessed documents. That might help avoid the issue of switching to the last accessed document but not updating that field (so it's pointing to the same document we just switched to). Then, just remove the document from the tree and let it rebalance itself to have a new "last accessed document". Thoughts? (This probably isn't appropriate for this PR, however, but would make this buffer close stuff more robust.)
|
@cole-h I might be missing something very elemental here, but why is tree needed and not vector for the last accessed docs? |
|
Ordering is something I had in mind. It would be nice to be able to move forward and backward in the "history" of recent docs, but maybe that's complicating things too much. A |
|
@cole-h thanks for suggestions. I refactored the PR. I am still hesitant if this is the correct approach. I was toying around with the result and encountered a few issues (that I tried to fix) with for example document in the history being closed. I am not sure what the right approach here would be. Ideally we might also store the document path in the history and if view tries to access closed document we re-open it? |
|
@archseer when you have some time left, would you please take a look at this PR? 🙏 most importantly the above comment |
That's a bug, such documents need to be removed here: Lines 62 to 64 in 2a1fa0f This is how we clean up the jumplist when a document is closed. |
| pub fn add_to_history(&mut self, id: DocumentId) { | ||
| if let Some(pos) = self.docs_access_history.iter().position(|&doc| doc == id) { | ||
| self.docs_access_history.remove(pos); | ||
| } | ||
| self.docs_access_history.push(id); | ||
| } |
There was a problem hiding this comment.
This seems like it could continually grow and leak memory. It would be good to truncate to a certain max length.
There was a problem hiding this comment.
The if above prevents a single document from being multiple times in history, thus the size should always be <= number of documents and those are un-capped too as far as I am aware, I don't think the limit is strictly necessary here, but I can add it for sure.
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
|
@archseer sorry for long period of nothing, I had a lot of work recently. I got back to this PR and tried to fix it based on suggestions. Would you please take a look one more time? |
|
I think this also closes #2034 |
|
This may close #2107 as well |
|
Just gave this a try and I can't reproduce the jump-backwards panic (#2107) with it 🎉 |
|
@matoous No problem, welcome back! 🎉 |
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