-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix picker preview #8265
Fix picker preview #8265
Conversation
helix-term/src/ui/picker.rs
Outdated
Self::EditorDocument(_) => "<Invalid File Location>", | ||
Self::Cached(preview) => match preview { | ||
CachedPreview::Document(_) => "<File preview>", | ||
CachedPreview::Document(_) => "<Invalid File Location>", |
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.
Why the change? Casing should match the rest of the variants
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 fixed the casing.
These branches were actually unreachable so far. We only show the placeholder if preview.document()
returns None
which doesn't happen for these two cases:
fn document(&self) -> Option<&Document> {
match self {
Preview::EditorDocument(doc) => Some(doc),
Preview::Cached(CachedPreview::Document(doc)) => Some(doc),
_ => None,
}
}
However, with this PR we also show the placeholder if range
is invalid so the only way to endup in this branch is if the location provided by whatever the picker is previewing is invalid (usually a problem with the LSP but for global search the file could also have been changed by another process since the initial search ran).
A way to make this a bit cleaner would be to document()
to have the following signature:
/// Returns the document to preview if available or a placeholder string otherwise
fn document(&self) -> Result<&Document, &str>
but I wanted to keep the change minimal here.
3bd21de
to
84e80a0
Compare
This PR fixes some issues with the picker preview.
Firstly it fixes a panic when previewing invalid ranges in the picker by simply only showing a placeholder in this case. Closes #8259.
While investigating this I noticed that #7837 had introduced a regression. In the past the picker centered all previewed ranges. However, after that PR only the fist line (instead of the middle of the previewed ranges) was centered.
I misread what the old code and therefore didn't notice during review. The second commit in this PR restores that behavior (can be easily tested by searching for
Modifier
in the global symbol picker in helix codebase) while keeping the soft wrap compatibility added by #7837.Adding to the release milestone since it's an easy fix for a crash plus a fix for a regression in the second commit.