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

Non Qwerty\En US Input #445

Closed
Tracked by #1061
Kethku opened this issue Jan 11, 2021 · 153 comments · Fixed by #1899
Closed
Tracked by #1061

Non Qwerty\En US Input #445

Kethku opened this issue Jan 11, 2021 · 153 comments · Fixed by #1899
Labels
enhancement New feature or request

Comments

@Kethku
Copy link
Member

Kethku commented Jan 11, 2021

Right now keyboard layouts are handled by manually listing out bindings given key presses and modifiers. This is very broken.

Luckily there is movement here which should fix this issue. rust-windowing/winit#1788

Once this lands and looks stable enough. I'm going to pull in some version of winit which uses it, and rewrite our keyboard input handling to use that. Most of the hard work was already done thanks to @j4qfrost, so now its a waiting game. Looks like its moving very quickly though so I'm hopeful we will have motion on this soon.

For now I'm going to comb through our issues looking for any that have to do with keyboard input handling and close them as this should fix them or at least make a fix possible.

@pickfire
Copy link

Note that IME support is different from keyboard layout, IME uses an external program unlike using a different layout/variant.

@Kethku
Copy link
Member Author

Kethku commented Jan 11, 2021

Agreed. However IME support in sdl2 is not good either. I think at this point supporting multiple windowing systems is not the way to go, so I'm going to be moving to switch to winit only once the upstream changes land. At that point, we can work with them to get IME support working for real on the relevant platforms.

@Kethku
Copy link
Member Author

Kethku commented Mar 24, 2021

#506 a PR has been contributed targetting the new winit api. It currently only works on windows, but once upstream support for linux and mac lands, we should be in very good shape.

@Kethku
Copy link
Member Author

Kethku commented Mar 24, 2021

rust-windowing/winit#1890 looks like an upstream macos PR has been created. Very excited

@p00f
Copy link
Contributor

p00f commented Apr 7, 2021

I've set my window manager to swap escape and caps lock but it doesn't work in neovide (they are not swapped), will this fix my issue too 👉👈 ?

@pickfire
Copy link

pickfire commented Apr 7, 2021

I've set my window manager to swap escape and caps lock but it doesn't work in neovide (they are not swapped), will this fix my issue too point_rightpoint_left ?

I did the same but IIRC I don't have this issue, I only have an issue with dvorak keys when used with other combination keys, I even swap my left ctrl and left alt but I think I don't have an issue with that either. Maybe I did but I don't recall.

@Kethku
Copy link
Member Author

Kethku commented Apr 7, 2021

I believe this would fix both of your problems @p00f @pickfire. At the very least it would unblock addressing those issues.

@Kethku
Copy link
Member Author

Kethku commented May 7, 2021

rust-windowing/winit#1788 PR one of 3 has merged! Shouldn't be long now!!!

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

German keyboard layout, macOS. (Dead keys is very platform dependent; we had to jump through a lot of hoops to get this working way back when.)

@fredizzimo
Copy link
Member

We need to determine if it's as winit bug or Neovide bug then. So someone on MacOs has to debug it and figure it out.

Neovide in itself does not have any platform specific code as far as I can see https://github.com/neovide/neovide/blob/main/src/window/keyboard_manager.rs

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

#767 added it.

Neovide in itself does not have any platform specific code as far as I can see

Oh my yes it does:

#[cfg(not(target_os = "macos"))]
fn use_alt(alt: bool) -> bool {
alt
}
// The option or alt key is used on Macos for character set changes
// and does not operate the same as other systems.
#[cfg(target_os = "macos")]
fn use_alt(alt: bool) -> bool {
let settings = SETTINGS.get::<KeyboardSettings>();
settings.macos_alt_is_meta && alt
}
#[cfg(not(target_os = "macos"))]
fn key_event_text(key_event: &KeyEvent) -> Option<String> {
key_event
.key_without_modifiers()
.to_text()
.map(|text| text.to_string())
}
#[cfg(target_os = "macos")]
fn key_event_text(key_event: &KeyEvent) -> Option<String> {
let settings = SETTINGS.get::<KeyboardSettings>();
if settings.macos_alt_is_meta {
key_event.text.as_ref().map(|text| text.to_string())
} else {
key_event
.text_with_all_modifiers()
.map(|text| text.to_string())
}
}

If you give me pointers on what to check, I will do so. (I haven't followed recent development of Neovide, I have to admit...)

@fredizzimo
Copy link
Member

Yes, but that platform specific code is not for the dead keys, just for the alt/meta key swapping.

The dead key handling was later refactored to the platform generic way it is now, and using the winit provided key::Dead instead of doing our own handling and that has been the case for a couple of years now.

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

Yes, but that platform specific code is not for the dead keys, just for the alt/meta key swapping.

No, the modifiers bit is relevant here as well (do look at the PR I linked).

The dead key handling was later refactored to the platform generic way it is now, and using the winit provided key::Dead instead of doing our own handling and that has been the case for a couple of years now.

That is correct, but the last commit regressed on that. Looks like I will have to dive into the diff for that ...

@fredizzimo
Copy link
Member

fredizzimo commented Jun 15, 2023

Ok, I looked a bit closer, on MacOS we call text_with_all_modifiers and on other platforms key_without_modifiers. My guess is that winit has fixed that platform difference, and now we should call key_without_modifiers on all platforms.

Indeed only key_without_modifiers is supposed to report dead keys https://github.com/rust-windowing/winit/blob/6300cf915e21089481398c96cc38808764de0a9c/src/platform/modifier_supplement.rs#L21

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

No, that's not it (and it's also needed to handle alt-modified keys correctly, like '~`, which is option-n for me).

I suspect the map to_string is stripping stuff out.

@fredizzimo
Copy link
Member

I suspect the map to_string is stripping stuff out.

That would be very weird, since we do that on all platforms, and it's not stripping anything out on those. And it just converts Option<&str> to Option<String>

The conversion to string is needed because text_with_all_modifiers() returns Option<&str> while KeyEvent.text is Option<SmolStr>

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

Let's move this to #1896

@fredizzimo
Copy link
Member

@clason, no let's not combine the issues. The compose problem is entirely different, and affects Linux. While the dead key issue is MacOS specific. But perhaps we should create a new issue for the dead key support on MacOS and close this one.

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

I'm not fully convinced that these are completely different issues, but if you prefer we can do this.

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

There's of course the very real (and even more likely) possibility that the issue is with upstream winit rather than any change here.

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

@fredizzimo
Copy link
Member

That one was reverted here rust-windowing/winit#2119

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

Sorry, I missed that this got merged in the main repo. As I wrote, I have not been following Neovide and upstream lately, so I should also refrain from needless speculation.

TL;DR: I can only report that things are broken and am happy to test whatever you tell me to test, but will shut up otherwise.

@fredizzimo
Copy link
Member

fredizzimo commented Jun 15, 2023

You could try this commit 2a82eae

That's a slightly older version of the keyboard branch, still pointing to our mirror. That's also one commit before the map
functions were added by us and before this rather big winit refactoring rust-windowing/winit#2817

Edit1:
You can also test alacritty with the winit master https://github.com/kchibisov/alacritty/tree/winit-update. And if that works, I guess we just have to compare our implementation to theirs.

Edit2:
Based on their implementation it looks like we could greatly simplify our code and always use text_with_all_modifiers and let it handle all the deadkey and IME stuff internally.

https://github.com/kchibisov/alacritty/blob/576e531d98cbf3a6b20c4b27f7ba9c83bc047fdb/alacritty/src/input.rs#L969

@clason
Copy link
Collaborator

clason commented Jun 15, 2023

You could try this commit 2a82eae

Same issue, unfortunately.

You can also test alacritty with the winit master https://github.com/kchibisov/alacritty/tree/winit-update. And if that works, I guess we just have to compare our implementation to theirs.

That handles dead/composing keys correctly.

@fredizzimo
Copy link
Member

That handles dead/composing keys correctly.

Great. I prepare a PR that simplifies our input to something very similar. Not sure when I will have time do do it, but probably during the weekend.

If someone else wants to do it, notify me before you start and I will do the same here. That way we don't duplicate the work.

@fredizzimo
Copy link
Member

I started looking at using the API the same way as Alacritty now. It looks promising, the dead keys works at least on my system without any kind of special handling.

The biggest thing I'm worried about though is the alt/meta swap on MacOS, since I don't have a Mac and the Alacritty branch has not implemented that yet, so I don't have any reference to look at.

The Alacritty branch do have IME support, and I think I should be able to test that, so I should probably be able to get that working.

I will also look at some of our mapping problems for example ctrl+number, if that's easily fixable now.

@clason
Copy link
Collaborator

clason commented Jun 16, 2023

The big thing (for me) related to that is third-layer characters like \ -- which seemed to work on alacrity.

Happy to test things in any case.

@fredizzimo
Copy link
Member

I created the very first draft version #1899.

I have personally only tested it on Wayland, but as far as I can see the dead keys and regular mappings work, even the ctrl-number ones that previously didn't.

I haven't attempted to implement neovide_input_macos_alt_is_meta and neovide_input_use_logo yet.

IME input is also missing.

Despite that I'm happy for all the testing I can get, especially on other platforms. I can only test Wayland, X11 and Windows myself, and working on Wayland currently so testing the other takes a bit of effort.

unclechu added a commit to unclechu/nixos-config that referenced this issue Jun 17, 2023
@unclechu
Copy link

My issue was in 2021:

When I try to type for instance $ on a Finnish layout (which is AltGr+4) it does something weird (goes to Normal mode).

I tested latest HEAD f412399 (see my Nix config for it unclechu/nixos-config@a65b445 ) and it doesn’t seem to reproduce anymore.

@fredizzimo
Copy link
Member

I marked #1899 as ready now. It should fix all the issues and also partially enable IME, which will properly be fixed in a later PR.

@unclechu
Copy link

I found out that <A-...> mappings are still prone to this kind of issue. Created a separate ticket for this: #1901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.