-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Show file preview in split pane in fuzzy finder #534
Show file preview in split pane in fuzzy finder #534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool but I have some concerns about performance, the loading should probably be async. Kind of hard to make it work with the current system though.
The current selected item is cloned on every event, which is undesirable
Files already loaded in memory have syntax highlighting enabled
I think this is good enough to merge after you resolve that last TODO :) |
Done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor nits!
helix-term/src/compositor.rs
Outdated
for layer in &self.layers { | ||
layer.render(area, surface, cx) | ||
for layer in &mut self.layers { | ||
layer.prepare_for_render(cx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you just do this in render()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require changing render()
to take a mutable ref to self, since it calculates the preview and stores it in the cache. I didn't want to do that since having a render function that could potentially change the state seemed like questionable design; should I simply have render()
take &mut self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature of render already takes a mut: pub fn render(&mut self, cx: &mut Context)
I think I changed that for some ui
component previously for the same reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant Component
's render function actually; changed it to take &mut self
anyway since the topmost render takes mutable reference anyway.
I think you forgot to push the latest commits! I'll do a review once those are up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🎉
TODO
EditorView::render_buffer()
Bugs
helix/helix-view/src/view.rs
Line 124 in ab41e45