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

Show path in file picker and explorer #12806

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paholg
Copy link

@paholg paholg commented Feb 7, 2025

The file picker and explorer can each be opened in several ways, each causing it to have a different root path. What's more, the file explorer can change its path while you have it open.

This can make it very difficult to keep track of the root directory for these pickers, so we add it as a header.

If desired, we could alternatively make this a configuration option, but I see little downside to always including it.

Example:

2025-02-07_01 51 46-w

@nik-rev
Copy link
Contributor

nik-rev commented Feb 8, 2025

@David-Else is this what you were talking about when you said where the path of the pickers should be? The path is directly above the items, replacing the usual picker column headers. I actually really like this idea!

@@ -235,7 +235,7 @@ pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> FilePi
log::debug!("file_picker init {:?}", Instant::now().duration_since(now));

let columns = [PickerColumn::new(
"path",
root.to_string_lossy(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of showing the absolute path, I think it'd be cleaner to show the relative path. Using helix_stdx::path::get_relative_path

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good to me. The only issue is when a picker is opened at the working directory, we show a blank header. Any suggestions on what to do here?

As far as I can tell, everywhere else we call get_relative_path, it's for a file and not a directory, so it doesn't have this problem.

2025-02-08_13 04 37-w

Copy link
Contributor

Choose a reason for hiding this comment

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

If picker's current directory is the same as helix's current directory, then showing the full path in that case is probably fine

Copy link
Author

Choose a reason for hiding this comment

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

That becomes a bit jarring when using the file exploring and changing directories. I think it's better to have a consistent experience and just use the full path always if we're going to use it sometimes, but can change it if folks feel strongly.

Copy link
Contributor

Choose a reason for hiding this comment

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

That becomes a bit jarring when using the file exploring and changing directories. I think it's better to have a consistent experience and just use the full path always if we're going to use it sometimes, but can change it if folks feel strongly.

Agreed

@David-Else
Copy link
Contributor

@David-Else is this what you were talking about when you said where the path of the pickers should be? The path is directly above the items, replacing the usual picker column headers. I actually really like this idea!

Yes. this is where I thought it should be. Seeing it on the screen, I am still not sure it is correct. but best yet.

I agree it should show the relative path, regarding showing a blank line when opened in the root dir, why not show ./?

@nik-rev
Copy link
Contributor

nik-rev commented Feb 12, 2025

@David-Else is this what you were talking about when you said where the path of the pickers should be? The path is directly above the items, replacing the usual picker column headers. I actually really like this idea!

Yes. this is where I thought it should be. Seeing it on the screen, I am still not sure it is correct. but best yet.

I agree it should show the relative path, regarding showing a blank line when opened in the root dir, why not show ./?

Showing ./ is a good idea. Always showing the absolute path is inconsistent with other parts of the editor

@paholg
Copy link
Author

paholg commented Feb 13, 2025

I have updated it to show the relative path or ./, and to always show a trailing slash for increased consistency, and to make it clear that it's a directory.

Comment on lines 189 to 204
pub fn get_relative_dir(path: &Path) -> Cow<'static, str> {
let path = helix_stdx::path::get_relative_path(path);
if path.components().next().is_none() {
"./".into()
} else {
let mut str = path.to_string_lossy().into_owned();
if path.is_dir() {
str.push('/');
}

str.into()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this always called with a directory as entry (&root) ? Could we dispense the FS check (slow) then ?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, done.

The file picker and explorer can each be opened in several ways, each
causing it to have a different root path. What's more, the file explorer
can change its path while you have it open.

This can make it very difficult to keep track of the root directory for
these pickers, so we add it as a header.

If desired, we could alternatively make this a configuration option, but
I see little downside to always including it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants