-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat: Show filepath context in bufferline #13565
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
base: master
Are you sure you want to change the base?
feat: Show filepath context in bufferline #13565
Conversation
99a23ae
to
0676f6a
Compare
1abbf96
to
7d43d39
Compare
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.
Only a couple of small changes that came to mind. Other than that, looks good :)
8165ca5
to
eecd9ae
Compare
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.
Looks good to me, nice work!
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.
Hello! Seeing the many many pull requests, I want to try helping out with some code reviews. I'm new to helix, so weight my review accordingly.
Related: #13113 |
eecd9ae
to
9183108
Compare
/// editor. For example, documents `a/b` and `c/d` would resolve to `b` and `d` | ||
/// respectively, while `a/b/c` and `a/d/c` would resolve to `b/c` and `d/c` | ||
/// respectively. | ||
fn expand_fname_contexts<'a>(editor: &'a Editor, scratch: &'a str) -> HashMap<DocumentId, String> { |
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.
Instead of taking editor, could this take less context?
e.g. pass in documents: BTreeMap<DocumentId, Document>, or possibly just the Option, not the whole Document.
Related: could this function have a unit test below?
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.
Re: unit test, it probably could but I wasn't sure if it would be a value add for the project. There seem to be very few unit tests in here.
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.
Imo tests are good, and adding tests is better than not :)
This change adds functionality that computes the shortest unique filepath for each document in the editor to display as the title in the bufferline, along with the appropriate configuration options.
9183108
to
bc0f718
Compare
/// editor. For example, documents `a/b` and `c/d` would resolve to `b` and `d` | ||
/// respectively, while `a/b/c` and `a/d/c` would resolve to `b/c` and `d/c` | ||
/// respectively. | ||
fn expand_fname_contexts<'a>(editor: &'a Editor, scratch: &'a str) -> HashMap<DocumentId, String> { |
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.
Imo tests are good, and adding tests is better than not :)
@the-mikedavis I don't know who to ask, but could I get checks run on my PR? |
This change adds functionality that computes the shortest unique filepath for each document in the editor to display as the title in the bufferline, along with the appropriate configuration options.
Configuration changes
editor.bufferline
has been moved toeditor.bufferline.show
, and a new keyeditor.bufferline.context
has been added with the following variants:"none"
: Don't provide any additional context in the bufferline. This maintains previous functionality."minimal"
: Show the shortest unique filepath for each document in the bufferline. This is the new default.Setting the render mode directly in
editor.bufferline
is still supported, so existing configs won't break.Functionality
With
editor.bufferline.context = "none"
, the following set of documents is mapped as follows:book/src/editor.md
editor.md
helix-term/src/ui/editor.rs
editor.rs
helix-view/src/editor.rs
editor.rs
With
editor.bufferline.context = "minimal"
, the same set of documents is now mapped as follows:book/src/editor.md
editor.md
helix-term/src/ui/editor.rs
ui/editor.rs
helix-view/src/editor.rs
src/editor.rs
Verification steps
editor.bufferline.show
toalways
.editor.bufferline.context
tonone
.Possible additions
We could add another context variant
"full"
which would map documents to their full path (either from Helix's working directory or, if outside that, from the user's home directory (e.g.~/...
) or from the system root (e.g./...
).