Skip to content

Miscelaneous improvements and fixes#18

Merged
hardening merged 21 commits intohardening:masterfrom
Rubycat-R-D-Labs:lkc_upstream_keyboard_refactor
Nov 4, 2025
Merged

Miscelaneous improvements and fixes#18
hardening merged 21 commits intohardening:masterfrom
Rubycat-R-D-Labs:lkc_upstream_keyboard_refactor

Conversation

@lkc-rc
Copy link
Contributor

@lkc-rc lkc-rc commented Oct 21, 2025

  • Fix an issue where the compositor wasn't actually minimizing redraws
  • Add support for Qt > 6.7
  • refactor keyboard handling code in its own class
  • fix a few minor keyboard handling issues
  • Add two strategies to enable interoperability with qtwebengine using keyboard layouts other than US qwerty

@hardening
Copy link
Owner

This masquerade_xcb build option is not clear to me. I feel like:

  1. this could possibly be a runtime option
  2. if we really need different plugins, we could build 2 plugins from the same sources with different defines, or I'm missing something ?

@lkc-rc
Copy link
Contributor Author

lkc-rc commented Oct 22, 2025

Okay so:

  1. I think Q_PLUGIN_METADATA in main.h needs to be set at moc compilation time, and it can apparently only be used once in the code (https://doc.qt.io/qt-6/qtplugin.html#Q_PLUGIN_METADATA). Beyond that, I'm not sure how to parametrise src/plugins/platforms/freerdp/main.cpp for runtime configuration.
  2. I mean, we could, but masquerade_xcb is a terrible hack that would probably break normal users' installs. The point of this feature is to replace the actual xcb plugin with qfreerdp under xcb's name, so that qtwebengine uses its better native keyboard event handling codepath. To me, building this version of the plugin by default would be putting users at risk of breaking their Qt install.

@hardening
Copy link
Owner

For 1 (runtime option), that could take the form of two different keyboard handling classes. When I've seen the changes that were moving the keyboard handling to its own class, I though that mKeyboard would be a pointer to a class instantiated at runtime (so either to a normal one or a xcb maquerade one).
Also a single plugin can export multiple names, that's why you have QFreeRdpIntegrationPlugin::keys, QFreeRdpIntegrationPlugin::create so that could be parametrized at runtime (if the requested name is freerdp_xcb, we would instantiate the platform with the corresponding keyboard)

@lkc-rc
Copy link
Contributor Author

lkc-rc commented Oct 22, 2025

That indeed sounds way better, I just wasn't able to make that happen with my limited knowledge of qt platform plugins.

We just need to make sure that QGuiApplication::platformName() returns exactly "xcb" (https://github.com/qt/qtwebengine/blob/603a1809481eb4d4ca972f0f64915d29fb99f53b/src/core/web_event_factory.cpp#L95).

Also, there is no need for a second qfreerdppeerkeyboard class, because the single one already behaves correctly by setting nativeScanCode, nativeVirtualKey, and nativeModifiers in QKeyEvent. It's just that qtwebengine refuses to use them if you're not called "xcb" (or "wayland") :/

Do you, by any chance, have additional documentation on QPlatformIntegrationPlugin ?

@hardening
Copy link
Owner

That indeed sounds way better, I just wasn't able to make that happen with my limited knowledge of qt platform plugins.

We just need to make sure that QGuiApplication::platformName() returns exactly "xcb" (https://github.com/qt/qtwebengine/blob/603a1809481eb4d4ca972f0f64915d29fb99f53b/src/core/web_event_factory.cpp#L95).

Also, there is no need for a second qfreerdppeerkeyboard class, because the single one already behaves correctly by setting nativeScanCode, nativeVirtualKey, and nativeModifiers in QKeyEvent. It's just that qtwebengine refuses to use them if you're not called "xcb" (or "wayland") :/

Do you, by any chance, have additional documentation on QPlatformIntegrationPlugin ?

Nope, but you can look at the code or if you set QT_DEBUG_PLUGINS env variable, you will see tons of logs of the plugin loading process, that usually helps (also that's so verbose that you set it once when debugging and then remove it)...

@lkc-rc lkc-rc force-pushed the lkc_upstream_keyboard_refactor branch from 0fd2bad to 7adfd4c Compare October 27, 2025 08:49
@hardening
Copy link
Owner

Would be nice if you could rebase your PR on master, I've added CI checks on the repo so that we can test builds with Qt 5 and 6.

lkc-rc and others added 17 commits October 30, 2025 09:49
Native windows machines generate a stream of keydown events when a key
is held down, including modifiers. This led to modifiers in xkb_state
being stuck in spurious states.

This commit disables the update of xkb's state on subsequent occurences
of a key down event, as recommended by
https://xkbcommon.org/doc/current/md_doc_2quick-guide.html
This line was incorrect given the type declared previously, and the
default behavior seems correct compared to an actual french windows
observed in the wild.
Also factorize code to send full refresh to the connected peer.
Caching was done on a stall variable not used anymore instead
of using the actual region updated by Qt.
Our compositor fix was a bit too overzealous and we ended up trimming
even update that should have been full
When confronted with an unknown platform like qfreerdp, qtwebengine
falls back to assuming that all inputs are generated by a US qwerty
keyboard.

In order to be able to use web apps that rely on DOM (key) .code
(supposed to represent the physical location of the keys on the
keyboard, not affected by any layout setting), we transform the Qt::Key
sent along each QKeyEvent as if sent by a US keyboard.

Given that qtwebengine seems to rely on the `text` attribute to generate
DOM .key event attributes, this workaround should not affect text entry
with different layouts.
https://github.com/qt/qtwebengine/blob/603a1809481eb4d4ca972f0f64915d29fb99f53b/src/core/web_event_factory.cpp#L1641
QFreeRdpPeer was starting to get a bit too crowded, especially with all
the conversion tables. Keyboard handling fits well as its own unit, and
will make writing unit tests for it later easier.

This was also the occasion to review the code and upgrade some call to
libxkbcommon to a more modern API.
The original name was just cruel to users.
Also modernize modifiers xkb state handling code.
This will improve keyboard support on qt webengine apps.
Introduces a new platform name: "freerdp_xcb", and does not conflict
with the actual installed xcb plugin anymore.
@lkc-rc lkc-rc force-pushed the lkc_upstream_keyboard_refactor branch from 7adfd4c to 44359af Compare October 30, 2025 09:25
@hardening hardening merged commit ac38016 into hardening:master Nov 4, 2025
1 check passed
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.

3 participants