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 issue with opening a relative directory with hx <dir> introduced in #8498 #8520

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

bjorn-ove
Copy link
Contributor

When opening the editor using hx <dir> where <dir> is relative it would either open the wrong directory in the picker, or a new file.

This was introduced by #8498

My solution is to replace the first argument by "." when it is a directory and override args.working_directory. This should effectively leave the original logic in place while still solve the config issue.

@woojiq
Copy link
Contributor

woojiq commented Oct 11, 2023

Does this fix #8515?

@bjorn-ove
Copy link
Contributor Author

Does this fix #8515?

Yes, it should

@the-mikedavis the-mikedavis linked an issue Oct 11, 2023 that may be closed by this pull request
@the-mikedavis the-mikedavis added C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 11, 2023
@the-mikedavis the-mikedavis added this to the 23.10 milestone Oct 11, 2023
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 11, 2023
@bjorn-ove
Copy link
Contributor Author

The new solution should work well together with working_directory. I have also rebased against master.

pascalkuthe
pascalkuthe previously approved these changes Oct 12, 2023
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I'd like to avoid passing around a flag like this, it seems leaky / ad-hoc to me. I think it should work to turn all relative paths we get in the args to absolute, relative to the current working directory at start-up time (before we switch the working directory to the one specified with -w). That should also have less confusing behavior when using the -w flag: hx -w path/to/helix Cargo.toml should open the Cargo.toml in the current directory rather than Helix's Cargo.toml IMO

@bjorn-ove
Copy link
Contributor Author

I also didn't like that flag, but it was the cleanest I could come up with while keeping the original behaviour. Since that was to be relative from -w <dir>, I tried to keep it.

This is also how all other tools I know work. If you run tar -C <dir> hello.txy it changes the directory before opening hello.txt. Usually it is basically an alias for (cd <dir> && tool arg1 arg2 arg3)

I also thought about joining working_directory and files[0], but to make that behave correctly on all platforms is complex.

@bjorn-ove
Copy link
Contributor Author

Another issue with your suggestion is non existing paths. As canonicalize needs the path to exist.

@the-mikedavis
Copy link
Member

Hmm yeah, let's follow other tools then and keep those files relative to the working path rather than cwd at invocation.

The behavior here seems ok then but I'd still like to clean up that rogue flag - it seems like that's leaking out of args. Let's make it a part of the Args struct, something like open_cwd

@bjorn-ove
Copy link
Contributor Author

it seems like that's leaking out of args. Let's make it a part of the Args struct, something like open_cwd

The reason I didn't do this is that I wanted to leave the args parsing without side-effects. And since I depend on setting the working directory before I check if it is a directory, that wouldn't work. I could make it mutable in main() and set it there, if you think that is acceptable?

@the-mikedavis
Copy link
Member

Yeah that's what I had in mind - making it let mut args = .. in main and updating the flag where you set the boolean now

@pascalkuthe pascalkuthe merged commit e6d2835 into helix-editor:master Oct 18, 2023
6 checks passed
@bjorn-ove
Copy link
Contributor Author

Hmm yeah, let's follow other tools then and keep those files relative to the working path rather than cwd at invocation.

I may have unintentionally lied about other tools. At least for tar, it will open the file from -f from the current directory, then change, and create all files in the new location.

So it might actually make sense to iterate trough all files in main() and resolve to full path before altering the directory.

Should I look into this, or just leave it as it is now?

@the-mikedavis
Copy link
Member

Hmm yeah, let's resolve the paths before altering the directory. IMO that behavior is more intuitive and I believe shell completion should help with that approach (it should assume files are relative to the shell's cwd)

danillos pushed a commit to danillos/helix that referenced this pull request Nov 21, 2023
dgkf pushed a commit to dgkf/helix that referenced this pull request Jan 30, 2024
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File picker not showing on directory open
4 participants