Bug 8337 - Switch client interface to Qt
Summary: Switch client interface to Qt
Status: NEW
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Client (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.18.0
Assignee: Bugzilla mail exporter
URL:
Keywords:
Depends on: 5878 8361
Blocks: 5646
  Show dependency treegraph
 
Reported: 2024-04-23 06:20 CEST by Pierre Ossman
Modified: 2024-10-28 16:09 CET (History)
2 users (show)

See Also:
Acceptance Criteria:
MUST: * FLTK should be completely replaced with Qt * All uses cases supported by the FLTK client should work the same in the Qt client * Works on all current Linux, Windows, and macOS systems without extra user steps SHOULD: * New code follows existing style in architecture and formatting * The commit history/blame clearly shows what behaviours have changed, and which remain the same WON'T: * Resolve issues or missing features in the FLTK client


Attachments
How to build Qt statically (18.18 KB, text/plain)
2024-04-23 06:34 CEST, Pierre Ossman
Details
How to link Qt statically to TigerVNC (10.78 KB, text/plain)
2024-04-23 06:35 CEST, Pierre Ossman
Details
Photo of Qt Creator in French (72.77 KB, image/jpeg)
2024-04-24 10:49 CEST, Pierre Ossman
Details

Description Pierre Ossman cendio 2024-04-23 06:20:36 CEST
The client is currently built upon FLTK. It was probably a good choice at the time, as it is available on every platform we need, gets the job done and is fairly lightweight.

It is very basic in its functionality, though, so as we want need features we want a more feature complete toolkit so we don't have to implement everything we need ourselves.

As we want to keep having a single codebase for all platforms, the only toolkit available is Qt. It is also LGPL, just like FLTK.
Comment 2 Pierre Ossman cendio 2024-04-23 06:26:25 CEST
Although Qt is generally vastly better than FLTK, there are some things that don't work quite like we want.

One such issue is the lack of support for full screen on multiple monitors. Not terribly surprising, since KDE didn't seem to care for that (bug 5482).

That means we'll have to come up with something more manual to handle full screen, rather than what's built in to Qt.

Our first iteration mimics Qt, in that it assumes support for EWMH. I.e. it won't work without a window manager. Unclear if it's worth the effort to support a bare X11 session. We could try releasing it without that support and see if people complain.
Comment 3 Pierre Ossman cendio 2024-04-23 06:28:57 CEST
Qt is able to handle macOS' native full-screen functionality (bug 4381). However, that only works on a single monitor. So it was difficult to make it interact well when we need something besides "current monitor".

For now, we've opted to completely disable it and always do full screen manually, similar to how it's done in FLTK.
Comment 4 Pierre Ossman cendio 2024-04-23 06:33:54 CEST
We are currently targetting Qt 5. That should hopefully not be a problem getting into our build system for Linux and Windows. But for macOS, the minimum requirements are 10.13. So we need to get a more modern SDK in place (bug 5878).

We also need to figure out how to get everything statically linked for easy distribution. Fortunately, we've got instructions from Qt on how to do this. It does rely on using CMake, and using some hidden magic in Qt's CMake files. We'll see if that's what we'll end up using, or if we'll use it as inspiration for how to link everything more manually.
Comment 5 Pierre Ossman cendio 2024-04-23 06:34:54 CEST
Created attachment 1188 [details]
How to build Qt statically
Comment 6 Pierre Ossman cendio 2024-04-23 06:35:15 CEST
Created attachment 1189 [details]
How to link Qt statically to TigerVNC
Comment 7 Pierre Ossman cendio 2024-04-23 06:45:02 CEST
Note that we originally tried to port things to Qt Quick 2 / Qt QML. This is the modern framework for Qt, designed with mobile touch devices in mind. The classical Qt, "Widgets", has been in maintenance mode for quite some time, so it seemed like the safe bet for the future was the new framework.

Unfortunately, it turned out to simply not be performant enough. It uses OpenGL (or Vulkan/Metal/DirectX 12 in Qt 6) to render everything, and it seems like it takes the brute force approach to things. This turned out to scale very poorly, and ended up being multiple times slower than the FLTK viewer.

We found some issues indicating that this seems like a fairly fundamental design choice in Qt Quick:

https://bugreports.qt.io/browse/QTBUG-74928
https://bugreports.qt.io/browse/QTBUG-101976

We were able to get some more performance out of things by using raw OpenGL commands to upload the frame buffer to a texture, but it still wasn't even close to the FLTK viewer's performance.

And although Qt Quick was intended to also support writing classical applications, it isn't that polished for that use case at the moment. The widgets look out of place, and there were other fundamental design issues like menus being drawn as part of the windows, so they could never extend outside of them.


For these reasons, we ended up pivoting to the classical Qt Widgets. Although they're probably not getting much new development, they are what Qt themselves use for all their tools. E.g. their big fancy IDE. So it's unlikely it will go away any time soon.


Note that the attachments for linking Qt statically were written with Qt Quick in mind. But it should hopefully be roughly the same for Qt Widgets.
Comment 8 Pierre Ossman cendio 2024-04-23 07:00:18 CEST
Next bug/regression we found is that Qt fails to track clipboard ownership on macOS properly. This has the effect of ping-ponging the clipboard back and forth a bit:

 1. Copy something in the ThinLinc session
 2. The clipboard is sent to the client
 3. The client sets the local macOS clipboard
 4. It gets a notification from Qt that the clipboard has changed
 5. Qt claims we don't own the clipboard, so we send it to the server
 6. Server overwrites the clipboard with the versions that's bounced of the client

The above scenario works with the FLTK client, but it turns out that's mostly by luck. If you do the same thing, but after step 3., you switch to another local application and then back to the client, then you'll get a bad notification and things will break just like for Qt.

The reason for this difference is that macOS has no clipboard events. So you have to poll for changes. And it turns out Qt and FLTK poll differently.

The issue is reported to Qt here:

https://bugreports.qt.io/browse/QTBUG-15980
https://bugreports.qt.io/browse/QTBUG-124214

We've added a crude workaround for now where we simply compare the contents of the clipboard.

Also note that the discussed Qt documentation also states that this is broken on Windows. This doesn't seem to be true, as we don't see such issues there.
Comment 9 Pierre Ossman cendio 2024-04-23 07:06:08 CEST
We're also seeing a bug on Windows where Qt doesn't report screen layout changes properly. It seems like only some changes trigger properly.

There are these bug reports for the problem:

https://bugreports.qt.io/browse/QTBUG-108348
https://bugreports.qt.io/browse/QTBUG-111874
https://bugreports.qt.io/browse/QTBUG-124202

I had a look at the Qt code, and I think I know what the bug is. Qt seems to trigger on the same WM_DISPLAYCHANGE that FLTK does. But it tries to be clever. That message includes the new dimensions of the monitor. But it's just a single set of dimensions, so I'm assuming it's a legacy thing since before support for multiple monitors. Still, Qt compares that to previous messages and assumes nothing has changed if those dimensions are the same. Which isn't true for such simple things as changing the position of a monitor.

The methods updating screen state seem to be private and unreachable from application code. But we might be able to fool it by sending a fake WM_DISPLAYCHANGE message with bogus dimensions. It doesn't seem to do anything useful with them anyway.

Or we can decide to fix it in our Qt and not need a workaround.
Comment 10 Pierre Ossman cendio 2024-04-24 10:24:03 CEST
There is an issue on macOS where the "Services" sub menu is not translated. This works fine with our current client, as can be seen in the screenshot on bug 8337 (there are other translations bugs in the current client, though).

Unfortunately, this seems to be a Qt issue as the same problem can be seen in Qt's own tools, like Qt Creator.

Oddly enough, the issue can also be seen with the macOS binaries downloaded from TigerVNC. Those are not built by us, so there seems to be something subtle here that is easy to get wrong.
Comment 11 Pierre Ossman cendio 2024-04-24 10:49:45 CEST
Created attachment 1190 [details]
Photo of Qt Creator in French
Comment 12 Pierre Ossman cendio 2024-04-25 16:36:16 CEST
For reference, this is on Qt itself used to handle full screen before macOS got native support (10.7):

https://github.com/qt/qtbase/commit/bdd3ead8270dfde12b0b4eb57125a22e51ee88ce
Comment 13 Pierre Ossman cendio 2024-04-26 13:42:51 CEST
Dark mode (bug 7968) seems to work out-of-box on macOS. Windows doesn't work yet, and looks like Qt 6.5 is needed:

https://www.qt.io/blog/dark-mode-on-windows-11-with-qt-6.5

I tried the -platform arguments suggested there, and it looked horrible.

Linux is unknown at the moment. Hopefully, respects KDE's dark mode setting just fine, but might be issues with GNOME's.
Comment 14 Pierre Ossman cendio 2024-05-23 10:06:24 CEST
(In reply to Pierre Ossman from comment #10)
> There is an issue on macOS where the "Services" sub menu is not translated.
> This works fine with our current client, as can be seen in the screenshot on
> bug 8337 (there are other translations bugs in the current client, though).
> 
> Unfortunately, this seems to be a Qt issue as the same problem can be seen
> in Qt's own tools, like Qt Creator.
> 
> Oddly enough, the issue can also be seen with the macOS binaries downloaded
> from TigerVNC. Those are not built by us, so there seems to be something
> subtle here that is easy to get wrong.

It's not just the menu. The Open and Save dialogs are also not translated.

I found this discussion:

https://forums.developer.apple.com/forums/thread/670760

After adding CFBundleLocalizations, the menu translations worked just fine. I did not have to add those dummy files. I also did not check if it correctly defaults to English for unknown languages.

Also note that we do not have CFBundleLocalizations in the current tlclient, which still works. So there seems to be more than one way to get proper translations.
Comment 15 Linn cendio 2024-05-28 11:19:26 CEST
Found a regression in an edge case where OS fullscreen doesn't work as intended when running the vncviewer over "ssh -X".

I used the following test setup:
 - Client machine: Fedora 39
 - Used ThinLinc to log in to a Mate desktop environment
 - Ran "ssh -X" in the ThinLinc session.

How to reproduce:
1. Have the session in windowed mode.
2. Use OS fullscreen, session is correctly going into fullscreen.
3. Use OS fullscreen again. It looks like the session is trying to go out of fullscreen, but it pops back into fullscreen again.
4. Use OS fullscreen again. Now we go into windowed mode as expected.

If I instead used the vncviewer from TigerVNC 1.13.1, the session correctly goes out of fullscreen in step 3.
Comment 16 Pierre Ossman cendio 2024-05-30 13:06:24 CEST
High DPI support is much better in Qt and does support it fully. There are some corner cases that still will require more work though:

 * The cursor scales, but still has artefacts on macOS (bug 8356)

 * Monitors with different scaling factors is still a mess on Windows, because we still need to creatively calculate dimensions and offsets (bug 7014)
Comment 17 Pierre Ossman cendio 2024-06-04 10:47:07 CEST
When we did full-screen support for FLTK, we considered all cases for X11:

 * No window manager at all
 * Window manager without full-screen support
 * Window manager with only single monitor full-screen support
 * Window manager with full support for multi-monitor full screen

It's not clear it's worth the effort to have all these working in the Qt port. I think it could be reasonable to require a decent window manager for the full feature set.

We've focused on the last case, but have made some basic attempt at solving at least the first case. More testing is needed if we want to support everything, though.
Comment 18 Pierre Ossman cendio 2024-06-19 11:15:52 CEST
Qt has a lot of fancy network stuff as well, which means we can get rid of the neon library for checking client updates. It also supports TLS, which could solve bug 4959. We still need to investigate that we are respecting the system trust store properly on all platforms, though, as that is a bit of a mess on Linux.

It doesn't automatically resolve bug 3205, but it gives us better tools to fix it, as Qt is very asynchronous.
Comment 19 Pierre Ossman cendio 2024-06-19 11:17:16 CEST
Here is some documentation about configuring Qt's TLS handling:

https://doc.qt.io/qt-6/qnetworkrequest.html#sslConfiguration
https://doc.qt.io/qt-6/qsslconfiguration.html#addCaCertificates
Comment 20 Samuel Mannehed cendio 2024-06-28 15:05:50 CEST
One minor detail to remember when integrating the Qt code, it seems the shortcut key 'c' for "Disconnect" in the F8-menu conflicts with the shortcut for "Ctrl". Upstream TigerVNC seems to use 'e' as the shortcut for "Disconnect".
Comment 22 Linn cendio 2024-07-02 10:56:46 CEST
Tested HiDPI (branch qt-hidpi_final2) for tigervnc. 

To summarize, when using Qt 5, almost all platforms have issues when there are different DPIs on different screens (e.g. one sceen zoomed 100%, one screen zoomed 200%). When using Qt 6, this is improved, but some platforms still have issues when screens have different DPI:s.

HiDPI on Windows and macOS were only tested with Qt 5. Single vs several screens means how many screens tigervnc is running fullscreen on.

Things I looked at:
  * vncviewer main window scales
  * options window scales
  * other windows scales, e.g. "About" and password input window [*]
  * F8-menu scales
  * In the version I tested, some platforms had an FPS overlay in the top left corner. IIRC, the FPS was only shown on Windows and macOS

[*] Note that the padlock icon shown when entering a password is quite pixel-y in HiDPI and probably should be updated
Comment 23 Linn cendio 2024-07-02 10:58:06 CEST
== Testing HiDPI on Windows and macos ==
See comment 22 for an overview of what was tested 

Windows
 * Qt 5:
   - Note that changes to DPI settings requires a restart to go through fully. Font size seem to be affected by this.
   - Single screen: OK
   - Several screens
     - Same DPI: OK
     - Different DPI: checkboxes and radio buttons are a tiny size
 
 * Qt 6:
   - Not tested

macOS
 * Qt 5:
   - Seen by our Qt developer: "Widget blurriness appears when application size is not an 'integer ratio'". The ratio referred to is the one between the "default" screen resolution and another screen resolution (e.g. 1920x1080 and 960x540 have a ratio of 2). I tried to reproduce this by changing the vncviewer window size to something not aligning with the screen resolution set in macOS, but I wasn't able to trigger the blurriness.
   - Single screen: OK
   - Several screens:
     - Same DPI: OK
     - Different DPI: The FPS overlay was not looking sharp, but other tested elements look fine
     
 * Qt 6:
   - Not tested.
Comment 24 Linn cendio 2024-07-02 11:03:08 CEST
== Testing HiDPI on Fedora 39 ==
See comment 22 for an overview of what was tested

GNOME X11 
 * Note that X11 only has a single global DPI scale 
 *  Qt 5:
   - Single screen: OK
   - Several screens: OK
   
 * Qt 6: 
   - Single screen: OK
   - Several screens: OK

KDE X11
 * Note that X11 only has a single global DPI scale 
 * Qt 5:
   - Single screen: Password prompt looks bad, small window, small buttons, large font overall. Other tested elements look fine.
   - Several screens:
     - Same DPI: Same issue with password prompt, other tested elements look fine.
     - Different DPI: Same issue with password prompt, other tested elements look fine.
     
 * Qt 6:
   - Single screen: OK
   - Several screens: OK

GNOME Wayland
 * Qt 5:
   - Single screen: OK
   - Several screens:
     - Same DPI: OK
     - Different DPI: OK
     
 * Qt 6:
   - Single screen: OK
   - Several screens:
     - Same DPI: OK
     - Different DPI: OK
     
KDE Wayland
 * Qt 5:
   - Single screen: Same issue with password prompt as for KDE X11. Other tested elements look fine.
   - Several screens:
     - Same DPI: Same issue with password prompt, other tested elements look fine.
     - Different DPI: Same issue with password prompt, other tested elements look fine.

 * Qt 6:
   - Single screen: OK
   - Several screens:
     - Same DPI: OK
     - Different DPI: F8-menu only appears "zoomed in" on the screen with 200% zoom, not on the one with 100%. Other tested element looks fine.
Comment 25 Linn cendio 2024-07-02 11:05:25 CEST
When testing HiDPI with X11 on Fedora 39, I noticed "freeze lag" at the start of a session (for both GNOME and KDE). I'm not sure if this is related to HiDPI or just in general with X11 and Qt 6.

The lag affects both keyboard input and mouse input - the keyboard doesn't seem to respond, and I could move the mouse but clicks and drags were not registered. Once the freeze stops after a few seconds, all previous keyboard and mouse input is registered.

I also noted that for Qt 6 with GNOME on Fedora 39, under Options -> Compression, the last "g" in "Preferred encoding" is slightly clipped. However, something changed with my window manager and fonts after installing KDE on my computer, so not sure if this is an issue on default GNOME.
Comment 26 Pierre Ossman cendio 2024-10-01 10:26:44 CEST
(In reply to Pierre Ossman from comment #4)
> We are currently targetting Qt 5.

This is not as certain any more. Looking at the distributions, Qt 6 is now not that rare. Ubuntu 20.04 lacks it, but it is soon EOL. RHEL 8 and 9 unfortunately both lack it in the base OS, but it is available in EPEL.

I discussed things with the RHEL TigerVNC maintainer, and his view is that RHEL 8 is going into maintenance mode and won't get any more major TigerVNC upgrade. And he could be okay with having to patch TigerVNC for RHEL 9.

For ThinLinc, Qt 6 would mean higher macOS requirements. Specifically, macOS 11.

Qt 6 also requires C++17. That means at minimum GCC 8, but ideally GCC 9.
Comment 28 Pierre Ossman cendio 2024-10-01 10:32:45 CEST
(In reply to Linn from comment #25)
> When testing HiDPI with X11 on Fedora 39, I noticed "freeze lag" at the
> start of a session (for both GNOME and KDE). I'm not sure if this is related
> to HiDPI or just in general with X11 and Qt 6.
> 

The WA_TranslucentBackground attribute for the toast widget blocks rendering for everything beneath it on Qt 6 for some reason. I've patched it out for now, but we'll need to investigate those attributes more.
Comment 29 Pierre Ossman cendio 2024-10-25 13:15:34 CEST
One hurdle to resolve is that plenty of things in TigerVNC are currently done semi-synchronously. E.g. opening the context menu, where a nested mainloop is started to give the appearance that we can get the result of the menu synchronously.

Unfortunately, this is a very fragile approach, as this results in code recursing in places where it is not designed to do so.

The current FLTK code is well-tested enough that the resulting bugs have been ironed out. But a switch to Qt causes a fresh set of bugs as a result of this design.

Rather than playing whack-a-mole with these, we should stop trying to play synchronous and do things properly asynchronous.

Fortunately, we can take some inspiration from noVNC which by its very nature cannot cheat and always has to do things properly.
Comment 30 Pierre Ossman cendio 2024-10-28 16:09:12 CET
On such area is the processing of incoming VNC/RFB data. It is currently done in a loop, where we explicitly call out to the mainloop between each VNC/RFB message.

Instead, we could set up so this function gets called repeatedly until there is no more available data.

In FLTK this is easily done with the explicit Fl::add_idle(). Qt, unfortunately, is not as explicit. Instead, they want you to create a timer with a timeout of 0 ms.

This mostly works well, but we seem to have hit a bug with GTK file dialogs:

https://bugreports.qt.io/browse/QTBUG-130638

However, this is in the semi-synchronous code path that we want to avoid anyway. So probably not an issue once we are done.

It is also not an issue for ThinLinc as we do not want runtime loading of GTK as that is a whole different mess. We will either use Qt's dialogs on Linux, or use the XDG desktop portal.

The issue is also unlikely to happen in practice anyway, as it requires the idle function to be constantly running. Any pause seems to be enough to get the GTK dialog going.

Note You need to log in before you can comment on or make changes to this bug.