try to fix #916: add maps to show all mappings
#1146
Conversation
jackielii
changed the title
maps to show all mappings
|
I suggest we rename this to |
|
I have implemented the If interested, I'll submit that as a PR |
There was a problem hiding this comment.
Thanks for the PR! I quite like the idea of maps as the name of the command. Update: Something like list-mappings, list-buffers also sounds good to me. I was thinking about the possibility of cmaps, but if we ever want that, it could be list-cmd-mappings instead.
I am not sure what chance, if any, this has of going into lf. That's up to @gokcehan.
I did leave a couple of comments from a technical point of view. I wouldn't spend too much time on my comments unless we get a go-ahead with this PR and this approach.
| // This function is used to run a shell command. Modes are as follows: | ||
| // | ||
| // Prefix Wait Async Stdin Stdout Stderr UI action | ||
| // $ No No Yes Yes Yes Pause and then resume | ||
| // % No No Yes Yes Yes Statline for input/output | ||
| // ! Yes No Yes Yes Yes Pause and then resume | ||
| // & No Yes No No No Do nothing | ||
| func (app *app) runShell(s string, args []string, prefix string) { | ||
| // $| No No Pipe Yes Yes Pause, Pipe execute, return cleanup to resume | ||
| func (app *app) runShell(s string, args []string, prefix string) cleanFunc { |
There was a problem hiding this comment.
Instead of returning a cleanup function, I would either make $| a separate function or (this is probably easier):
- rename this function to
runShellInternaland add ansendToStdin stringargument to it. - create a new
runShellfunction that callsrunShellInternaland passes""forsendToStdin - create a
runShellWithStdingfunction with argumentss,args, andsendToStdinthat callsrunShellInternal(s, args, "$|", sendToStdin).
Perhaps we can think of something more elegant than exactly what I suggested. However, we should avoid returning functions that the caller must to remember to call in some cases, but are completely ignored in other cases.
There was a problem hiding this comment.
That's certainly a good idea! I did thought of the possibility that cleanup is not called. Although I figured the docs should help.
Separating the runShell in different modes should certainly help with cleanness of the code. Although existing runShell feels a bit cramped up as well, there is a good separation of start, run/start, cleanup state in it, but the switch cases had to be duplicated a couple of times. I think $, % etc should be separated as well.
Anyway, that's just some of my thoughts. I'll definitely try to get this in another function as the $| is a internal one. I have the list-mappings etc in a branch ready to go. I'll modify all this in the other PR. sending it soon.
| @@ -2076,6 +2076,11 @@ func (e *callExpr) eval(app *app, args []string) { | |||
|
|
|||
| app.ui.cmdAccLeft = acc | |||
| update(app) | |||
| case "maps": | |||
| cleanUp := app.runShell("$PAGER", nil, "$|") | |||
There was a problem hiding this comment.
You should try doing something like what I did in #918 to try to support Windows.
It'd be even better if somebody could test it on Windows.
There was a problem hiding this comment.
You're right! I'm on Mac. Didn't really test on Windows.
Although did a quick search seems pipe should work for windows. We're only invoking pager...
|
@jackielii Thanks for working on this. I think I'm ok with this patch. @ilyagr From what I understand, I think you are also ok with this patch as well. @jackielii One small suggestion/nitpick, can we not simply use |
|
By the way, this does not seem to work on windows unfortunately. |
|
This wasn't quite ready to be merged, unfortunately. I'll look at the fix-up PR when I get a moment. |
|
@ilyagr Sorry for rushing the merge. I just did not want to leave this feature out as you have been really helpful as a contributor lately and proven yourself to be responsible. Feel free to make any changes with @jackielii in the followup patch or patches and let me know when it is ready. |
This makes the `:maps` command from gokcehan#1146 work on Windows. This (as well as `:maps` itself) is based on @jackielli's work in gokcehan#1146 and gokcehan#1152. I intend to refactor the code in this or a separate PR, but I thought I'd submit the critical and simple bit first.
After the naming scheme from gokcehan#1146, it would seem weird to not have a :cmaps command. This could help with issues like gokcehan#876.
* Make `:maps` work on Windows This makes the `:maps` command from #1146 work on Windows. This (as well as `:maps` itself) is based on @jackielli's work in #1146 and #1152. I intend to refactor the code in this or a separate PR, but I thought I'd submit the critical and simple bit first. * Add `:cmaps` command After the naming scheme from #1146, it would seem weird to not have a :cmaps command. This could help with issues like #876.
I know there is a previous effort at #918 . I took the stdio suggestion. I think this opens up doors for other internal state to be echo'ed easily, e.g. copy/cut buffers, selections etc