First stab at <Plug> mappings

[dhleong]

Using a non-existent key stroke is great, but ambiguous
plug mappings aren't supported out of the box. Imagine you
have two mappings, both of which take a motion:

More
MoreMagic

You've mapped gmm to More and gmc to MoreMagic.

When you type gmc, the recursive mapping is available, and the
else if (mappingInfo != null) branch can feed the mapped keys
back into the system. Because these are fed as recursive, when
the keys come back through, they can get the mapping to MoreMagic
(which is no longer a prefix) and execute.

For gmm, when it feeds the keys for More, it is still
a prefix; when a motion is inputted, it is no longer a prefix,
and so it would hit the else block, feeding the keys WITHOUT
recursion. So, we have to catch the case where we're no longer
a prefix, but can find a mapping within the input. This
seems to work pretty well, but causes "recursive records found"
warnings in the console, and complaints about local history being
broken. I'm not entirely sure whether this is just an idiosyncrasy
of the unit test environment, or whether this would cause
significant problems in the live environment. I'm hoping you have
more experience here about whether that warning can be ignored,
and am open to suggestions otherwise.

Also included is some prep work for -based repeat handling.
Extra input sequences past the mapping are stored in a
PlugInputModel which is read from in preference to the
ModalInputDialog.

[dhleong]

It seems like the "recursive records found" is also printed when I run the test on master, so either there's also a problem there... or it's no big deal.

[dhleong]

Also, it seems the ambiguous mappings is not just an issue for <Plug> mappings. See my branch https://github.com/dhleong/ideavim/tree/dhleong/CommentaryExtension. That provides two mappings; gc and gcc. gc expects an operator, while gcc operates on the current line. Because extensions are handled as mappings, the same situation as described above happens—gce, though expected to be valid, is not a prefix and does not have a mapping, so it is ignored in handleKeyMapping. It would be handled if it were built on top of this branch and mapped everything to <Plug> mappings first, but I'm not sure if we should force that just to get extensions to work.

@vlasovskikh thoughts?

[vlasovskikh]

I'll look at this PR in more detail on the weekend. The general idea should be to emulate ambiguous mappings as close to Vim as possible.

At the moment I'm very concerned with VIM-1086.

[dhleong]

I just added some more tests. The techniques here actually do handle ambiguous, direct (IE: not <Plug>) mappings, mostly because the KeyMapping#getPlug() method just looks for any extension handler.

There is still a bug if you do those sort of mappings with the <Plug> indirection, however. I'll look into that today.

[dhleong]

Okay just pushed some changes that fix the test case I had.

[vlasovskikh]

I'm not sure about this weekend, but this is the next PR I'll look at.

[albertdev]

@dhleong
I haven't read your code changes in detail but maybe consider enforcing that all plug names must start and end with '(' and ')' (and thus forbid them in the names themselves). It can at least remove ambiguity in plugs because you can't accidentally match some other plug which is a prefix of some other plug .

Fixing prefix remappings is then a different problem and at least won't suprpise extension authors.

[dhleong]

That's certainly one approach. I'm hesitant to break from Vim's behavior
here, but I'm not opposed to enforcing this best practice. Either way,
though, we have to handle the case where regular keymaps to
ExtensionHandlers are ambiguous, as well. This solution handles both cases
with the same mechanism, I believe.

On Thu, Feb 25, 2016 at 11:11 AM Bert Jacobs notifications@github.com
wrote:

@dhleong https://github.com/dhleong
I haven't read your code changes in detail but maybe consider enforcing
that all plug names must start and end with '(' and ')' (and thus forbid
them in the names themselves). It can at least remove ambiguity in plugs
because you can't accidentally match some other plug which is a prefix of
the http://stackoverflow.com/a/13698530/983949.

Fixing prefix remappings is then a different problem and at least won't
suprpise extension authors.

—
Reply to this email directly or view it on GitHub
#105 (comment).

[vlasovskikh]

I'm going to restore the original Vim behavior in IdeaVim regarding ambiguous mappings with timeout / notimeout and then use only the part of your PR about <Plug> pseudo-character. Extension authors should not use ambiguous mappings, and if they do, they are responsible for the correct behavior of their extensions, similar to the original Vim.

[vlasovskikh]

As for getchar() and input() in the case of ambiguous mappings, I'm going to investigate options here. Maybe it's not worth fixing at all, at least until there is real demand from the extensions where their emulation requires ambiguous mappings and user input.

[dhleong]

The comments extension requires ambiguous mappings. Doesn't use
getchar(), but it uses gcc to comment a line, and gc to comment a
text object. vim-exchange similarly allows cxx to select a line, and cx
to select a text object. Again, neither of these use getchar(), but
because the g@ stuff doesn't use the regular "argument" handling, this
common mapping strategy doesn't work without extra attention.

On Sun, Feb 28, 2016 at 6:38 PM Andrey Vlasovskikh notifications@github.com
wrote:

As for getchar() and input() in the case of ambiguous mappings, I'm going
to investigate options here. Maybe it's not worth fixing at all, at least
until there is real demand from the extensions where their emulation
requires ambiguous mappings and user input.

—
Reply to this email directly or view it on GitHub
#105 (comment).

[vlasovskikh]

I still haven't managed to spend some time working on this PR yet. Hope things will get better the next weekend.

[vlasovskikh]

I've (mostly) restored the original Vim behavior for ambiguous mappings in 530846b and added <Plug> for mapping commands in f280f81.

@dhleong I hope this is enough for your examples with gc and gcc. See the unit tests in 530846b. Thank you for your ideas on handling <Plug>.