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.

Sign up for GitHub

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

Jump to bottom

Hide duplicate symlinks from the picker #5658

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Hide duplicate symlinks from the picker #5658

merged 3 commits into from
Feb 2, 2023

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Jan 23, 2023 •
edited
Loading

Fixes #5395
Fixes #5725
Fixes #5210

This PR adds a config option to the picker to hide symlinks that point into the root directory of a picker.
These symlinks always point at files that would be included in the picker anyway and are therefore essentially duplicate.
The config option is enabled by default.

While working on this I noticed some small issues with the existing handling around symlinks.
They were included in the file picker if follow-links was disabled (which seems really odd, global_search already handled this correctly). Also added documentation for follow-links since that seems to have been missed.

Edit: The fix for filtering symlinks from the picker I described in the last paragraph more generally filters all kinds of "non standard" files like pipes, block devices, etc. Reading thise can lock up the edtior. By fixing that I actually accidentally fixed #5725

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 23, 2023
g-re-g
g-re-g reviewed Jan 25, 2023
View reviewed changes
Copy link
Contributor

@g-re-g g-re-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Solid feature. Duplicate entries due to symlinks is something I've run into as well. imo, everywhere links is used symlinks should be used. Personally I'm not one for abbreviation and in anycase both link and symlink are used in this PR and it should probably stick to one of the two.

helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
helix-term/src/lib.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe force-pushed the deadup_symlinks branch 2 times, most recently from dbe9290 to 1958fb2 Compare January 25, 2023 13:28
@pascalkuthe @g-re-g
Apply suggestions from code review
87f87a0 
Co-authored-by: g-re-g <123515925+g-re-g@users.noreply.github.com>
@pascalkuthe
Copy link
Member Author

pascalkuthe commented Jan 25, 2023 •
edited
Loading

Solid feature. Duplicate entries due to symlinks is something I've run into as well. imo, everywhere links is used symlinks should be used. Personally I'm not one for abbreviation and in anycase both link and symlink are used in this PR and it should probably stick to one of the two.

The follow-links setting already existed and a breaking change is not worth it for such a small rename and to stay consistent I also named the setting deduplicate-links. In the code I mostly use symlink tough (there was once place where I used links but I changed that now). The only place where it's called deduplicate-links is in the FilePickerConfig struct (which corresponds to the setting)

@pascalkuthe pascalkuthe mentioned this pull request Feb 1, 2023
Closed
the-mikedavis
the-mikedavis reviewed Feb 1, 2023
View reviewed changes
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.

Just a minor style nit, otherwise I think this looks good 👍

helix-term/src/ui/mod.rs Outdated Show resolved Hide resolved
@the-mikedavis
Copy link
Member

Ah nice, this fixes themes showing up twice in the helix repo under runtime/themes and through the contrib/themes symlink 👍

@pascalkuthe @the-mikedavis
minor stylistic fix
100ae2a 
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
@the-mikedavis the-mikedavis changed the title hide duplicate symlinks from the picker Hide duplicate symlinks from the picker Feb 2, 2023
@the-mikedavis the-mikedavis merged commit 6ed2348 into helix-editor:master Feb 2, 2023
@pascalkuthe pascalkuthe deleted the deadup_symlinks branch February 2, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g-re-g g-re-g g-re-g left review comments

@the-mikedavis the-mikedavis the-mikedavis approved these changes

Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
3 participants
@pascalkuthe @the-mikedavis @g-re-g