[vim/vim] Don't set seenModifyOtherKeys on meta and some special keys (PR #11413)
Trygve Aaberge
The kitty terminal emulator sends CSI u keys with the meta modifier and keys in the Unicode Private Use Area by default (encoded it it's own keyboard protocol which is similar, but not identical to modifyOtherKeys) and doesn't support modifyOtherKeys, so don't set seenModifyOtherKeys for those. Setting seenModifyOtherKeys means that vim don't use mappings for control characters, which kitty still use for Ctrl-key presses, so those would stop working.
Since the Meta modifier is not present on standard keyboards anymore, I think it's very rarely used, so therefore I don't see a problem with not setting seenModifyOtherKeys for it. It's very likely that Shift, Control or Alt would be used before Meta, so then seenModifyOtherKeys would be set by those.
Keys in the Unicode Private Use Area is not used by modifyOtherKeys as far as I can see, so not setting seenModifyOtherKeys for those should not affect terminals supporting modifyOtherKeys, only terminals supporting the kitty keyboard protocol.
Fixes #11403 in kitty. That issue still persists in the foot terminal emulator though, but that's caused by a different problem so it needs a different solution.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/11413
Commit Summary
- 9a5a5fb Don't set seenModifyOtherKeys on meta and some special keys
File Changes
(1 file)
- M src/term.c (7)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #11413 (9a5a5fb) into master (d5337ef) will decrease coverage by
61.66%.
The diff coverage is0.00%.
@@ Coverage Diff @@ ## master #11413 +/- ## =========================================== - Coverage 61.96% 0.29% -61.67% =========================================== Files 162 152 -10 Lines 186329 175376 -10953 Branches 42878 40345 -2533 =========================================== - Hits 115456 520 -114936 - Misses 58905 174799 +115894 + Partials 11968 57 -11911
| Flag | Coverage Δ | |
|---|---|---|
| huge-gcc-unittests | 0.29% <0.00%> (ø) |
|
| linux | 0.29% <0.00%> (ø) |
|
| mingw-x64-HUGE | ? |
|
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/term.c | 0.00% <0.00%> (-31.49%) |
⬇️ |
| src/float.c | 0.00% <0.00%> (-95.64%) |
⬇️ |
| src/sha256.c | 0.00% <0.00%> (-91.88%) |
⬇️ |
| src/crypt_zip.c | 0.00% <0.00%> (-85.30%) |
⬇️ |
| src/testing.c | 0.00% <0.00%> (-84.58%) |
⬇️ |
| src/typval.c | 0.00% <0.00%> (-82.24%) |
⬇️ |
| src/if_lua.c | 0.00% <0.00%> (-80.94%) |
⬇️ |
| src/eval.c | 0.00% <0.00%> (-79.91%) |
⬇️ |
| src/digraph.c | 0.00% <0.00%> (-79.67%) |
⬇️ |
| src/vim9script.c | 0.00% <0.00%> (-79.33%) |
⬇️ |
| ... and 134 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> The kitty terminal emulator sends CSI u keys with the meta modifier
> and keys in the Unicode Private Use Area by default (encoded it it's
> own keyboard protocol which is similar, but not identical to
> modifyOtherKeys) and doesn't support modifyOtherKeys, so don't set
> seenModifyOtherKeys for those. Setting seenModifyOtherKeys means that
> vim don't use mappings for control characters, which kitty still use
> for Ctrl-key presses, so those would stop working.
>
> Since the Meta modifier is not present on standard keyboards anymore,
> I think it's very rarely used, so therefore I don't see a problem with
> not setting seenModifyOtherKeys for it. It's very likely that Shift,
> Control or Alt would be used before Meta, so then seenModifyOtherKeys
> would be set by those.
>
> Keys in the Unicode Private Use Area is not used by modifyOtherKeys as
> far as I can see, so not setting seenModifyOtherKeys for those should
> not affect terminals supporting modifyOtherKeys, only terminals
> supporting the kitty keyboard protocol.
>
> Fixes #11403 in kitty. That issue still persists in the foot terminal
> emulator though, but that's caused by a different problem so it needs
> a different solution.
terminal emulators (e.g. to represent the Cmd key on Mac), just to get
it to change what a key does. I have no idea who would use the Unicode
Private Use area, this looks a bit too much like guessing.
How about checking $TERM for containinng "kitty"? Since kitty doesn't
support modifyOtherKeys (and it's very unlikely it will), that is a more
direct way. It appears the default value for $TERM is "xterm-kitty",
thus that should work.
--
Mynd you, m00se bites Kan be pretty nasti ...
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Trygve Aaberge
@trygveaa pushed 1 commit.
- 1628c6c Only set seenModifyOtherKeys when it's safe
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Trygve Aaberge
Hmm, this looks a bit arbitrary. The Meta modifier may be used by some terminal emulators (e.g. to represent the Cmd key on Mac), just to get it to change what a key does. I have no idea who would use the Unicode Private Use area, this looks a bit too much like guessing. How about checking $TERM for containinng "kitty"? Since kitty doesn't support modifyOtherKeys (and it's very unlikely it will), that is a more direct way. It appears the default value for $TERM is "xterm-kitty", thus that should work.
I see your point. I hadn't thought about Mac using Meta to represent Cmd for instance. Usually I don't like checking TERM because it won't help if other terminals does the same, but I agree that in this case it makes sense since the issue is very specific.
I have updated the commit to check TERM against xterm-kitty. I also added a check for if the control sequence to enable modifyOtherKeys or the kitty keyboard protocol has been sent and only set seenModifyOtherKeys if it has. By doing this, the issue is also fixed in foot. I have updated the commit message with a description of this new behavior. I will also update the PR description with this message.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> > Hmm, this looks a bit arbitrary. The Meta modifier may be used by
> > some terminal emulators (e.g. to represent the Cmd key on Mac), just
> > to get it to change what a key does. I have no idea who would use
> > the Unicode Private Use area, this looks a bit too much like
> > guessing. How about checking $TERM for containinng "kitty"? Since
> > kitty doesn't support modifyOtherKeys (and it's very unlikely it
> > will), that is a more direct way. It appears the default value for
> > $TERM is "xterm-kitty", thus that should work.
>
> I see your point. I hadn't thought about Mac using Meta to represent
> Cmd for instance. Usually I don't like checking TERM because it won't
> help if other terminals does the same, but I agree that in this case
> it makes sense since the issue is very specific.
>
> I have updated the commit to check TERM against xterm-kitty. I also
> added a check for if the control sequence to enable modifyOtherKeys or
> the kitty keyboard protocol has been sent and only set
> `seenModifyOtherKeys` if it has. By doing this, the issue is also
> fixed in foot. I have updated the commit message with a description of
> this new behavior. I will also update the PR description with this
> message.
enable the protocol, but that doesn't mean it works. We know that the
modifyOtherKeys enabling will not work on terminals that have "xterm" in
the name but are not actually an xterm. This relies on the unknown code
to be ignored. Same for the kitty protocol.
I would actually prefer to detect kitty by the termresponse, but
unfortunately I haven't receive an answer on my question what
termresponse kitty sends. I can only see what it sends for the version
I have, I don't know about older and newer versions.
Perhaps checking $TERM for "\<kitty\>" (that is "kitty" as a word) might
work best for now.
--
Computers make very fast, very accurate, mistakes.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Kovid Goyal
If you mean the response to the secondary device attributes query kitty will always send
1; 4000 + primary kitty version number ; secondary kitty version number
so if the kitty version number is 1.23.45 you will get 1;4001;23
For the current kitty release you will get
1;4000;26 since the kitty version number is 0.26.4
Incidentally the adding of 4000 was put in there precisely because of vim turning on SGR_MOUSE mode only if the number was high enough.
And just touching briefly on our last discussion, in order to get this response, you have to send the terminal the bytes ESC[>c . To query for the kitty keyboard protocol you need to send ESC[?u. Since you are not worried about the former "leaking" on terminals that do not handle it, its a bit, umm extreme, to be worrying about the latter leaking. Both are very simple CSI escape codes. If there is indeed a terminal out there that parses the former and not the latter I would be amazed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> If you mean the response to the secondary device attributes query
> kitty will always send
> 1; 4000 + primary kitty version number ; secondary kitty version number
>
> so if the kitty version number is 1.23.45 you will get 1;4001;23
> For the current kitty release you will get
> 1;4000;26 since the kitty version number is 0.26.4
>
> Incidentally the adding of 4000 was put in there precisely because of
> vim turning on SGR_MOUSE mode only if the number was high enough.
containing "kitty".
> And just touching briefly on our last discussion, in order to get this
> response, you have to send the terminal the bytes ```ESC[>c ```. To
> do not handle it, its a bit, umm extreme, to be worrying about the
> latter leaking. Both are very simple CSI escape codes. If there is
> indeed a terminal out there that parses the former and not the latter
> I would be amazed.
emulators out there that started out as a nice idea, but don't meet all
the requirements. And some are not responsive to bug reports (the Mac
Terminal app is a good example, they seem to only fix security issues,
and even then it takes many weeks). Oh, I actually have some code
commented out for Konsole because t_u8 doesn't work.
Let me make a patch with my best guess, then we can see if it works well
enough or needs something more.
I wonder if there is a way to set a "seenKittyKeys" flag. Not sure it's
even needed, but we do need seenModifyOtherKeys, thus we might need it
to avoid matching the simplified form. Anyway, then users can just
enable the Kitty keyboard protocol (in Vim or in terminfo/termcap) and
it would work without further settings.
--
ARTHUR: The swallow may fly south with the sun, or the house martin or the
plover seek warmer hot lands in winter, yet these are not strangers to
our land.
SOLDIER: Are you suggesting coconuts migrate?
Andrea Pappacoda
As @dnkl has been involved in issue #9014 and foot development I think it is appropriate to tag him, as GitHub PRs do not notify users subscribed to the linked issues
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Trygve Aaberge
Checking the contents of t_TI is not reliable. The code may be there to enable the protocol, but that doesn't mean it works.
Well, yes, it's not perfect. However, when I submitted this patch seenModifyOtherKeys was set unconditionally, which is even less reliable. I think only setting seenModifyOtherKeys when we have at least tried to enable the protocol is a lot better than unconditionally setting it.
I see that you've changed it to not be set for kitty now, but the problem with foot remains without this patch.
It would of course be better to know for sure if modifyOtherKeys 2 is enabled, but from what I've seen in the issue comments, there isn't any way to do this? If there was, we could just use that and we wouldn't need to set seenModifyOtherKeys when a key with a modifier is seen at all as far as I understand.
Even if this is not the perfect solution, I don't think the idea of a perfect solution should get in the way of a partial solution which improves the current situation. Issue #9014 has been open for a year, and this patch addresses it. There is a possibility that there exist a terminal with this problem and that it isn't fixed by this patch. However, this patch fixes it for the two problems we know about, which is with kitty and foot, and it doesn't make the situation worse for any other terminal.
By the way, it would be better to set seenModifyOtherKeys for kitty when the kitty keyboard protocol is enabled, like this patch does, because it would make it possible to bind Ctrl-i and Tab as separate keys if the user has set t_TI to enable the kitty keyboard protocol.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> > Checking the contents of t_TI is not reliable. The code may be there
> > to enable the protocol, but that doesn't mean it works.
>
> Well, yes, it's not perfect. However, when I submitted this patch
> seenModifyOtherKeys was set unconditionally, which is even less
> reliable. I think only setting seenModifyOtherKeys when we have at
> least tried to enable the protocol is a lot better than
> unconditionally setting it.
state a terminal is in, the user can send raw sequences, there are
menus, config files, etc. Believe me, we had quite a few trial and
error solutions before we ended up with the current one. Maybe not
perfect, but definitely better than other solutions.
> I see that you've changed it to not be set for kitty now, but the
> problem with foot remains without this patch.
that Vim existed for many, many years, new terminals should make sure
they work with Vim, not the other way around.
> It would of course be better to know for sure if modifyOtherKeys 2 is
> enabled, but from what I've seen in the issue comments, there isn't
> any way to do this?
version enabled as broken, we can't possibly handle those.
> If there was, we could just use that and we
> wouldn't need to set seenModifyOtherKeys when a key with a modifier is
> seen at all as far as I understand.
ideas to make it work, but nothing that would be good enough to push
forward. I'm also not aware of anybody else having a solution (other
than individual terminal solutions that won't ever be implemented
widely). Unless something is going to happen, we might add an entry in
the list of key codes requested (this works for xterm and has a chance
that others might implement it too). It's a bit of a hack, but might be
the only way out. See the req_more_codes_from_term() and
got_code_from_term() functions.
> Even if this is not the perfect solution, I don't think the idea of a
> perfect solution should get in the way of a partial solution which
> improves the current situation.
> Issue #9014 has been open for a year,
> and this patch addresses it. There is a possibility that there exist a
> terminal with this problem and that it isn't fixed by this patch.
> However, this patch fixes it for the two problems we know about, which
> is with kitty and foot, and it doesn't make the situation worse for
> any other terminal.
side effects. Usually these things seem OK until it's committed and
then people who were not paying attention speak up that it breaks
something for them.
> By the way, it would be better to set seenModifyOtherKeys for kitty
> when the kitty keyboard protocol is enabled, like this patch does,
> because it would make it possible to bind Ctrl-i and Tab as separate
> keys if the user has set t_TI to enable the kitty keyboard protocol.
sends these keys. Why wouldn't it?
--
Q: Why do Norwegians excel at text editing?
A: Their ancestors are Vi-kings
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
I have just submitted patch 9.0.0930 which adds the 'keyprotocol' option.
It's not fully implemented yet, but goes a long way in the right direction.
I have tried to find other ways to detect that the terminal has modifyOtherKeys level 2 enabled,
but it appears there is no such way. The trick in this PR to inspect the t_TI termcap entry for
an escape sequence is unreliable, the terminal may have modifyOtherKeys enabled in another
way, and this check would break things, without providing the user with any way to work around it.
For those terminals that support the kitty keyboard protocol and enable it there should not be
a problem, because requesting the state will trigger a response that they kitty keyboard protocol
is enabled, which would then reset seenModifyOtherKeys (not implemented yet though).
Another change that might help is to set seenModifyOtherKeys only for a sequence that the kitty
protocol does not send: {lead}27;{modifier};{key}~. With xterm it is possible to change to the
other form {lead}{key};{modifier}u, but I doubt it is actually used in practice. And we can tell
users to simply not do that.
I'll await comments on patch 9.0.0930 and make the other changes mentioned. After that please
check if you still see a problem. If so then open a new issue or PR. I'm closing this one now,
since it is outdated.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
Closed #11413.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.