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

Fix expansion of ~ #284

Merged
merged 6 commits into from Jun 18, 2021
Merged

Fix expansion of ~ #284

merged 6 commits into from Jun 18, 2021

Conversation

vv9k
Copy link
Contributor

@vv9k vv9k commented Jun 17, 2021

Closes: #280

// This function is used instead of `std::fs::canonicalize` because we don't want to verify
// here if the path exists, just normalize it's components.
pub fn canonicalize_path(path: &Path) -> std::io::Result<PathBuf> {
std::env::current_dir().map(|current_dir| normalize_path(&current_dir.join(path)))
Copy link
Member

Choose a reason for hiding this comment

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

You still want this because paths need to be converted to absolute. Not taking current_dir() into account will consider myfile.c and /full/path/to/myfile.c as two separate files.

Copy link
Member

Choose a reason for hiding this comment

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

@sudormrfbin
Copy link
Member

Autocompletion for :open isn't working yet and I think it'd be better to show the path with ~ instead of the fully expanded path in the status line.

@archseer
Copy link
Member

archseer commented Jun 17, 2021

The display part should be fixed by modifying this:

pub fn relative_path(&self) -> Option<&Path> {
let cwdir = std::env::current_dir().expect("couldn't determine current directory");
self.path
.as_ref()
.map(|path| path.strip_prefix(cwdir).unwrap_or(path))
}

@vv9k vv9k changed the title Fix expansion of ~, dont use directory relative to cwd. Fix expansion of ~ Jun 17, 2021
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Tested and works good, let's change doc.relative_path to replace the home path with ~ as well

@vv9k
Copy link
Contributor Author

vv9k commented Jun 18, 2021

Tested and works good, let's change doc.relative_path to replace the home path with ~ as well

Yes, completely forgot about that one. Updated, opening home dir path now looks like this after opening the full path /home/user/dev/rust/helix/Cargo.toml:

image

@@ -1070,8 +1070,8 @@ mod cmd {
.filter(|doc| doc.is_modified())
.map(|doc| {
doc.relative_path()
.and_then(|path| path.to_str())
.unwrap_or("[scratch]")
.map(|path| path.to_string_lossy().to_string())
Copy link
Member

Choose a reason for hiding this comment

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

No need for to_string() here, it's already a Cow I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunatelly:

error[E0515]: cannot return value referencing function parameter `path`
    --> helix-term/src/commands.rs:1073:33
     |
1073 |                     .map(|path| path.to_string_lossy())
     |                                 ----^^^^^^^^^^^^^^^^^^
     |                                 |
     |                                 returns a value referencing data owned by the current function
     |                                 `path` is borrowed here

Copy link
Contributor Author

@vv9k vv9k Jun 18, 2021

Choose a reason for hiding this comment

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

It is a consequence of changing the return type of relative_path to be owned PathBuf not &Path but I'm not sure I can return a &Path while still folding home directory into tilde.

@archseer archseer merged commit 41b0748 into helix-editor:master Jun 18, 2021
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.

File picker should have an option to display ignored files
3 participants