-
Notifications
You must be signed in to change notification settings - Fork 326
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
Provide coinstallability for Qt 5 and Qt 6 themes #1789
Conversation
This is needed for sddm/sddm#1789 to have SDDM run a Qt 6 greeter.
4bdcd47
to
6766038
Compare
# Keep the unversioned name for Qt5. When upgrading SDDM, the old daemon | ||
# might still be running and only know about "sddm-greeter". Keeping the | ||
# previous name around also helps users calling it directly. | ||
set(GREETER_TARGET sddm-greeter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just handle the unversioned name via symlink for this purpose? I'd rather us just change to versioned names unless we intend to quickly drop Qt5 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of using a symlink instead?
I decided against it because it's one more file and installing a symlink with cmake needs a custom command...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does? file(CREATE_LINK)
has been available since CMake 3.14...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would need to check whether bumping the requirement would be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set(CMAKE_CXX_STANDARD 17)
was added in CMake 3.20, so you already require that as a minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was most likely an oversight, but nobody complained so it's probably ok 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set(CMAKE_CXX_STANDARD 17) was added in CMake 3.20, so you already require that as a minimum.
That was actually in CMake 3.8: https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html#prop_tgt:CXX_STANDARD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh welp, right, C++20 was in CMake 3.20. My bad.
Does the daemon need a similar -qt6 handling or is it expected that mixing qt5 daemon and qt6 greeter (or vice-versa) will still work? |
Both combinations are valid. #1790 makes that even more explicit. |
@aleixpol @davidedmundson Could you provide input on the open topics? |
This is needed for sddm/sddm#1789 to have SDDM run a Qt 6 greeter.
Ping! This needs to be done before anyone develops themes without QtVersion=6 which will then inadvertedly break after this is merged. |
Diff implies that pragmatically they are.
I would say no, the point of this is to have SDDM do the work so theme devs don't have to. QML is not ifdef-able and trying to support multiple versions has not been easy for other places.
The default theme (elarun) has to work in both. That gets baked in via qrc into both executables. We also have the case at a cmake level of not building a Qt6 greeter at which point we want these to work. I would suggest leave them alone as-is for now then bump when we make Qt6 mandatory. I could not reproduce the Image issue.
Definitely, especially the part for packagers |
That's for TS files. QM files are basically serialized Qt hashes and I have no idea whether that's meant to be stable... Arguably if they're compatible now, then using the Qt 5 files for Qt 6 is fine, but not necessarily the other way around.
Yep...
Ok.
Can do. Currently the wiki is the best documentation source or should I put that into README? |
Ping |
Ping. |
I patched the sddm package on my Arch box and installed it. Works like a charm and the code is simple enough. Feel free to consider this "Tested-by" and "Reviewed-by" fwiw o/ |
void ThemeMetadata::setTo(const QString &path) { | ||
QSettings settings(path, QSettings::IniFormat); | ||
// read values | ||
d->mainScript = settings.value(QStringLiteral("SddmGreeterTheme/MainScript"), QStringLiteral("Main.qml")).toString(); | ||
d->configFile = settings.value(QStringLiteral("SddmGreeterTheme/ConfigFile"), QStringLiteral("theme.conf")).toString(); | ||
d->translationsDirectory = settings.value(QStringLiteral("SddmGreeterTheme/TranslationsDirectory"), QStringLiteral(".")).toString(); | ||
d->qtVersion = settings.value(QStringLiteral("SddmGreeterTheme/QtVersion"), 5).toInt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we set QtVersion
for the themes we ship? Casual grep suggests that these would need an update:
data/themes/elarun/metadata.desktop
data/themes/maldives/metadata.desktop
data/themes/maya/metadata.desktop
src/greeter/theme/metadata.desktop
While I haven't tried the Qt6 side, just yet. I've build this with Qt5 and it works like a charm.
With the above .desktop/QtVersion handled, I think we're good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we set QtVersion for the themes we ship? Casual grep suggests that these would need an update:
Kind of. The builtin themes are only semi-compatible with Qt 6 :-/
That's also mentioned in the PR message:
- What should the example themes specify as Qt version? elarun currently has a broken background with Qt 6.
When not building for Qt 5, give the sddm-greeter binary a Qt version suffix. Also version the translation directory.
metadata.desktop in the theme directory has a new key "QtVersion" which defaults to 5. If it's not 5, sddm-greeter-qt${QtVersion} will be used to show the theme.
This is needed for sddm/sddm#1789 to have SDDM run a Qt 6 greeter.
This is needed for sddm/sddm#1789 to have SDDM run a Qt 6 greeter.
sddm-greeter is already a separate process, so it's actually rather simple to support themes using Qt 5 and themes using Qt 6 on the same system. This PR achieves this by installing the greeter specific files with a -qt6 suffix when
BUILD_FOR_QT6=ON
and dynamically choosing which greeter to execute based on theQtVersion=
property in the thememetadata.desktop
. For backwards compatibility, this defaults to 5 if not present.This means that all current Qt 6 themes (i.e. just breeze?) will have to add
QtVersion=6
, otherwise the Qt 5 greeter will be used.Open topics:
QtVersion
property allow to specify multiple versions as fallback? I'm not sure whether it's practical to have a theme which supports multiple major versions, especially if .qm files are not necessarily compatible.QtVersion
property and behaviourWith #1790 on top, the Qt 6 build will automatically build and install the Qt 5 greeter as well.