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

Add c-f c-d keymap to file_picker to filter option. #1405

Closed
wants to merge 3 commits into from

Conversation

cossonleo
Copy link
Contributor

add c-f to file_picker for filter path starts_with current file's dir
add c-d to file_picker for filter path starts_with cwd
Peek 2021-12-30 13-45

EventResult::Consumed(None)
}
ctrl!('f') => {
if let Some(cwd) = doc!(ctx.editor).path().and_then(|p| p.parent()) {
Copy link
Member

Choose a reason for hiding this comment

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

cwd is misleading, as the current file will not always be in the current working directory. This should be something like doc_dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does doc_dir mean?

Copy link
Member

Choose a reason for hiding this comment

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

Document directory...

@@ -306,6 +306,15 @@ Keys to use within picker. Remapping currently not supported.
| `Ctrl-v` | Open vertically |
| `Escape`, `Ctrl-c` | Close picker |

## File Picker extra keymap
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason these can't just go into the Picker section's table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shortcut only works on FilePicker.

Copy link
Member

Choose a reason for hiding this comment

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

Are you telling me that these shortcuts in the File Picker section work outside of the file picker?

helix/book/src/keymap.md

Lines 297 to 309 in ddbf036

# Picker
Keys to use within picker. Remapping currently not supported.
| Key | Description |
| ----- | ------------- |
| `Up`, `Ctrl-k`, `Ctrl-p` | Previous entry |
| `Down`, `Ctrl-j`, `Ctrl-n` | Next entry |
| `Ctrl-space` | Filter options |
| `Enter` | Open selected |
| `Ctrl-s` | Open horizontally |
| `Ctrl-v` | Open vertically |
| `Escape`, `Ctrl-c` | Close picker |

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, those shortcuts works on any picker and this is specific to file picker.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhh, I see, sorry for my confusion. In that case, could we update the title to just "File picker", and update the wording in the sentence just below here to say that these are specific to the file picker?

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@cossonleo cossonleo force-pushed the file_picker_improve branch from 69020ed to 6d36207 Compare January 11, 2022 05:48
@cossonleo cossonleo requested a review from kirawi January 11, 2022 05:48
@twe4ked
Copy link

twe4ked commented Jan 11, 2022

Love the idea of having this functionality. Just calling out that there is an open issue (#1467) to add C-d and C-u mappings to the picker UI, these don't conflict but I think it might be worth considering what other mappings might be added in future.

Another alternative could be to introduce more SPACE mappings for these specific cases? Or potentially exposing these as commands (not sure what to call this?) that people can add their own mappings for (with no default).

Comment on lines 276 to 283
ctrl!('a') => {
if let Ok(cwd) = std::env::current_dir() {
filter(cwd.as_path());
}
EventResult::Consumed(None)
}
ctrl!('f') => {
if let Some(cwd) = doc!(cx.editor).path().and_then(|p| p.parent()) {
filter(cwd);
}
EventResult::Consumed(None)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this seemed oddly specific. Do you know any prior art that do something similar? I don't see something like this in doom emacs IIRC or maybe I didn't dig deep, also stuff like these are hard to discover. And what is your usecase?

I wonder if it would be good to C-r % to paste the current file and C-w to remove the last file path to do the same without doing something so specific?

@hovsater
Copy link
Contributor

hovsater commented Jan 15, 2022

While I think these filters are a nice addition, I also think we should be careful with binding ctrl+ combinations. As @twe4ked pointed out, C-d and C-u are sometimes used for navigating a page down or up. As an example, we're already using it for the popup at hover.

@hahanein
Copy link
Contributor

hahanein commented Jan 20, 2022

ctrl-f, too, is being used instead of page down: #41 cbb3eba

Maybe you could start a search string in a specialized way and helix would know to interpret it as doc_dir?

. mysearchstring

...would only look up entries in /path/to/doc_dir/.

Or more restrictive but more obvious, too:

./mysearchstring

...translates to effectively searching for:

/path/to/doc_dir/mysearchstring

@bbodi
Copy link
Contributor

bbodi commented Jan 21, 2022

If this will be merged, #1352 can be closed as this one solves the same issue without taking away any space shortcut.

@cossonleo
Copy link
Contributor Author

How about c-a and c-l?

@cossonleo cossonleo force-pushed the file_picker_improve branch from 6d36207 to 8e985be Compare January 24, 2022 07:52
@pickfire
Copy link
Contributor

pickfire commented Jan 24, 2022

If this will be merged, #1352 can be closed as this one solves the same issue without taking away any space shortcut.

Nope, @bbodi I think it would be good to have space f F for that later, but maybe for now that can still be space F.

How about c-a and c-l?

Not sure if we want this but I think it would be good to rethink the approach. For me I think it's better to ctrl-r % to paste the current file name and ctrl-w to remove the filename, I think that is more consistent with normal mode and command mode so users don't have to learn new commands, instead of the approach taken here (at least I don't think it's common enough to warrant a shortcut), say if you want to go up two directories, just ctrl-w twice, or if you want to open the parent directory of alternate file, just ctrl-r # and ctrl-w. @cossonleo What do you think?

@twe4ked
Copy link

twe4ked commented Jan 25, 2022

How about c-a and c-l?

FWIW C-a is often jump to start of line in emacs bindings (most shells and common in REPLs).

Could we somehow make this optional for a time with a way to add it as a keybind? Then see how the bindings progress across the editor?

add c-f to file_picker for filter path starts_with current file's dir
@pickfire
Copy link
Contributor

pickfire commented Feb 16, 2022

If you ask me, I probably won't give approval for this pull request since it is very specific and not inter-changable with other parts, other users may not be able to figure this out easily. If anything I still think ctrl-r % + ctrl-w is a more flexible option to let users remove as many subpath if they want. Say if a user want a parent directory of parent directory, the current approach does not support that. Not sure about other maintainers, this is only my personal opinion.

@the-mikedavis the-mikedavis added the S-needs-discussion Status: Needs discussion or design. label May 18, 2022
@pickfire
Copy link
Contributor

pickfire commented Jun 8, 2022

#2458 (support insert register in prompt) is now merged, so if we add support for ctrl-r % current file register and tweak the behavior of ctrl-w in prompt to delete until the parent directory in prompt, I guess this should be set to be able to use ctrl-r % + ctrl-w which is more interoperable with other parts.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Sep 13, 2022
@pascalkuthe
Copy link
Member

I am closig this PR as it has gone stale. I also don't think we will be looking to add a feature like this to core. Its too specific/niche and IMO better covered by features like c-F.

Thank you for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-needs-discussion Status: Needs discussion or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants