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

Restore view offset when switching buffers with multiple windows #7568

Closed
wants to merge 2 commits into from

Conversation

t-monaghan
Copy link

@t-monaghan t-monaghan commented Jul 8, 2023

This pull request is in response to changes requested by @pascalkuthe in #7414 looking to solve the issue #7355.

The changes made are to ensure views can store and access their own positioning for documents by way of including the field view_data: HashMap<ViewId, ViewData> in Document.

I haven't yet addressed the point of not differentiating between last view position and current view position. I'm unsure how to proceed with this goal.

Fixes #7355

@intarga intarga mentioned this pull request Feb 10, 2024
12 tasks
@the-mikedavis the-mikedavis changed the title View positions correctly restored when switching buffers with multiple windows Restore view offset when switching buffers with multiple windows Feb 10, 2024
@the-mikedavis the-mikedavis added C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 10, 2024
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me. I'll test it for a while to make sure the behavior feels right

Comment on lines +1621 to +1627
pub fn view_data(&mut self, view_id: ViewId) -> &ViewData {
self.view_data.entry(view_id).or_default()
}

pub fn view_data_mut(&mut self, view_id: ViewId) -> &mut ViewData {
self.view_data.entry(view_id).or_default()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these should return Option<&[mut ]Viewdata> instead? That would allow us to drop the Default requirement for ViewData. It's not currently useful to do that between this and #6198 though so I don't have a strong opinion either way

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus still hast Adressen the issue about mapping view position trough changes. If you edit the buffer in another split the view offset would change. So deleting text in another view causes the position to slide up.

This idea was to entirely replace view.offset with the ViewPosition introduced in this PR (since that fixes other oddities that I ran into) but it's a larger change that can be kept separate.

@the-mikedavis
Copy link
Member

For reference see where we update selections when there are changes to a document:

for selection in self.selections.values_mut() {
*selection = selection
.clone()
// Map through changes
.map(transaction.changes())
// Ensure all selections across all views still adhere to invariants.
.ensure_invariants(self.text.slice(..));
}

diagnostics:

// map diagnostics over changes too
changes.update_positions(self.diagnostics.iter_mut().map(|diagnostic| {
let assoc = if diagnostic.starts_at_word {
Assoc::BeforeWord
} else {
Assoc::After
};
(&mut diagnostic.range.start, assoc)
}));
changes.update_positions(self.diagnostics.iter_mut().filter_map(|diagnostic| {
if diagnostic.zero_width {
// for zero width diagnostics treat the diagnostic as a point
// rather than a range
return None;
}
let assoc = if diagnostic.ends_at_word {
Assoc::AfterWord
} else {
Assoc::Before
};
Some((&mut diagnostic.range.end, assoc))
}));
self.diagnostics.retain_mut(|diagnostic| {
if diagnostic.zero_width {
diagnostic.range.end = diagnostic.range.start
} else if diagnostic.range.start >= diagnostic.range.end {
return false;
}
diagnostic.line = self.text.char_to_line(diagnostic.range.start);
true
});

and inlay hints:

// Update the inlay hint annotations' positions, helping ensure they are displayed in the proper place
let apply_inlay_hint_changes = |annotations: &mut Rc<[InlineAnnotation]>| {
if let Some(data) = Rc::get_mut(annotations) {
changes.update_positions(
data.iter_mut()
.map(|annotation| (&mut annotation.char_idx, Assoc::After)),
);
}
};

@intarga
Copy link
Contributor

intarga commented Apr 22, 2024

Since it doesn't seem like @t-monaghan is coming back for this, and I'm blocked on it for #9143, I decided to take a stab at it. I successfully implemented mapping the ViewPositions in Document.view_data through changes, and entirely removed View.offset in favour of Document.view_data.

It works, and solves the issue, but I'm not happy with it. Now every time any function needs to access a ViewPosition, it needs to do a hashmap lookup, which has a performance cost. But worse, I feel that it really clouds the abstraction boundary between View and Document to have ViewPositions natively live in Documents, it means that View methods that change the ViewPosition suddenly need a mutable reference to the doc of that view, and would arguably make more sense as methods on Document...

I'm starting to think that creating ViewData on Document was the wrong start on this problem, and that we'd be better served putting this history of ViewPositions on View instead. That way, the view abstraction doesn't leak onto Document, View can keep a .offset field for the position on the current document, which can be accessed without a hashmap lookup, and we should still be able to map through changes by finding views relevant to a document from Document.selections, and updating the relevant view position history value on them.

@the-mikedavis @pascalkuthe, I would appreciate if you have any guidance/insights to share on this

@pascalkuthe
Copy link
Member

we should still be able to map through changes by finding views relevant to a document from Document.selections, and updating the relevant view position history value on them.

this is not possible you can't access views from apply

I plan to remove document and view as separate structs entirely in the future. Separating them to begin with was fairly pointless since nearly all fields are public so there is little to no encapsulation to begin with (for example the jumplist is a gift that keeps on giving more and more crashes because the laz4y mapping is just not robusta and it can't be encapsulated because it needs to be accessed from document).

I want to refactor that so Document and View only become ids and all their data is stored in separate containers (slotmap or hashmap) instead of one big sprawling struct (so a more entity component-like system which in my experience is a better data model for large rust applications). The data would be grouped into files/substructs where encapsulation actually makes sense/is possible (litmus test: all fields can and should be private). These changes will be required anyway for plugins/making the event system more useful (and the config refactor). But it's a really large set of changes that I will take on down the line.

I am not concerned about the extra hashmap lookup, that is basically irrelevant. HasMaps are really really fast.

@intarga
Copy link
Contributor

intarga commented Apr 22, 2024

this is not possible you can't access views from apply

Good point.

I plan to remove document and view as separate structs entirely in the future.

In that case what I've implemented makes more sense. I'll clean it up and file a new PR. Thanks for the info!

@the-mikedavis
Copy link
Member

Closing in favor of #10559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to not centre the cursor when switching between buffers
4 participants