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

KeyEvent::text should be None when Ctrl is pressed? #3038

Open
dhardy opened this issue Aug 22, 2023 · 18 comments
Open

KeyEvent::text should be None when Ctrl is pressed? #3038

dhardy opened this issue Aug 22, 2023 · 18 comments
Labels
D - easy Likely easier than most tasks here S - api Design and usability

Comments

@dhardy
Copy link
Contributor

dhardy commented Aug 22, 2023

TLDR: should Winit force KeyEvent::text to None when:

  • Ctrl / Alt / Super modifier keys are pressed
  • Text is a unicode control chars

such that text is always printable? (We retain text_with_all_modifiers for use by terminals.)


From the doc:

This is None if the current keypress cannot be interpreted as text.

If, for example, I press Ctrl + U I get (Wayland with winit v0.29.1-beta):

KeyboardInput: KeyEvent {
  physical_key: KeyI,
  logical_key: Character("u"),
  text: Some("u"),
  location: Standard,
  state: Pressed,
  repeat: false,
  platform_specific: KeyEventExtra {
    key_without_modifiers: Character("u"),
    text_with_all_modifiers: Some("\u{15}"),
  },
}

My understanding is that the purpose of text is for text entry in an edit field. If someone presses Ctrl + U in an editor, this may or may not match some shortcut but should never enter "u". Therefore, Winit should always pass text: None when the Ctrl modifier is held?

Should this behaviour be extended to other modifiers?

  • Not to Shift or AltGr
  • Yes for "Super" on MacOS (Cmd)
  • Other cases are less clear. Intuitively probably yes. I note that Firefox on Wayland accepts text input (for unmapped modifiers) with the Super key but not the Alt key, while the KDE appears to accept text input with both Super and Alt modifiers (but not Ctrl). The Super key is used for the menu (thus losing app focus) on Windows and sometimes on Linux; it is sometimes used as a modifier for keyboard shortcuts on Linux.
@dhardy
Copy link
Contributor Author

dhardy commented Aug 23, 2023

Also related:

logical_key: Backspace, text: Some("\u{8}"),

This is a control code, not text, so should it also be text: None?

@dhardy
Copy link
Contributor Author

dhardy commented Aug 23, 2023

Sorry, turns out I did open a discussion on this topic already. Doesn't show up when searching issues.

Note: text is None when pressing a dead key in my testing, so that isn't an issue at least. And text_with_all_modifiers is probably not useful except when emulating a terminal.

I've only really been testing on Wayland, but given what Winit is I would expect all platforms to behave similarly (which may mean mapping some codes).

@dhardy
Copy link
Contributor Author

dhardy commented Aug 24, 2023

XKB allows usage of various other keys as second/third/fifth layer selectors. When I try this, these are reported as e.g. ISO_Level5_Shift (by xev), not e.g. Control_R. Hence, at least on Linux, it seems safe to assume that a key reported as Alt/Ctrl/Super is not being used to select a different keyboard layer.

There may, however, be utility in reporting control codes via text, especially since Enter (presumably) has a platform dependent mapping (though this may not be a good thing — e.g. a text editor should probably detect and follow the convention of the rest of the document instead of just inserting whatever line-break the platform uses).

@kchibisov
Copy link
Member

XKB allows usage of various other keys as second/third/fifth layer selectors. When I try this, these are reported as e.g. ISO_Level5_Shift (by xev), not e.g. Control_R. Hence, at least on Linux, it seems safe to assume that a key reported as Alt/Ctrl/Super is not being used to select a different keyboard layer.

Yeah, xkb has a level concept for those Shift, level 3/5 shifts, etc.

TLDR: should Winit force KeyEvent::text to None when:

If OS tells us that it's not like that, I'm not sure that we should, but I know that some apps which can't handle regular control bindings (the \x codes) just input normal text like control wasn't pressed. I just don't see a benefit in removing this... Also, I'm pretty sure you can setup control on linux yourself to report something normal...

This is a control code, not text, so should it also be text: None?

I think we explicitly decided to handle Backspace, Tab, Enter. See

pub fn to_text(&self) -> Option<&str> {
However, maybe it's wrong to do it like that and we shouldn't set text to control codes?

@dhardy
Copy link
Contributor Author

dhardy commented Aug 27, 2023

If OS tells us that it's not like that,

My main concerns are documentation and platform consistency. To be honest, this goes way beyond Winit, e.g. KDE (Qt) apps will block all text entry while Ctrl is held but do not do the same for Alt or Super (which input text if no shortcut is matched). Also portability: Super is used more for shortcuts on MacOS than on other platforms.

I'm tempted to set text = None in KAS (for modifiers other than Shift) regardless of what Winit chooses here just so that widgets receive a consistent event.


Control codes are a separate matter. My current perspective is that e.g. Enter is easier to deal with as a Key code since it may or may not insert a line-break depending on the widget while Tab is usually used for navigation not indentation, etc.

                Event::Key(event, false) if event.state == ElementState::Pressed => {
                    if let Some(text) = event.text {
                        // This is not a control code and no modifier (besides Shift) is pressed, enforced by toolkit
                        self.received_text(cx, data, &text)
                    } else {
                        // handle event.logical_key ...
                    }
                }

https://github.com/kas-gui/kas/blob/f342d7a05d1d55498c78880b37f2dcf1c7690d9e/crates/kas-widgets/src/edit.rs#L709

This of course does not apply universally so Winit may not want to do the same regarding control codes.

Maybe just adding fn KeyEvent::is_printable would be a good alternative (to let consumers easily filter out control codes), though I'd be tempted to make the text field private and use accessor fns instead.

See keyboard.rs:1480

So Key::to_text will report \r while KeyEvent::text is \n on Linux. I guess this is okay (I also don't have a use for to_text, unless that were recommended over KeyEvent::text, but in that case why have both).

@daxpedda daxpedda added the C - needs discussion Direction must be ironed out label Aug 28, 2023
@kchibisov
Copy link
Member

Hm, maybe making text private is not that bad, but it could be a little bit inconvinient for the end users. The term non-printable is a bit vague, however rust already have a https://doc.rust-lang.org/std/primitive.char.html#method.is_control, which sort of solves the issue...

Though, I'd agree that there's no need to have a text with control chars attached to text for consistency reasons, and we could have a special method to map text in such cases?

And from what I can see the w3c stuff doesn't emit any text for the Enter key itself, so if you want to make text None it sounds good to me.

@kchibisov kchibisov added S - api Design and usability D - easy Likely easier than most tasks here and removed C - needs discussion Direction must be ironed out labels Aug 29, 2023
@fredizzimo
Copy link

fredizzimo commented Sep 2, 2023

If text is none, then we need an alternative in Neovide, since it will no longer be possible to map special characters. We don't have access to the keymap so we can't know what character a specific combination produces. In Vim you always map by the resulting character, not the modifiers + base key.

Actually, I think it should produce text or an alternative even in combination with dead keys. For example, on Windows I can have a working mapping for <M-^>, (M stands for alt) where ^ is a dead key, but unfortunately the same mapping with CTRL does not work.

At the very minimum the Vim default <C-^> mapping should work, where ^ is just a shifted number.

@dhardy
Copy link
Contributor Author

dhardy commented Sep 2, 2023

@fredizzimo I think you should be matching logical_key for that? It should work with <C-^> since logical_key is affected by Shift. The mentioned differences for text do not appear useful for shortcuts.

@fredizzimo
Copy link

I guess so, we already use the logical key as a backup if the text is empty. That is actually the case for some, but not all ctrl combinations on Windows for some reason, see #2898

But if we use just the logical key then we lose the ability to map dead key combinations, which is currently possible, at least in combination with other modifiers, like alt on Windows. So, it would be a regression for Neovide. But if logical key also included the resolved character(s), then everything should be fine.

We also still need to deal with the text field for raw typing, those keys are sent exactly the same way to Neovim as the ones that are combined with modifiers. So, if this change is made, for us I think that it would mean to send, the text if it's set, otherwise send logical_key with all the pressed modifiers, excluding altgr and shift, unless it's special like space for example, in which case we need to include shift.

@dhardy
Copy link
Contributor Author

dhardy commented Sep 3, 2023

For example, on Windows I can have a working mapping for <M-^>, (M stands for alt) where ^ is a dead key

I assumed by this that you match the key used to start a dead-key sequence, but from your last reply it sounds more like you want to match the resulting symbol, e.g. ô? I don't think logical_key is supposed to do that... and in general I don't think shortcuts are supposed to work like that (you'd need to press Shift + 6 to start the dead-key sequence then Alt + o to get the shortcut?).

So, if this change is made, for us I think that it would mean to send, the text if it's set, otherwise send logical_key with all the pressed modifiers, excluding altgr and shift, unless it's special like space for example, in which case we need to include shift.

You're talking just text entry here IIUC — so why care about shift+space? Why even care about cases where text == None? The whole point here is that text is just the printable stuff. You are sending text to an interpreter which expects some control-codes? Then which ones? And should Enter automatically map to \n?

@fredizzimo
Copy link

I assumed by this that you match the key used to start a dead-key sequence, but from your last reply it sounds more like you want to match the resulting symbol, e.g. ô? I don't think logical_key is supposed to do that... and in general I don't think shortcuts are supposed to work like that (you'd need to press Shift + 6 to start the dead-key sequence then Alt + o to get the shortcut?).

In (neo)vim, there's not really any concept of shortcuts, we are talking about mappings. To borrow from the documentation:

Key mapping is used to change the meaning of typed keys. The most common use
is to define a sequence of commands for a function key. Example:

:map a=strftime("%c")

This appends the current date and time after the cursor (in <> notation |<>|).

In those terms it's entirely possible to map :nmap <ô> :echo "hello"<cr>, and mapping with modifiers should be consistent, so with alt for example it becomes :nmap <M-ô> :echo "world"<cr> and not, :nmap <C-^>o :echo "world"<cr>, which additionally needs the variation :nmap <C-^><C-o> :echo "world"<cr> to be ergonomic to use.

There's also another inconsistency, if we do it that way. The default mapping ` {a-z}, which jumps to a mark with the exact column and line, requires pressing ``space+```, on my keyboard, where it's a dead key, also in the terminal. So that mapping has to work the same way. In fact, doing anything else, would totally destroy the normal typing of dead characters as explained next.

You're talking just text entry here IIUC — so why care about shift+space?

The comment was a bit off-topic to the discussion, but we do need to handle that in Neovide.

In (neo)vim, there's no difference between text entry and mappings, everything it sees is just a sequence of characters, you could even do :imap ô to insert a fancy unicode character instead ô as you type. Or perhaps you want to use the alt combination from before for that. In Neovide all we do is to call nvim_input with a sequence of characters, including the modifiers.

Mapping shift+space, shift+enter, shift+F1 and so on are very useful mappings, since the keys are easy to hit. But we don't want to send <S-A>, when it's supposed to be just A, <S-a> is also not correct.

Why even care about cases where text == None?

Using just logical_key would make text entry with dead keys impossible. So, we need to primarily deal with the case where `text' is available, which in our preference would be always, but I do also understand that for other use cases it isn't.

@kchibisov
Copy link
Member

If text is none, then we need an alternative in Neovide, since it will no longer be possible to map special characters. We don't have access to the keymap so we can't know what character a specific combination produces. In Vim you always map by the resulting character, not the modifiers + base key.

The issue was purely about the text but text_with_all_modifiers should have the thing you need, because it's basically what nvim sort of wants from what I understood. There could also be an option to get the actual text out of key, where we basically map Enter to e.g. \r (we have such a thing now for example).

@fredizzimo
Copy link

text_with_all_modifiers is also not suitable, since some of those control codes have duplicate meanings. See neovide/neovide#1899 (comment) for example.

@kchibisov
Copy link
Member

@fredizzimo besides all of that, can't you communicate with neovim by the means of the kitty's keyboard protocol?

@kchibisov
Copy link
Member

text_with_all_modifiers is also not suitable, since some of those control codes have duplicate meanings. See neovide/neovide#1899 (comment) for example.

I see, I think the issue is that we want shift levels exposed somehow, but I think it's more for a future work with keyboard stuff. I also needed that sort of thing, but resorted to use to_lower with the logical_key in alacritty...

@fredizzimo
Copy link

I don’t think that a neovim gui application can use the Kitty protocol. And we have nvim_input, which is perfectly capable of repeesenting any possible key combination, and even imaginary ones, since it’s all just text,you can send mappable custom keys.

Apart from a few bugs that I have reported here in the issue tracker, we do have everything we need from winit for the keyboard support right now. But if text, is removed in combination with modifiers, we loose proper deadkey support. I can see three alternatives.

  1. Make setting the text to none an option that can be toggled off
  2. Provide yet another field
  3. Include the resolved dead key string in virtual_text

@kchibisov
Copy link
Member

Hm, this issue was mostly about non-printable text from what I can say and whether it should be reported. Honestly, I'm not even sure what clients really want from text, and compare it with text_with_all_modifiers, I know that control + a is reported via the text_with_all_modifiers with a text being Some("a"). Maybe it's fine the way it is now, I just compared with web stuff and on web they remove the text when you have control pressed.

@dhardy
Copy link
Contributor Author

dhardy commented Sep 4, 2023

At this point my thoughts are:

  • The platform-specific text_with_all_modifiers and key_without_modifiers are not particularly useful (I've yet to be convinced about either, though maybe text_with_all_modifiers has uses for terminal emulators)
  • It seems like neovim really does need text as it is now
  • I think most users would be better off with a printable_text variant which is None on control chars and when at least the Ctrl modifier is pressed. Given that this is easy to calculate from the rest of KeyEvent, it can just be a method: fn KeyEvent::printable_text(&self) -> Option<&str>.

There is one reason to expose text as a field instead of as a method: to allow deconstruction / taking the value. I suspect this won't often be useful, but don't see a reason to prevent it.

Possibly text isn't the right name when it may contain control chars so it could be renamed but I don't have a good suggestion... possibly raw_text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D - easy Likely easier than most tasks here S - api Design and usability
Development

No branches or pull requests

4 participants