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

[RDY] Apply :lmap in macros #5658

Merged
merged 9 commits into from May 17, 2018

Conversation

Projects
None yet
4 participants
@hardenedapple
Copy link
Contributor

commented Nov 24, 2016

I mentioned this patch in the bug #5652, but realised it should be a PR.

Vim ignores :lmap when executing a mapped command as documented in :help language-mapping, but this messes up macro replay.

Example:

:set iminsert=1<CR>
:lnoremap a l<CR>
qaiaaa<CR><Esc>q@a

which inserts two lines: lll and aaa.

This extra line adds an exception for macro replay.

@marvim marvim added the RFC label Nov 25, 2016

@justinmk

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

If I recall, this has come up on vim_dev recently (couldn't find the recent discussion, but here is an older one). Bram always rejects it. Should we break Vim-compat here? Why? What are the alternatives? Can you comment on that discussion (if it's relevant, I didn't look closely).

Maybe @chrisbra can comment. Also, did this discussion ever get fully resolved? (Is it relevant? I didn't look closely.)

cc @ZyX-I @blueyed

@justinmk justinmk added the bug-vim label Nov 25, 2016

@chrisbra

This comment has been minimized.

Copy link

commented Nov 25, 2016

I believe the discussion you are refering to has been merged as the 'langnoremap' option. However there have been a couple of more discussions, e.g. vim/vim#543. The problem is, this part of the code seems extremly fragile, changing something usually breaks in subtile ways, so one needs to have propper tests for those kind of changes.

@hardenedapple

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2016

As if to prove @chrisbra 's point of it being fragile, I noticed that my change added in some bad behaviour when a macro recording includes an :nmap that inserts some text including some :lmapped characters.

:nmap l ix
:lmap x q
qalll<CR><Esc>q@a

You see one line of xll and another of qll with my patch, and two lines of xll beforehand, so what I have suggested here is broken.

Reading the discussion of vim/vim#543 (thanks for pointing to that) I think the changes that are discussed there sound much better than the patch I gave here. Bram says

I look at keymap/langmap as a low level operation, which is applied when
typing on the keyboard, before it goes to the rest of Vim, which is
unaware of this filtering.

which would work with recording a macro and trying to replay it (unlike the current behaviour), and make things easier to understand than now.
Also, the discussion there of applying 'langmap' to cmdline mode seems sensible too.

As a side note: it seems that starting nvim with nvim -u NONE means that while :set langnoremap? says the setting is on, the effect isn't seen.

I'll try and comprehensibly write down the current behaviour and where it differs from the mental model of a conversion between the user typed characters and Vim itself, so we can see what other changes that simpler model would add. I'll also have a look at the patch in vim/vim#543 to see if that patch makes this change.

@hardenedapple hardenedapple changed the title [RFC] Account for :lmap in macros close #5652 [WIP] Account for :lmap in macros close #5652 Nov 26, 2016

@marvim marvim added WIP and removed RFC labels Nov 26, 2016

@hardenedapple

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2016

My opinion (though I'm biased because the current behaviour annoys me) is that
the current behaviour is too difficult to reason about, and in this specific
instance (macros and keymap) essentially blocks macros from being used.
At the moment, the only way to run a macro with characters you don't have on
the keyboard but have translated in the keymap is to manually put the text
into that register instead of recording it interactively, which is enough of a
pain that I don't do it.

After thinking more on it I would actually propose a different solution to my
problem: instead of applying :lmap mappings to the register when executing
it, I would apply them when interactively recording into the register (I
haven't yet checked whether this would be an easy change or not).
Assuming other users of keymap also don't record macros interactively because
of this difficulty, then this wouldn't break anyones workflow, and would make
macros with keymap much more usable.

Assuming the settings langnoremap and using :lmap then there are only
really two places that the mental model of a pre-translation of characters is
broken. One is what happens during macro recording that is the original problem
above, and the other is in the priority that mappings between :lmap and
:imap are applied.
Instead of :lmap getting priority (as it would be if the mental model would
hold, it depends on the order the mappings are defined).

Example:

:set iminsert=1
:lmap a l
:imap a i
oaaa

Inserts iii -- i.e. the imap mappings are applied first.

Although this is be dependant on the order the mappings were defined
Example:

:set iminsert=1
:imap a i
:lmap a l
oaaa

Inserts lll -- i.e. the lmap mappings are applied first.

I think this should be different, but frankly it bothers me much less, so I
don't mind if not.

I agree there should be thorough testing here -- it's been pretty difficult
for me to understand what's going on sometimes. I'll be happy to start on that.

In the meantime I'll have a look at what changes the vim/vim#543 PR introduces to see if there's anything interesting there.

@justinmk

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

instead of applying :lmap mappings to the register when executing it, I would apply them when interactively recording into the register

But then the input from the macro (during replay) is not respecting keymap or langmap? Which is correct, based on Bram's comments in vim/vim#543.

The 'keymap' option is meant for when typing text in Insert mode. It
does not apply to when typing commands, in Normal mode. The 'langmap'
option is the other way around.

A register contains commands, which may start Insert mode and then
insert some text. Thus it's a mix. Translating all characters with
'keymap' (or 'langmap') will cause problems.

I wonder how you managed to get characters into a register that you want
to apply 'keymap' or 'langmap' to. This translation should have already
happened when typing the characters.

And later:

Perhaps the problem is that when recording into
a register the keymap/langmap is not applied, resulting in the wrong
characters in the register?

I look at keymap/langmap as a low level operation, which is applied when
typing on the keyboard, before it goes to the rest of Vim, which is
unaware of this filtering.

@hardenedapple so I think you're right, the keys should be translated before being stored in the macro register. Based on Bram's comments, it is already expected to do so.

And using the Neovim test infrastructure it should be easy to correctly test this. (helpers.feed() is identical to raw user input)

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch 2 times, most recently from 0b12c7d to da2d92e Dec 16, 2016

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch 4 times, most recently from 9009c23 to b3035e1 Apr 3, 2017

@hardenedapple

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2017

I believe this fixes most of the problems with :lmap, 'keymap', and 'langmap' while still keeping the implementation essentially the same.

I intend to try and write a completely new implementation, but seeing as that will probably have backwards compatibility problems I'll keep that for a different PR.
It turned out my alternate implementation isn't any better at all.

@hardenedapple hardenedapple changed the title [WIP] Account for :lmap in macros close #5652 [RFC] Account for :lmap in macros close #5652 Apr 4, 2017

@marvim marvim added RFC and removed WIP labels Apr 4, 2017

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch 3 times, most recently from 0a5d7c8 to 69b6967 Apr 4, 2017

@hardenedapple

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2017

Making 'keymap' respect the mental model of a translation before getting to vim proper was a reasonably clean and simple task.
Making 'langmap' respect the mental model is much more awkward, and I would like some input on what we should aim for.

Hence I've made this PR just fix the 'keymap' / :lmap discrepancies, and will be opening another PR to show and start working on the 'langmap' behaviour.

hardenedapple added a commit to hardenedapple/neovim that referenced this pull request Apr 10, 2017

General 'langmap' stuff
This PR has been split off from neovim#5658.
That PR now only works with :lmap and 'keymap', while this one will work
on 'langmap'.
This is because the two are fundamentally different, and that changes to
'langmap' are much less clear-cut about what we should do.

First add some tests.

hardenedapple added a commit to hardenedapple/neovim that referenced this pull request Apr 10, 2017

General 'langmap' stuff
This PR has been split off from neovim#5658.
That PR now only works with :lmap and 'keymap', while this one will work
on 'langmap'.
This is because the two are fundamentally different, and that changes to
'langmap' are much less clear-cut about what we should do.

First add some tests.

hardenedapple added a commit to hardenedapple/neovim that referenced this pull request Apr 11, 2017

General 'langmap' stuff
This PR has been split off from neovim#5658.
That PR now only works with :lmap and 'keymap', while this one will work
on 'langmap'.
This is because the two are fundamentally different, and that changes to
'langmap' are much less clear-cut about what we should do.

First add some tests.
expect('llllaaa')
end)
it("mappings applied to keys created with :lmap", function()
feed_command('imap a x')

This comment has been minimized.

Copy link
@justinmk

justinmk Apr 19, 2017

Member

in general use command() instead of feed_command() unless the asynchrony is intended. Also feed_command() doesn't bubble errors up (though it does set v:errmsg, but that needs to be checked explicitly)

@@ -126,3 +128,208 @@ describe('input utf sequences that contain CSI/K_SPECIAL', function()
expect('')
end)
end)


-- Testing 'keymap'.

This comment has been minimized.

Copy link
@justinmk

justinmk Apr 19, 2017

Member

Let's put this in test/functional/options/keymap_spec.lua.

(test/functional/ui/... is for testing the UI)

@marvim marvim added the RFC label Jan 27, 2018

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch from 48d614d to 814cc8f Jan 28, 2018

@justinmk justinmk modified the milestones: 0.2.4, 0.2.3 Jan 28, 2018

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch from 814cc8f to 18a8ec5 Jan 31, 2018

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch from 18a8ec5 to 8d63cf7 Feb 10, 2018

@hardenedapple hardenedapple changed the title [RFC] Apply :lmap in macros [RDY] Apply :lmap in macros Feb 11, 2018

@marvim marvim added RDY and removed RFC labels Feb 11, 2018

hardenedapple added some commits Apr 3, 2017

Record :lmap transformed keys in gotchars()
The mental model of :lmap and 'keymap' is of a transformation done
before anything else. Hence when recording a macro, or writing to a
scriptfile, the transformed keys should be recorded instead of the keys
before the transformation.
Account for :lmap in macros
close #5652
Start by adding some tests
Ensure :lmap mappings take preference
If the mental model of :lmap mappings is a translation between your
keyboard and vim proper, then they should take preference over :imap
(and other) mappings. This patch makes that happen.
Update documentation
Update vim_diff.txt with :lmap differences, update documentation on
'keymap', and add tests.

The tests added are to demonstrate the behaviour specified in the
documentation of :loadkeymap.
Split :lnoremap test into done and pending
There is some behaviour that we keep with the recent changes, and some
behaviour that we change.
Instetad of having one failing test covering  all behaviour, we split
the test into two.
'keymap' now uses :lmap instead of :lnoremap
This means that the major way that :lmap mappings are applied works as
one would expect with macros.
This also means that having a translation with 'keymap' does not
preclude using mappings in insert mode with :imap.

@hardenedapple hardenedapple force-pushed the hardenedapple:lmap-macros branch from 8d63cf7 to 9058a5a Mar 14, 2018

@justinmk justinmk merged commit 2aa308c into neovim:master May 17, 2018

4 of 5 checks passed

QuickBuild Build pr-5658 finished with status FAILED
Details
codecov/patch 100% of diff hit (target 80.82%)
Details
codecov/project 80.85% (+0.03%) compared to 5ce8158
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@justinmk

This comment has been minimized.

Copy link
Member

commented May 17, 2018

Leave :lnoremap alone, letting users still have the functionality but possibly introducing confusing situations if they use :lnoremap to influence the behaviour of 'keymap' (my preferred option as this seems like an unlikely case).

Let's try that, and revisit if needed.

justinmk added a commit to justinmk/neovim that referenced this pull request May 21, 2018

justinmk added a commit to justinmk/neovim that referenced this pull request May 22, 2018

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 10, 2018

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 10, 2018

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 10, 2018

doc: job/channel, misc neovim#7783
doc: termios defaults. ref neovim#6992
doc: :help shell-powershell
doc: provider: Python minimum version is 2.7, 3.4
doc: remove :!start special-case. neovim#5844
doc: mention neovim#7917 change which accepts empty Array for Dictionary parameter
doc: <Cmd> pseudokey
doc: lmap change neovim#5658
doc: -s, -es

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 10, 2018

doc: job/channel, misc neovim#7783
doc: termios defaults. ref neovim#6992
doc: :help shell-powershell
doc: provider: Python minimum version is 2.7, 3.4
doc: remove :!start special-case. neovim#5844
doc: mention neovim#7917 change which accepts empty Array for Dictionary parameter
doc: <Cmd> pseudokey
doc: lmap change neovim#5658
doc: -s, -es

justinmk added a commit that referenced this pull request Jun 11, 2018

NVIM v0.3.0
FEATURES:
3cc7ebf #7234 built-in VimL expression parser
6a7c904 #4419 implement <Cmd> key to invoke command in any mode
b836328 #7679 'startup: treat stdin as text instead of commands'
58b210e :digraphs : highlight with hl-SpecialKey #2690
7a13611 #8276 'startup: Let `-s -` read from stdin'
1e71978 events: VimSuspend, VimResume #8280
1e7d5e8 #6272 'stdpath()'
f96d99a #8247 server: introduce --listen
e8c39f7 #8226 insert-mode: interpret unmapped META as ESC
98e7112 msg: do not scroll entire screen (#8088)
f72630b #8055 let negative 'writedelay' show all redraws
5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330
a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422)
207b7ca #6844 channels: support buffered output and bytes sockets/stdio

API:
f85cbea #7917 API: buffer updates
418abfc #6743 API: list information about all channels/jobs.
36b2e3f #8375 API: nvim_get_commands
273d2cd #8329 API: Make nvim_set_option() update `:verbose set …`
8d40b36 #8371 API: more reliable/descriptive VimL errors
ebb1acb #8353 API: nvim_call_dict_function
9f994bb #8004 API: nvim_list_uis
3405704 #7520 API/UI: forward option updates to UIs
911b1e4 #7821 API: improve nvim_command_output

WINDOWS OS:
9cefd83 #8084, #8516 build/win: support MSVC
ee4e1fd win: Fix reading content from stdin (#8267)

TUI:
ffb8904 #8309 TUI: add support for mouse release events in urxvt
8d5a46e #8081 TUI: implement "standout" attribute
6071637 TUI: support TERM=konsole-256color
67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3)
3d0ee17 TUI/rxvt: enable focus-reporting
d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior

FIXES:
ed6a113 #8273 'job-control: avoid kill-timer race'
4e02f1a #8107 'jobs: separate process-group'
451c48a terminal: flush vterm output buffer on pty output #8486
5d6732f :checkhealth fixes #8335
53f11dc #8218 'Fix errors reported by PVS'
d05712f inccommand: pause :terminal redraws (#8307)
51af911 inccommand: do not execute trailing commands #8256
84359a4 terminal: resize to the max dimensions (#8249)
d49c1dd #8228 Make vim_fgets() return the same values as in Vim
60e96a4 screen: winhl=Normal:Background should not override syntax (#8093)
0c59ac1 #5908 'shada: Also save numbered marks'
ba87a2c cscope: ignore EINTR while reading the prompt (#8079)
b1412dc #7971 ':terminal Enter/Leave should not increment jumplist'
3a5721e TUI: libtermkey: force CSI driver for mouse input #7948
6ff13d7 #7720 TUI: faster startup
1c6e956 #7862 TUI: fix resize-related segfaults
a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output
303e1df #7624 TUI: disable BCE almost always
249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum
6f41ce0 #7704 macOS: Set $LANG based on the system locale
a043899 #7633 'Retry fgets on EINTR'

CHANGES:
ad60927 #8304 default to 'nofsync'
f3f1970 #8035 defaults: 'fillchars'
a6052c7 #7984 defaults: sidescroll=1
b69fa86 #7888 defaults: enable cscopeverbose
7c4bb23 defaults: do :filetype stuff unless explicitly "off"
2aa308c #5658 'Apply :lmap in macros'
8ce6393 terminal: Leave 'relativenumber' alone (#8360)
e46534b #4486 refactor: Remove maxmem, maxmemtot options
131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343
c57d315 #8031 jobwait(): return -2 on interrupt also with timeout
6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940
300d365 #7919 Make 'langnoremap' apply directly after a map
ada1956 #7880 'lua/executor: Remove lightuserdata'

INTERNAL:
de0a954 #7806 internal statistics for list impl
dee78a4 #7708 rewrite internal list impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.