Differential D14000

qqc2-desktop-style: basic support for QGuiApplication-based apps
ClosedPublic
Actions

Authored by rjvbb on Jul 9 2018, 9:56 AM.

Details

Reviewers
mart
Group Reviewers
Frameworks
Commits
R858:1e02355c1786: Support for QGuiApplication-based apps
Summary

See https://bugs.kde.org/show_bug.cgi?id=396287

Setting QT_QUICK_CONTROLS_STYLE=org.kde.desktop used to be safe but has started the cause applications to crash. Or I just never ran into what seem to be random (from a Joe User experience) but in fact are QGuiApplication-based applications.

This patch presents a proof-of-concept fix that makes it safe to set QT_QUICK_CONTROLS_STYLE or whatever other options there are to use the QQC2 desktop style in all applications that use QuickControls2.
The current implementation creates a local instance of the user's current desktop style obtained from KConfig, with Qt's built-in Fusion style as fallback. Creating a new QStyle instance is an easy way to follow changes to the user style. There's probably some optimisation to be done here though I'm not convinced it will make any difference in practice, in the general context of using QtQuick.

An alternative approach would be to do the programmatic equivalent of the command-line override (e.g. quickcontrols2/gallery/gallery -style Default) but I have not yet been able to figure out if it is possible to change the QQuickStyle at this point. Probably not.

Test Plan

Tested with

> env QT_QUICK_CONTROLS_STYLE=org.kde.desktop /path/to/qt5-examples/quickcontrols2/gallery/gallery

before: immediate crash because the plugin dereferences a NULL qApp->style() without flinching
after: practically normal behaviour that is certainly preferable to crashing due to a potential security exploit.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 750
Build 762: arc lint + arc unit
rjvbb created this revision.Jul 9 2018, 9:56 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 9 2018, 9:56 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rjvbb requested review of this revision.Jul 9 2018, 9:56 AM
rjvbb updated this revision to Diff 37439.Jul 9 2018, 10:09 AM
rjvbb edited the summary of this revision. (Show Details)
rjvbb edited the test plan for this revision. (Show Details)

Getting the user's desktop style was easier than I thought.

rjvbb edited the summary of this revision. (Show Details)Jul 9 2018, 10:12 AM
rjvbb edited reviewers, added: Frameworks; removed: Framework: Syntax Highlighting.
rjvbb set the repository for this revision to R858 Qt Quick Controls 2: Desktop Style.
broulik added a subscriber: broulik.Jul 9 2018, 12:31 PM

Why, or who, is setting this environment variable? Plasma-integration sets QQuickStyle depending on whether there is a QApplication or not, so this makes no sense.

mart added a subscriber: mart.Jul 10 2018, 4:22 PM

in principle i agree, I would love to be able to use this in qguiapplications, but are we sure qstyles won't access qapplication? doesn't this risk to just move the crash?

rjvbb updated this revision to Diff 37568.Jul 11 2018, 2:05 PM

build on Mac too

davidedmundson added a subscriber: davidedmundson.Jul 16 2018, 3:35 PM

Please swap qApp->style for QApplication::style as it's static.

Solution is quite clever.
It's risky as any qstyle will always be written assuming it's a QApplication; but by definition the QQC2 code shields from a lot of the QWidget casting that would cause an issue.

Worst case we crash, which is the current state.

I don't think we should remove the plasma-integration guard, but this is surprisingly un-invasive. Meh, +1

alexeymin added a subscriber: alexeymin.Jul 16 2018, 3:58 PM

Don't know how much offtopic will be what I'm going to say, but I recently ran into an issue while packaging QQC2 application in flatpak with org.kde.Platform//5.9 runtime. My app was functioning perfectly fine in system, but under flatpak it failed to start with the following message: QWidget: Cannot create a QWidget without QApplication. Of course, I had only QGuiApplication. But I wasn't using widgets! The only class from QtWidgets was QSystemTrayIcon for a nice integration with desktop. My guess was that was caused by platform style used in flatpak's KDE runtime may use widgets. But this was unexpected, without flatpak all was working. In the end I was forced to use QApplication, and visually saw the style used was ogr.kde.desktop.

I wrote a small test QML QQC2 program with QGuiApplication and packaged it with flatpak with the same runtime, surprisingly, it started OK, without any error message. Basically, QQC2 ApplicationWindow with a button inside, not linked to widgets. ( https://github.com/minlexx/test_qqc2hello - repo with test scripts )

So, if the program is linked to Qt5Widgets in any way, it has to use QApplication in flatpak's org.kde.Platform even if it does not use widgets? Is it related to this patch? Will it solve my problem? (if included in flatpak's kde runtime) 😄

rjvbb updated this revision to Diff 37897.Jul 16 2018, 5:08 PM

Using QApplication::style()

rjvbb updated this revision to Diff 44083.Oct 22 2018, 7:47 PM

updated for the current git/head.

David: did you perhaps forget to accept the revision (aka, should I have committed this)?

rjvbb retitled this revision from qqc2-desktop-style: basic support for QGuiApplication-based apps (WIP/PoC) to qqc2-desktop-style: basic support for QGuiApplication-based apps.Nov 13 2018, 10:20 AM
mart accepted this revision.Nov 23 2018, 2:58 PM
This revision is now accepted and ready to land.Nov 23 2018, 2:58 PM
Closed by commit R858:1e02355c1786: Support for QGuiApplication-based apps (authored by rjvbb). · Explain WhyNov 24 2018, 9:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mplugin/kquickstyleitem.cpp (256 lines)
Mplugin/kquickstyleitem_p.h (9 lines)

Diff 37568

View Options

plugin/kquickstyleitem.cpp

Loading...
View Options

plugin/kquickstyleitem_p.h

Loading...