Langmap things #6494

hardenedapple · 2017-04-10T14:26:13Z

I had written some tests initially intending to fix the problems they show.
I've changed my mind, and don't plan on fixing them, as, frankly it's not a pain point for me, and it seems like a lot of effort. I've made some improvements in the 'langmap' option by recording the translated keys.

I have also covered two special cases that were previously not translated. I think this is an improvement, but others may disagree.
I have left other special cases alone despite thinking they may be better changed -- that's a topic I'd like to get input on.

n.b. the recording of 'langmap' translations in macros was discussed here.

justinmk · 2017-05-13T19:36:11Z

test/functional/normal/langmap_spec.lua

+    end)
+    -- pending('-- More -- prompt', function()
+    --   -- The 'b' 'j' 'd' 'f' commands at the -- More -- prompt
+    -- end)


is this still needed?

The test should be implemented, but I couldn't immediately see how to write it and just left it as a comment.

justinmk · 2017-05-13T19:36:43Z

test/functional/normal/langmap_spec.lua

+local curwin = helpers.curwin
+
+describe("'langmap'", function()
+  clear()


is this needed? it's already in before_each().

justinmk · 2017-05-13T19:42:22Z

In 5414960 this wording doesn't make sense to me:

This changes functionality it two places that used to be in my
"exceptions" category. It now applies 'langmap' in these cases when it
didn't before.
I'm not "fixing" them because I believe they should be changed. On the
other hand, there are still other "exception" cases that have not been
changed because the way to do so is reasonably ugly, and not changing
percieved bad behaviour is not objectionable.

A lot of things in that paragraph seem contradictory. Can it be reworded?

justinmk · 2017-05-13T19:44:53Z

src/nvim/getchar.c

@@ -1465,9 +1465,10 @@ int vgetc(void)
       * converted character.
       * Note: This will loop until enough bytes are received!
       */
-      if (has_mbyte && (n = MB_BYTE2LEN_CHECK(c)) > 1) {
+      buf[0] = (char_u)c;
+      i = 1;


this is an odd place to put this, because it's done again on line 1472. Perhaps this should go in a else {} block?

justinmk · 2017-05-13T19:57:40Z

test/functional/normal/langmap_spec.lua

+    -- This is a reasonably easy fix, but it doesn't make sense to fix this and
+    -- not translating text recorded in the macro when mappings are not
+    -- applied.
+    feed_command('nnoremap w l')


use command in most cases, unless the asynchrony of feed_command is needed.

hardenedapple · 2017-05-13T20:02:30Z

Just as a caveat -- at the time I made this PR [RFC] it was to get feedback on whether the functional changes I'd made were good or not, not because I thought the commits were ready to merge.

Yeah I apologise about that commit message, what I was trying to say was:

At the moment (before that commit) there are 7 exceptions (that I know of) where vim takes a character to apply as a command, yet langmap is not applied I personally think they shouldn't exist.
After that commit, 2 of those exceptions are not exceptions any more, I could make them exceptions again, but that requires adding a few lines of code.
On the other hand, in that commit I don't add code to force the functional change on the other 5 exceptions because I believe that adding code requires more justification than not adding it.

A less kind-to-me way of putting it is:

I'm lazy and like the changes in functionality this introduces so I'm not going to bother adding code to get rid of it. On the other hand, I don't think I can justify changing all the other cases just because I don't like them in what claims to be a bug-fix / code-neaten PR.

hardenedapple · 2017-05-13T20:28:44Z

To note (thought this might be unclear just from the commits).

c8e8196 is an independent bug-fix commit that doesn't need any of the others. It's this that I referred to in the gitter room.
ccf7e5d makes it so that a map triggered while recording a macro is triggered when replaying it (i.e. applies langmap to the characters recorded when those characters trigger a map). It doesn't make sense to have this but not 5414960
5414960 does the same fix as ccf7e5d for recording unmapped keys in a macro (so that replaying a macro does the same as what happened while recording it), but it introduces the other functionality changes I'm unsure about.

hardenedapple · 2017-05-14T17:04:49Z

I just noticed a bug while recording macros under this PR and wanted to mention it so the bug doesn't get merged.

The bug can be seen using a special key like <BS>.

Starting with neovim -u NONE and pressing the following keys

qaohellot<BS><esc>q@a

you see that the macro doesn't replay the same actions that happened while it was being recorded.
I expect this is because recording is now done after unescaping special keys instead of before.

I'll get round to fixing it, but in a while ...

I still believe the commit changing the definition of the LANGMAP_ADJUST is good (it's independent of the others).

justinmk · 2018-01-21T17:49:26Z

@hardenedapple Which commits here are ready to merge?

hardenedapple · 2018-01-21T19:00:33Z

@justinmk I'll have a look at this over the next week (probably next weekend).

There is one commit that fixes a simple bug, while the other commits are around
getting 'langmap' to behave with macros in the same way that #5658
was trying to get :lmap to behave.

From what I remember, this PR worked fine, but changing 'langmap' without
changing :lmap seemed inconsistent, and hence I didn't push anything through.

I'll need to find those parts of the tests I introduced to check the macro
behaviour and split them out from the tests checking the current behaviour of
'langmap'.

Once I've done that I'll create a separate PR containing the tests for the
current behaviour (including some known bugs) and the commit fixing the plain
bug.

hardenedapple · 2018-01-27T13:16:44Z

I've split the PR into two.
PR #7919 is the bug fix, this PR now only contains the behaviour to do with macro recording in a similar way to #5658 .

This PR is some commits on top of the ones in #7919
This PR has merge conflicts with #5658 they're ones I've resolved quite a few times locally.

hardenedapple · 2018-01-28T10:04:25Z

After the merge of #7919 this has been rebased onto master.

We put LANGMAP_ADJUST() in vgetc() instead of vgetorpeekc(), because vgetc() is the first place that vim switches from handling bytes to characters. The only place vgetorpeek() is called outside of vgetc() is vpeekc() Each of the calling places of this is written below, along with the reason that not applying LANGMAP in that case is fine. insertchar() (in INSERT mode) command_line_execute() (in CMDLINE mode) command_line_changed() (in CMDLINE mode) getexline() (used when executing from register) openscript() (return value is not used) vpeekc_any() which is used in ins_compl_check_keys() (in INSERT mode) f_getchar() (checked against empty (i.e. LANGMAP doesn't matter) command_line_changed() (check for empty) char_avail() (checked against empty) normal_get_additional_char() (Check for empty, Check for empty, Check for multibyte to determine whether need to read more, any more is read with plain_vgetc()) do_mouse() (Check for empty) Before this commit, there are 7 exceptions to the rule that 'langmap' is applied whenever neovim takes a key to be interpreted as a command. These exceptions are mentioned in the test spec. This commit changes two of these so they are no longer exceptions, these are: confirming whether to apply a substitution after :s///c, and asking y/n after a backwards range. I'm leaving the changes as I believe they are improvements. On the other hand, I'm not changing the behaviour of the other exceptions despite being of the opinion that they should change, as the point of this commit is to record the 'langmap' translation in macros, not to change when the 'langmap' translation is applied.

justinmk · 2019-01-03T00:10:15Z

@hardenedapple Now would be a good time to merge this. But users won't know about it unless we describe it. Can you suggest a documentation update, or at least a brief description of the behavior changes?

roket1428 · 2019-12-29T01:56:51Z

Tested, it fixes macro behaviors of langmap. Sweet!

hardenedapple · 2020-01-18T12:47:06Z

@justinmk I've just merged with master and re-run the tests.

One thing that surprised me:

If you remember from my comment above #6494 (comment) , there were some exceptions to the general rule of "commands have LANGMAP applied" that I recorded in the testsuite.
One of those exceptions is not an exception any more.

Frankly I don't understand the previous behaviour (haven't spent enough time investigating since I'm not worried about it) but the current behaviour makes sense.

Overall, the PR doesn't seem to have bitrotted too much.

Documentation changes?

In regards to documentation changes: The idea is that this series makes things work as one would expect from the documentation as it is. The differences are:

Macros behave properly after langmap translation.
Some places where one would expect a LANGMAP translation but there isn't one now have that translation applied.

Apologies for going dark -- I got more busy ;-)

amerlyq · 2022-04-24T16:27:33Z

I still can't understand, does this PR fixes the vim/vim#3018 bug, and specifically vim/vim#7458 bug of not supporting multibyte chars in "from" ?

hardenedapple force-pushed the langmap-things branch from 591293a to 41083a9 Compare April 10, 2017 14:28

marvim added the WIP label Apr 10, 2017

hardenedapple force-pushed the langmap-things branch from 41083a9 to ac0ddd2 Compare April 11, 2017 14:42

hardenedapple changed the title ~~[WIP] Langmap things~~ [RFC] Langmap things Apr 11, 2017

marvim added RFC and removed WIP labels Apr 11, 2017

hardenedapple force-pushed the langmap-things branch from ac0ddd2 to fb59a81 Compare May 13, 2017 19:19

justinmk reviewed May 13, 2017

View reviewed changes

justinmk added this to the 0.2.1 milestone May 14, 2017

hardenedapple force-pushed the langmap-things branch from 4bcafd5 to 7ea83a2 Compare June 2, 2017 20:32

justinmk modified the milestones: 0.2.1, 0.2.2 Sep 16, 2017

hardenedapple mentioned this pull request Jan 27, 2018

Langremap after map #7919

Merged

hardenedapple force-pushed the langmap-things branch from 7ea83a2 to f76efd8 Compare January 27, 2018 12:37

hardenedapple force-pushed the langmap-things branch from f76efd8 to bf6ddc3 Compare January 27, 2018 15:32

hardenedapple force-pushed the langmap-things branch from bf6ddc3 to 5289a02 Compare January 31, 2018 08:44

hardenedapple force-pushed the langmap-things branch from 5289a02 to 502f171 Compare February 10, 2018 17:43

hardenedapple changed the title ~~[RFC] Langmap things~~ [RDY] Langmap things Feb 11, 2018

marvim removed the RFC label Feb 11, 2018

marvim added the RDY label Feb 11, 2018

hardenedapple force-pushed the langmap-things branch from 502f171 to b1cde5a Compare March 14, 2018 11:20

hardenedapple added 4 commits May 17, 2018 08:05

Add tests for macro behaviour w.r.t 'langmap'

a0c8391

Record subsequently mapped langmap translations in macros

66cc813

Account for special keys in recording

51fbd52

hardenedapple force-pushed the langmap-things branch from b1cde5a to 51fbd52 Compare May 17, 2018 07:33

justinmk modified the milestones: 0.3.1, 0.3.2 Jun 10, 2018

justinmk mentioned this pull request Dec 26, 2019

Setting langmap doesn't handle macros. #11614

Open

justinmk mentioned this pull request Jan 13, 2020

Ensure we do undo sync when emulating <Esc><some_key> #11706

Merged

hardenedapple added 2 commits January 18, 2020 12:24

Merge branch 'master' into langmap-things

2c2dd81

Update test, it is no longer an exception.

8d9f7be

maisiliym mentioned this pull request Dec 15, 2020

Langmap-things rebase #13536

Closed

janlazo modified the milestones: 0.5, 0.5.1 Feb 14, 2021

bfredl modified the milestones: 0.5.1, 0.6 Aug 15, 2021

bfredl modified the milestones: 0.6, 0.7 Oct 30, 2021

clason removed this from the 0.7 milestone Feb 24, 2022

justinmk removed the status:needs-approval label Aug 22, 2022

zeertzjq added the input label Mar 20, 2023

dundargoc changed the title ~~[RDY] Langmap things~~ Langmap things Apr 2, 2023

Uh oh!

Langmap things #6494

Are you sure you want to change the base?

Langmap things #6494

Uh oh!

Conversation

hardenedapple commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk May 13, 2017

Choose a reason for hiding this comment

Uh oh!

hardenedapple May 13, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk May 13, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk commented May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk May 13, 2017

Choose a reason for hiding this comment

Uh oh!

justinmk May 13, 2017

Choose a reason for hiding this comment

Uh oh!

hardenedapple commented May 13, 2017

Uh oh!

hardenedapple commented May 13, 2017

Uh oh!

hardenedapple commented May 14, 2017

Uh oh!

justinmk commented Jan 21, 2018

Uh oh!

hardenedapple commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hardenedapple commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hardenedapple commented Jan 28, 2018

Uh oh!

justinmk commented Jan 3, 2019

Uh oh!

roket1428 commented Dec 29, 2019

Uh oh!

hardenedapple commented Jan 18, 2020

One thing that surprised me:

Documentation changes?

Uh oh!

amerlyq commented Apr 24, 2022

Uh oh!

Uh oh!

hardenedapple commented Apr 10, 2017 •

edited

Loading

justinmk commented May 13, 2017 •

edited

Loading

hardenedapple commented Jan 21, 2018 •

edited

Loading

hardenedapple commented Jan 27, 2018 •

edited

Loading