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

Add support for QEMU LED State pseudo encoding #1795

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

otthou
Copy link
Contributor

@otthou otthou commented Jul 27, 2023

QEMU supports using a Pseudo Encoding to send keyboard LED status to the VNC client:
https://www.qemu.org/docs/master/interop/vnc-ledstate-pseudo-encoding.html

This complements the QEMU extended key event no-vnc already supports. When you have a VNC server like QEMU that uses the extended key events to send the scan codes of the pressed keys, instead of the letter that is typed, the remote state of the keyboards CapsLock, NumLock and ScrollLock matters, instead of the local state.

This pull request just exposes these events, if supported by the VNC server, via an event. This way, it is possible for an integrator of no-vnc to show the remote keyboard led states in the client.

@CendioOssman
Copy link
Member

Unfortunately, this extension is about more than just showing the state of those LEDs. The server also expects the client to be able to synchronize the state if it supports this extension. Something we cannot do, as the browsers will not let us change the state.

Are you seeing issues with QEMU right now? Because last I checked, it is supposed to try to do this synchronisation on behalf of the client if the LED extension is missing. So things are likely to get worse if we include partial support.

@otthou
Copy link
Contributor Author

otthou commented Aug 21, 2023

You are right, just blindly enabling this option would make things worse in some cases, so this (partial) support should be explicitly enabled on the client side, if we keep it just as a notification like this.

I looked into the QEMU code for this synchronization, (https://github.com/qemu/qemu/blob/master/ui/vnc.c#L1911), and it looks this synchronization is indeed supported, but only (in case of caps-lock) for keys that are letters a-z. So it doesn't work if the VNC client keyboard layout uses non-latin letters. It makes sense that it can't do that for all keyboard layouts, since it doesn't actually know the clients caps-lock state, only its own. So the best QEMU can do is guess it based on the shift state and the send sym.

And yes, we cannot actually toggle the local led state, due to javascript restrictions, but by supporting this extension we can actually do better than QEMU can, since we actually do know the led state on both sides with it. So apart from showing the remote led state, we can also send a capslock/numlock key if needed to make the remote state match our state, before sending a keypress. This is basically the same fixing behavior that QEMU tries to do if we don't support this extension, but with actually knowing the led state on both sides, it would work in all cases, not just the case where the clients keyboard layout only uses the latin letters.

@CendioOssman
Copy link
Member

Yeah, I suppose we could try doing QEMU's fallback in noVNC instead. Even if it isn't the ideal, it should hopefully not be worse than the current state of things.

Except if there is a network issue, I suppose. That's something that QEMU doesn't have to deal with. There might be an issue if we try to send fake CapsLock multiple times just because we haven't got an updated LED state back yet. So some care might be needed.

Could you update the patch with a suggestion for such logic?

@otthou
Copy link
Contributor Author

otthou commented Aug 22, 2023

Ok, I've updated the patch

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, thanks!

I don't suppose the logic can be moved to rfb.js? So far we've kept keyboard.js rather generic and it would be nice to keep that.

core/input/keyboard.js Outdated Show resolved Hide resolved
Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work well during some quick testing here. Nice work! :)

Hopefully, there aren't too many corner cases, but I guess we'll have to roll this out in the wild to find out. :)

this._sendKeyEvent(KeyTable.XK_Caps_Lock, 'CapsLock', true);
this._sendKeyEvent(KeyTable.XK_Caps_Lock, 'CapsLock', false);
this._sendKeyEvent(KeyTable.XK_Caps_Lock, 'CapsLock', true, null, null);
this._sendKeyEvent(KeyTable.XK_Caps_Lock, 'CapsLock', false, null, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider using undefined instead so that callers can just omit the lock arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make null the default value for those parameters so that they can be omitted yes. I prefer that over using undefined directly, since it is more explicitly optional this way.

// since it may take some time to receive the new remote CapsLock state.
if (code == 'CapsLock' && down) {
this._remoteCapsLock = null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice safety logic. :)

core/rfb.js Outdated
this._remoteCapsLock = null;
}
if (this._remoteCapsLock !== null && capslock !== null && this._remoteCapsLock !== capslock && down) {
Log.Warn("Fixing remote caps lock");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These syncs should be somewhat common, and normal. So I think a debug log level is more appropriate. We don't want to scare the users with something expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, I'll change it to debug.

core/rfb.js Outdated

this.dispatchEvent(new CustomEvent(
"ledstatus",
{ detail: { scrollLock: scrollLock, numLock: numLock, capsLock: capsLock } }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer directly related to what we're trying to do, so please leave this new event out. It's a change of the API, which we want to be more careful about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, this event is no longer that useful. I'll remove it.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, thanks. I just have a final request if you could see if you could get the new tests a bit more robust.

tests/test.rfb.js Outdated Show resolved Hide resolved
tests/test.rfb.js Outdated Show resolved Hide resolved
@otthou
Copy link
Contributor Author

otthou commented Sep 11, 2023

I can combine the tests and make them more end to end yes, instead of setting and checking internal states for both separate halves.

@otthou
Copy link
Contributor Author

otthou commented Sep 26, 2023

@CendioOssman Can you take another look at this?

@CendioOssman
Copy link
Member

Sorry for the delay. I'm unfortunately completely booked with other things at the moment. Please bear with me and I will have another look at this PR once things calm down a bit.

@otthou
Copy link
Contributor Author

otthou commented Sep 27, 2023

Ok, thanks for letting me know. Also not sure why the linter is failing here. It passes just fine locally, and it looks like it is failing with the same error on master too.

@CendioOssman
Copy link
Member

Yeah, I don't know what the linter is up to. I can't reproduce the problem here either.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! Just squash everything to a single commit and I can go ahead and merge it.

Previously, num-lock and caps-lock syncing was performed on a best effort basis by qemu.
Now, the syncing is performed by no-vnc instead. This allows the led state syncing to work
in cases where it previously couldn't, since no-vnc has with this extension knowledge of both
the remote and local capslock and numlock status, which QEMU doesn't have.
@otthou otthou force-pushed the qemu_ledstate_pseudo_encoding branch from 5087122 to a0b7c0d Compare September 29, 2023 12:06
@otthou
Copy link
Contributor Author

otthou commented Sep 29, 2023

Squashed

@CendioOssman CendioOssman merged commit 85a4652 into novnc:master Sep 29, 2023
11 checks passed
@otthou otthou deleted the qemu_ledstate_pseudo_encoding branch September 29, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants