Support modifiers in special key mappings

[joelim-work]

Fixes #205
Fixes #1163

Closes #943 (tcell can't distinguish between <enter> and Shift+<enter>)

---

For me the following mappings now work and are all distinguishable from each other:

map <down> echo down
map <c-down> echo ctrl down
map <a-down> echo alt down
map <s-down> echo shift down

cmap expressions are also supported:

cmap <a-up> cmd-menu-complete-back
cmap <a-down> cmd-menu-complete

push commands also work with these new mappings:

map <a-enter> echo hello world
push <a-enter>

It would be nice if someone could also try this out to see if they can map some new key combinations.

[ilyagr]

I've been thinking about this as well. I don't remember if there are any important subtleties, but I'd like to double-check and compare it to what I did in a bit more detail.

One observation is that these keys will not work with the push command. I think that is OK (fixing it doesn't seem remotely worth the complexity), but should be mentioned in the docs for that command. I'd say something like: "Not all modifiers are supported by this command. As a rule of thumb, keys that lf binds in its default config are safe to use. Mouse events are unsupported".

[ilyagr]

I think this PR works fine.

For reference, there still are some keys we could, in principle, support but don't even after this PR. For example: <a-c-home>, <a-c-f>, <a-space>, maybe <c-space>, <a-lt>.

Some of this keyboard-handling code makes me itch to try to make it more systematic, but I'm not sure that's easy or worth the effort. If we want to do that, we should write some unit tests first.

[DusanLesan]

@joelim-work These changes work for me well and are very welcome. Thank you.

[joelim-work]

@ilyagr Thanks very much for your review comments. I have actually managed to implement push for these mappings, so that commands like :push <a-down> work, but I had to rework the code by a fair amount. There are still some cases that don't work like combining modifiers with <space>, <lt> and <gt>, and combining multiple modifiers, but at this stage I have come to the conclusion that the code is in need of a refactor plus unit tests, and the logic is already complex enough that it's not worth supporting anything else for the time being.

[joelim-work]

@DusanLesan Thanks for helping out. I updated the PR description with a few more things that I have changed. If you're interested you can help me by playing around with the new changes to see if they work for you.

[ilyagr]

at this stage I have come to the conclusion that the code is in need of a refactor plus unit tests, and the logic is already complex enough that it's not worth supporting anything else for the time being.

+1. I think simplifying this is a totally reasonable project. The main reason I haven't tried to do it myself is that the current version, in spite of all of its imperfections, does seem to cover all the essential stuff.

I'm of two minds about your latest improvements (support for push and cmap bindings). On one hand, your code looks good (and suspiciously similar to what I would've done in parts). On the other hand, I think you went a little far: this add complexity and merely hides some of the fundamental mismatch between the code for map bindings, the code for cmap bindings, and the push parsing. We don't really need push to be able to parse crazy bindings.

I guess now that you did it all, we can keep it, though maybe I'll give it another look to search for potential bugs just in case.

[joelim-work]

I'm of two minds about your latest improvements (support for push and cmap bindings). On one hand, your code looks good (and suspiciously similar to what I would've done in parts). On the other hand, I think you went a little far: this add complexity and merely hides some of the fundamental mismatch between the code for map bindings, the code for cmap bindings, and the push parsing. We don't really need push to be able to parse crazy bindings.

I actually sort of had the opposite experience. At first I thought the only thing that needed change was the map command. Then you pointed out that the new mappings would not work with the push command, so I felt that it was missing functionality and decided to implement it as well. And then I realised that there were cmap mappings as well.

I understand that for the most part users would only care about map, but I feel that it is better to maintain consistency between map, cmap and push as much as possible, otherwise it could be confusing for users.

[gokcehan]

@joelim-work Thanks for the patch. @ilyagr is right that the additional logic is somewhat complex, though the patch itself looks pretty clean so I think it's ok. I'm also inclined towards consistency in this case. Also, I have just released r30 so there will likely be plenty of time to fix things until r31 is released if something is broken.