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

Qt desktop GUI: upgrade to Qt6 #9189

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

SomberNight
Copy link
Member

@SomberNight SomberNight commented Sep 10, 2024

This PR migrates the Qt Widgets GUI used on desktop from Qt5 to Qt6.

Notes:

  • the latest debian stable and ubuntu lts now both package python3-pyqt6, meaning most distros should now have it
  • raises the minimum Windows version needed (for users) to win10 x64 (from 8.1 32 bit?)
  • raises the min macOS version needed to 11 (from 10.13)
    • related Minimum supported MacOS version (OSX, mac, qt, pyqt) #3685
    • 10.13 was released in 2017-09, and 11 released in 2020-11. That's a significant bump, and the new minimum is only ~4 years old. Is this OK?
      • I think we bumped to 10.13 (from 10.11) around 095464b, which was released in 4.0.1 in 2020-07, at which point 10.13 was ~3 years old. Hence, the current bump is less strict and I think is OK.
  • the win/mac/appimage binaries grew by ~20 MB :/
    • qt6 takes up more space. I tried to strip out some unneeded new modules (and adjusted our existing stripping-out to layout changes) - without that the size diff would be even greater.
  • when running the AppImage on a system that has both wayland and xwayland, AFAICT previously QT_QPA_PLATFORM=xcb was the default, but now it is QT_QPA_PLATFORM=wayland
    • compare running $ QT_QPA_PLATFORM=xcb ./run_electrum vs $ QT_QPA_PLATFORM=wayland ./run_electrum
    • note that they look a bit different
      • in particular the titlebar for e.g. QMessageBox is missing on wayland
        • e.g. try in console tab: >>> window.question(msg="msg")

closes #8007

@accumulator
Copy link
Member

accumulator commented Sep 16, 2024

electrum/gui/common_qt/__init__.py Outdated Show resolved Hide resolved
electrum/gui/qt/qrreader/__init__.py Outdated Show resolved Hide resolved
SomberNight added a commit that referenced this pull request Sep 16, 2024
related #9189
(version 3.2 added support for qt6, so this version supports both qt5 and qt6)
@SomberNight
Copy link
Member Author

There's a load_stylesheet_pyqt6() func since qdarkstyle>=3.2

Thanks, done.


At a higher level, what do you think about doing this migration now? :) AFAICS there are no real blockers.

@accumulator
Copy link
Member

accumulator commented Sep 17, 2024

There's a load_stylesheet_pyqt6() func since qdarkstyle>=3.2

Thanks, done.

At a higher level, what do you think about doing this migration now? :) AFAICS there are no real blockers.

It looks fully functional. The only thing I ran into testing the branch was an error when grabbing QR from screen. That was on both wayland and xcb backends.

I didn't test the AppImage yet. There might be dependency issues between pyqt6 and debian buster.

SomberNight added a commit to SomberNight/electrum that referenced this pull request Sep 17, 2024
The wayland plugin would require at least debian 12 (or ubuntu 22.04) at runtime.

see spesmilo#9189 (review) :

> I've now tried running the appimage on debian 10 (oldoldstable), and am getting an error with wayland.
>
> ```
>   4.16 | D | util.profiler | Plugins.__init__ 0.0422 sec
>   4.16 | I | daemon.Daemon | launching GUI: qt
>   4.76 | I | gui.qt.ElectrumGui | Qt GUI starting up... Qt=6.7.1, PyQt=6.7.1
> /tmp/.mount_electrFlGFOt/usr/bin/python3: symbol lookup error: /tmp/.mount_electrFlGFOt/usr/lib/python3.11/site-packages/PyQt6/Qt6/plugins/platforms/../../lib/libQt6WaylandClient.so.6: undefined symbol: wl_proxy_marshal_flags
> ```
>
> If I explicitly specify `QT_QPA_PLATFORM=xcb`, it starts and works as expected. But it picks wayland by default.
> I found https://bugreports.qt.io/browse/QTBUG-114635 and it looks like even debian 11 might be affected.
macOS reserves the "About" menu item name, similarly to "Preferences" (see a few lines above).
The "About" keyword seems even more strictly locked down:
not allowed as either a prefix or a suffix.
- With Qt5, a matching menu item is simply auto-recognised as the special "About" item,
- but with Qt6, it seems we explicitly have to do this dance, as directly adding
  a menu item with the "About" name results in a segfault...
Some checkboxes, e.g. main_window.warn_if_testnet became buggy with pyqt6:
looks like the stateChanged signal passes an int, not a Qt.CheckState.
(and note that Qt.CheckState is an Enum, not an IntEnum).
So `x == Qt.CheckState.Checked` would always evaluate to False.

```
def on_cb(_x):
    print(f"heyheyhey. {_x=!r}, {Qt.CheckState.Checked=!r}, {cb.checkState()=!r}, {cb.isChecked()=!r}")
cb = QCheckBox("")
cb.stateChanged.connect(on_cb)
```

heyheyhey. x=2, Qt.CheckState.Checked=<CheckState.Checked: 2>, cb.checkState()=<CheckState.Checked: 2>, cb.isChecked()=True
heyheyhey. x=0, Qt.CheckState.Checked=<CheckState.Checked: 2>, cb.checkState()=<CheckState.Unchecked: 0>, cb.isChecked()=False
The wayland plugin would require at least debian 12 (or ubuntu 22.04) at runtime.

see spesmilo#9189 (review) :

> I've now tried running the appimage on debian 10 (oldoldstable), and am getting an error with wayland.
>
> ```
>   4.16 | D | util.profiler | Plugins.__init__ 0.0422 sec
>   4.16 | I | daemon.Daemon | launching GUI: qt
>   4.76 | I | gui.qt.ElectrumGui | Qt GUI starting up... Qt=6.7.1, PyQt=6.7.1
> /tmp/.mount_electrFlGFOt/usr/bin/python3: symbol lookup error: /tmp/.mount_electrFlGFOt/usr/lib/python3.11/site-packages/PyQt6/Qt6/plugins/platforms/../../lib/libQt6WaylandClient.so.6: undefined symbol: wl_proxy_marshal_flags
> ```
>
> If I explicitly specify `QT_QPA_PLATFORM=xcb`, it starts and works as expected. But it picks wayland by default.
> I found https://bugreports.qt.io/browse/QTBUG-114635 and it looks like even debian 11 might be affected.
@SomberNight
Copy link
Member Author

The only thing I ran into testing the branch was an error when grabbing QR from screen. That was on both wayland and xcb backends.

I think that functionality is already broken on Linux if wayland is being used as window manager (regardless of the qpa plugin used). It might still work if wayland is not used at all.
On Windows and macOS, it works though.

@SomberNight SomberNight merged commit 5548d28 into spesmilo:master Sep 18, 2024
11 of 15 checks passed
SomberNight added a commit that referenced this pull request Oct 10, 2024
SomberNight added a commit that referenced this pull request Oct 10, 2024
This fixes the pinmatrix dialog (used by trezor one, keepkey, safet),
which was previously segfaulting.

follow-up #9189
SomberNight added a commit that referenced this pull request Oct 14, 2024
related #9189
(version 3.2 added support for qt6, so this version supports both qt5 and qt6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Qt desktop GUI: upgrade to Qt6 (gui/qt/, QtWidgets)
2 participants