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 :keys command to list current key bindings. #918

Closed
wants to merge 1 commit into from

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Aug 27, 2022

This outputs the result of listBinds to a temporary file
and then pass it to $PAGER. A bit of care is taken to remove
the temporary file and refresh lf (just in case it's showing
/tmp).

I haven't tested it on Windows, since lf doesn't seem to work
on wine, but I think it should work there too.

It would be a bit more elegant to pipe the output directly to
$PAGER without a temporary file, but that would require larger
code changes, which doesn't seem worth it at the moment.

This is also the first Go code I ever wrote, so let me know if something
could be more idiomatic.

Fixes #916.

@p-ouellette
Copy link
Contributor

Maybe use :map with no arguments instead (like vim)?

@ilyagr
Copy link
Collaborator Author

ilyagr commented Sep 8, 2022

That's an interesting idea, @pauloue, but I'd rather not. A separate command is easier to remember, for me at least. A somewhat more objective argument is that in lf, map <key> unmaps <key> . In vim, map <key> shows mappings that start with <key>. So, your idea would make less sense in lf than it does in vim.

That said, it's not a hill I'd be willing to die on. Both are OK options to me.

ilyagr added a commit to ilyagr/lf that referenced this pull request Sep 20, 2022
This mainly merges gokcehan#924 with @laktak's
gokcehan#861. This also merges in the `:keys`
command (gokcehan#918) as well as 
and some unrelated minor corrections (currently
dd5dc59).
ilyagr added a commit to ilyagr/lf that referenced this pull request Oct 9, 2022
This mainly merges gokcehan#924 with @laktak's
gokcehan#861. This also merges in the `:keys`
command (gokcehan#918) as well as 
and some unrelated minor corrections (currently
dd5dc59).
@ilyagr
Copy link
Collaborator Author

ilyagr commented Oct 9, 2022

Just rebased this on top of master to get rid of conflicts.

@gokcehan
Copy link
Owner

@ilyagr Thanks for the patch and sorry for late reply. I'm going over past patches as part of #950. I'm hesitant about creating a temporary file for this as we have been trying to remove temporary files (e.g. log files). I haven't looked at this in detail but would it be too difficult to avoid the temporary file?

@ilyagr ilyagr force-pushed the list-keys branch 3 times, most recently from 19601b0 to 52f4a41 Compare October 16, 2022 00:01
@ilyagr
Copy link
Collaborator Author

ilyagr commented Oct 16, 2022

Well, I do remove the temporary file ASAP. :)

I'll look into not using one when I have a moment. I'll need to use lower-level functions. Also, I don't know how well it'll work on Windows and have no way of checking. Otherwise, it sounds totally doable.

@gokcehan
Copy link
Owner

@ilyagr How about exporting an environment variable for this purpose (e.g. $lf_keys) so users can define a shell command (e.g. cmd keys $printf $lf_keys | less). Would that be a simpler approach?

@laktak
Copy link
Contributor

laktak commented Oct 16, 2022

How about exporting an environment variable for this purpose

Is it a good idea to have large(?) env. variable that has to be set for every shell invocation?

@laktak
Copy link
Contributor

laktak commented Oct 16, 2022

You can pipe stdin to the pager. If no pager is defined just print out everything and let the user scroll back in his terminal.

@ilyagr ilyagr force-pushed the list-keys branch 3 times, most recently from 439bfcf to e5e1f1f Compare October 16, 2022 19:24
@ilyagr
Copy link
Collaborator Author

ilyagr commented Oct 16, 2022

I prefer @laktak stdin approach, though I could live with the environment variable approach.

In some ways, @gokcehan 's approach is more flexible, and a kilobyte-long environment variable shouldn't cause any real problems as far as I can tell. Personally, however, I find such variables a bit unsightly. The extra configuration required seems to be a downside to me that might not be worth the extra flexibility.

I'll give this a bit of time in case I (or anyone) has more thoughts and until I have the time to look at the details.

This outputs the result of [`listBinds`] to a temporary file
and then pass it to `$PAGER`. A bit of care is taken to remove
the temporary file and refresh lf (just in case it's showing
`/tmp`).

I haven't tested it on Windows, since `lf` doesn't seem to work
on `wine`, but I think it should work there too.

It would be a bit more elegant to pipe the output directly to
`$PAGER` without a temporary file, but that would require larger
code changes, which doesn't seem worth it at the moment.

[`listBinds`]: https://github.com/gokcehan/lf/blob/e03143299d4596bbc9bb8a2d8a1b0931e3183410/ui.go#L940
@gokcehan
Copy link
Owner

@ilyagr Is there a consensus for this patch?

I have read the whole discussion again now. I'm not sure how @laktak 's stdin approach should work but I feel like it is better not to mess with stdin/stdout/stderr. Performance overhead of environment variables is a valid concern but we haven't considered it so far with other things (e.g. we also export all options for each command). I wouldn't expect much performance overhead but it might be worth to measure. There might also be some copy-on-write mechanism in the OS to avoid such overheads. We could also work on minimizing os.Setenv calls to only update variables when necessary if that makes any difference.

Personally I think the extra configuration required is somehow justified. So far I have tried to keep a single documentation so users would not hesitate where to look for when they need help. And to be completely honest, I also fail to see much benefit for adding a :keys command as the quick reference section in the doc has already a similar purpose. Of course the difference is that the documentation is not updated dynamically. But if a user does not remember their own bindings, do we really need to hand hold them? :map is required for a program like vim not only because it has lots of bindings by default but also plugins can add keybindings as well which can be in any of the files possibly in thousands.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Nov 28, 2022

I ended up busy with other things, but I mean to come back to this (around Christmas, maybe).

I use lf with this PR, and use :keys relatively often. It mainly is useful to remember how to do operations I use less often, both those that have default bindings and those defined in my config.

The stdin approach should work. It would be implemented similar to the $ command, but with stdin bound to a buffer. I am still thinking about whether this could be useful for purposes other than :keys. As part of this, I wanted to do a refactoring of the shell calling code as well, to make the logic more clear and make UI refresh after & operation happen after the operation finishes (perhaps I should do this a separate PR).

These are just some quick thoughts, I haven't reached any conclusions yet.

@gokcehan
Copy link
Owner

@ilyagr With all due respect, why do you have bindings for rarely used commands? Can't you just keep them as named commands if you rarely use them? Or bind them under the same prefix to show as menu? (e.g. map aa command1, map ab command2, ...). You can also have a binding to show your config file in a pager or editor (e.g. map <f-2> $less ~/.config/lf/lfrc). You could copy the default bindings to your config file to keep them as comment so you wouldn't need to jump to the doc. Is this really something we should provide by default? Aren't there better ways to spend your holiday?

@laktak
Copy link
Contributor

laktak commented Dec 10, 2022

I've actually been using something similar to @gokcehan's idea for a few years with vim and lf:

map <space>h $tput smcup; clear; grep -E ^#: ~/.config/lf/lfrc|cut -c 4-|less -+F

With that I can document my config like

#: === copy/paste
#: pl      paste link
map pl ...
#: pt      paste time
map pl ...

and generate my own doc with the previous <space>h keybindng:

=== copy/paste
pl      paste link
pt      paste time

I prefer this as it is much nicer to read than looking at the "raw" mapping of pt to some touch arguments ...

@ilyagr
Copy link
Collaborator Author

ilyagr commented Dec 11, 2022

OK, I think there's not a consensus that this feature should be part of lf, so it's probably time to close the PR. I plan to continue maintaining it (mainly to resolve merge conflicts) so that I can keep it in my personal version of lf. I'll see if there's an easy way for me to support other people patching it in. If so, I'll create a Patches section in the wiki. @laktak can then add his focus events PR there.

There doesn't seem to be much point to reworking this patch to avoid using a temporary file at this point.

For the record, @gokcehan , I have binding for commands that I don't use everyday, but use many times in a row when I do use them. For instance follow symlink and paste symlink commands. I use some built-in commands the same way.

I think the idea of combining help with commands that show the config file is a decent one. I don't like the friction it creates, but I might be able to get used to it. @laktak 's version of this idea is clever, though it might be a bit too much work for me to maintain.

Thanks, everyone, for your thoughts and for taking a look!

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.

FR: Command to list current keybindings
4 participants