Update to use new upstream unified keyboard branch

[Kethku]

What kind of change does this PR introduce?

• Fix

Did this PR introduce a breaking change?

A breaking change includes anything that breaks backwards compatibility either at compile or run time.
This may break ime support. Not sure, this needs to be tested.

[Kethku]

Note: the ime support seems way better in the new upstream branch, so at some point after this merges, we should pipe the cursor positions to the ime system. Should resolve some long standing bugs.

We also get precommit texts now for ime, so it should also be possible to render in progress texts next under the cursor to fully round out our ime support.

[shaunsingh]

Small fix to the our glutin fork to build properly on macOS, we can/should upstream it though

[shaunsingh]

Cargo.toml also still puts winit at the old new-keyboard-all branch, not sure if that should be updated to v3

[fredizzimo]

I think we need to prioritize getting this fixed and merged. wgpu will need a more recent winit. And so will my changes to the update loop, so that I can read the display refresh rate.

As far as I see, it's failing because of glutin, we should probably update that to the latest as well. We will need some changes to the context creation, but the glutin-winit makes it look almost the same as before.
Edit
Actually #1813, fixes the compilation issues here with Glutin 0.26.0. But in order to also fix Wayland we need a more recent Glutin as done here #1821, but without embedding glutin-winit. That's part of the official glutin, but we probably needs some small modifications to make it use our version of Winit.
Edit2: Acutally it looks like the the new-keyboard-v3 is quite badly broken for key repeats, if you read the comments on #1821. But it looks like that's an easy fix on our side.

[fredizzimo]

@Kethku, @shaunsingh. I updated to the latest Glutin and cherry-picked the relevant changes from here #1821, this fixes the build issues, and additionally fixes the Wayland configuration as descirbed in that PR.

Now, since there hasn't been a response to rust-windowing/winit@639e1d4#r108100537, the question remains, should we fix that locally in our fork, or try to reach out to the winit guys once more.

I would really like to see this moving forward, not only for the reasons I described in my earlier message, but also because switching to a newer winit should close a lot of the bugs that we have currently.

[github-actions]

Test Results

    3 files  ±0      3 suites  ±0   7s ⏱️ ±0s
  75 tests ±0    75 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
225 runs  ±0  225 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit b72807e. ± Comparison against base commit ed58c49.

♻️ This comment has been updated with latest results.

[shaunsingh]

Now, since there hasn't been a response to rust-windowing/winit@639e1d4#r108100537, the question remains, should we fix that locally in our fork, or try to reach out to the winit guys once more.

Feel free to add it into the fork, I feel like it's quite an urgent issue, and can take quite some time to get patches merged upstream

[fredizzimo]

The keyboard reperat fix should now be included

[last-partizan]

I've been using this branch for a few days, and it works great.

Anything else needs to be done before merging?

[fredizzimo]

Nothing from my part, except that perhaps it would make sense to lock the commit id of the new-keyboard-v3 branch in Cargo.toml instead of relying on just the lockfile to point to the right commit. That we can better control when to update to newer versions.

[last-partizan]

Let's do it and merge.

This solves some of the most annoying bugs for me.

[MultisampledNight]

Sidenote that building this branch failed with cargo install or after cargo update is ran, with errors in winit and this warning:

warning: Patch `winit v0.28.1 (https://github.com/neovide/winit?branch=new-keyboard-v3#4d8d82fd)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

I've pushed a commit locking winit to =0.28.1 which fixes this. Seemingly the implicit ^ specifier was not enough and upgraded to 0.28.6 unconditionally (and also fixes a small formatting issue), since this was cargo tree -i winit after cargo update is ran:

winit v0.28.6
├── glutin-winit v0.3.0
│   └── neovide v0.10.4 (/home/multisn8/info/lurk/neovide)
└── neovide v0.10.4 (/home/multisn8/info/lurk/neovide)

[fredizzimo]

We can't merge this. There seems to be a deadlock in the new keyboard handling of winit on Windows. I'm not sure what's wrong, and I'm don't think I have time to debug it today. But we did have a deadlock earlier too that we fixed and notified the upstream about. They did not take our fix as I wrote it and instead they made another fix by their own, so I wonder if they messed it up. But it could of course be some other deadlock as well.

The deadlock is 100% for me, when I use neovide on a windows remote desktop after I minimize the desktop and return.

[fredizzimo]

It's indeed the same bug, or at least very similar, the code calls PeekMessage inside the keyboard event handling which deadlocks due to re-entrancy.

It's either a new regression in the keyboard branch after our previous merge or their fix does not solve the problem ArturKovacs/winit#1

Edit: I reported the bug here rust-windowing/winit#2662 (comment)

[fredizzimo]

It looks like we might be able to switch over to point to the official branch instead. The deadlock appears to be fixed rust-windowing/winit#2662 and the Wayland keyboard repeat might also be fixed.

I have some minor changes that moves this over to that instead of our fork, along with a few minior fixes due to very slight interface changes. Should I update this PR with those changes, our should we use a separate one for that?

I don't think it makes much sense to pull in the changes to our fork and have Neovide point to that. If we encountered bugs that needs to be fixed, we should send PR upstream instead. And if we lock the commit hash along with the branch it should be just as stable as having our own fork.

[last-partizan]

I agree with you, it's better to use official branch.

[fredizzimo]

I updated it to point to a specific commit of the upstream branch. But so far, I have only tested the Windows version myself.

[MultisampledNight]

Tested this in combination with #1870 for the past 25 minutes on NixOS with Sway (Wayland) with an Intel iGPU, and I cannot observe any change in behavior so far, nice!

[fredizzimo]

Later today took a couple of days, but now it's merged :) Let's see what all this fixes and breaks.

[fredizzimo]

I quickly went through all our open issues to see what could potentially be fixed, but I probably missed a lot of issues, so it would be good to do another pass at some point.

It looks like the update itself might not have fixed that many keyboard issues, we still need to do some work especially around the modifier support.

[manfredlotz]

Once I had opened #1236.

My system: EndeavourOS using Xorg

Having build neovide from commit 4b3c234 (May 14) and neovide doesn't react when having started neovide and switched to other app and switching back to neovide. Only moving the mouse makes it work.

Now haven git pulled f412399 (June 14) and rebuilt neovide.

Problem seems to be fixed as when switching to other apps and switching back to neovide then neovide is responsive without any intermediate mouse movement.

So, from my side things seem to be fixed! Thanks a lot for your work.