Keep ViewPosition consistent when a Document is edited from different Views, and on buffer switching #10559
Conversation
|
Can we get a retry on this test failure? It looks unrelated to my changes, and I can't reproduce it. Ran the same commit through CI on my fork and got a pass |
|
Something must have changed on the Github Actions runner, I'm seeing the same failure in other PRs. |
|
Looks like macos runners switched from x86 to arm. Clearing the tree-stter-grammars cache for macos seems to have fixed it |
intarga
force-pushed
the
restore_view_offset
branch
from
23ee43a
to
83e3ce0
Compare
|
this lgtm now and was a smaller change than I expected. I will test this out locally a bit |
| for view_data in self.view_data.values_mut() { | ||
| view_data.view_position.anchor = transaction | ||
| .changes() | ||
| .map_pos(view_data.view_position.anchor, Assoc::Before); |
There was a problem hiding this comment.
I do wonder if we should be using Assoc::After here instead. I don't have any opinion here. I will try with before and see if it annoys me. It sprobably doesn't make too much difference due to ensure_cursor_in_view keeping the view position somewhat fixed in extreme cases. In thsose cases it probably makes more sense to keep the cursor at the top so before is probably right
|
@pascalkuthe Did you get a chance to try this? |
|
@the-mikedavis Is there anything we can do to move this forward? It's been sitting here a long time now |
|
I've been running this for a while (I think Pascal has too although he's quite busy lately). It's hard to tell whether it should be After a rebase or master merge I think this is good to go. WDYT @pascalkuthe? |
since we sorted out the borrow checker issues using partial borrows, there's nothing stopping us from going back to the simpler implementation
|
from my side this is good to go. I think the |
intarga
force-pushed
the
restore_view_offset
branch
from
83e3ce0
to
b0754ed
Compare
|
@the-mikedavis The branch is up to date now |
|
I thought I noticed an undesirable change with the jumplist or LSP goto-definition but I tried a bunch of different workflows with this branch vs master and couldn't tell the difference, so I must've been mistaken |
| doc.ensure_view_init(view.id); | ||
| view.sync_changes(doc); | ||
| doc.mark_as_focused(); | ||
|
|
||
| align_view(doc, view, Align::Center); | ||
| view.ensure_cursor_in_view(doc, scrolloff) |
There was a problem hiding this comment.
I think this change may be where Mikes comments are coming from. However, it seems that we are manually centering the view for LSP jumps. I do definitely perefr centering the view when jumping with gd. If we notice commands that relied on the editor implicitly centering here we can add that back to the command definition. I think if we want to change the view position we should do it explicitly instead of implicitly.
|
Thanks for the awesome effort all, not to mention your patience/persistence @intarga! =D |
* replicate t-monaghan's changes * remove View.offset in favour of Document.view_data.view_position * improve access patterns for Document.view_data * better borrow checker wrangling with doc_mut!() * reintroduce ensure_cursor_in_view in handle_config_events since we sorted out the borrow checker issues using partial borrows, there's nothing stopping us from going back to the simpler implementation * introduce helper functions on Document .view_offset, set_view_offset * fix rebase breakage
* replicate t-monaghan's changes * remove View.offset in favour of Document.view_data.view_position * improve access patterns for Document.view_data * better borrow checker wrangling with doc_mut!() * reintroduce ensure_cursor_in_view in handle_config_events since we sorted out the borrow checker issues using partial borrows, there's nothing stopping us from going back to the simpler implementation * introduce helper functions on Document .view_offset, set_view_offset * fix rebase breakage
* replicate t-monaghan's changes * remove View.offset in favour of Document.view_data.view_position * improve access patterns for Document.view_data * better borrow checker wrangling with doc_mut!() * reintroduce ensure_cursor_in_view in handle_config_events since we sorted out the borrow checker issues using partial borrows, there's nothing stopping us from going back to the simpler implementation * introduce helper functions on Document .view_offset, set_view_offset * fix rebase breakage
* replicate t-monaghan's changes * remove View.offset in favour of Document.view_data.view_position * improve access patterns for Document.view_data * better borrow checker wrangling with doc_mut!() * reintroduce ensure_cursor_in_view in handle_config_events since we sorted out the borrow checker issues using partial borrows, there's nothing stopping us from going back to the simpler implementation * introduce helper functions on Document .view_offset, set_view_offset * fix rebase breakage
Supersedes #7568, fixes #7355.
Since view offsets can no longer be initialised in
View::new(), I moved them next to selection initialisation indoc.ensure_view_init(view_id). I was a little concerned about all those unwraps ondoc.view_data(view_id)(those functions are infallible and don't have a mutable reference to doc, so there's no way to ensure view_data is initialised in there), but in theory it should be panic-safe, as long asdoc.ensure_view_init(view_id)is called for every new view, which it is. It was missing from some test cases, since they didn't seem to need selections, but it's added even to those now.