Details

Reviewers

Group Reviewers

Commits

R135:14a546e53bfa: Use DBusMenu if available

Summary

This is part one in a series to restore global menu support in Plasma.

Since we inherit from QPlatformTheme we do not get the global menu support for free like Qt's own platform themes inheriting from QGenericUnixTheme would, hence the use of private headers. It includes a fork of Qt's QDBusMenuBar to give us access to some of its properties (the window it's operating on and the object path). This requires relicensing of Plasma-Integration to LGPL3.

To support Wayland instead of relying on window IDs alone, we set the DBus service name and object path where the menu is as window properties which KWin or any other interested party could read. This way this code becomes completely window ID agnostic and since all the boilerplate (emit layout changed and what not) is in the client, we wouldn't even really need the registrar anymore.

To facilitate keyboard navigation, long-pressing Alt will request the application to open the menu, and then either KWin will trigger the window title bar button or the application menu applet will do something.

Test Plan

checkDBusGlobalMenuAvailable() et al are from Qt's upstream
QDBusMenuBar is also from Qt
requires Qt 5.7

still need to figure out the cmake stuff

in KXMLGui applications you still get a traditional menu bar in addition to the global menu, it seems it explicitly sets visible true on the menu bar - in VLC it hides its own menu bar.

Diff Detail

Repository

R135 Integration for Qt applications in Plasma

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

broulik updated this revision to Diff 7457.Oct 17 2016, 1:25 PM

broulik retitled this revision from to RFC: Use DBusMenu if available.

broulik updated this object.

broulik edited the test plan for this revision. (Show Details)

broulik added a reviewer: Plasma.

broulik set the repository for this revision to R135 Integration for Qt applications in Plasma.

Restricted Application added a project: Plasma. · View Herald TranscriptOct 17 2016, 1:25 PM

Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

broulik mentioned this in T4071: Global Menu.Oct 17 2016, 1:51 PM

Overall significant less code than I expected - nice :-)

CMakeLists.txt
46	version 5.7?
cmake/modules/FindQt5PlatformSupport.cmake
2	given that it has my copyright I assume you copied from kwin - that would be a reason to move it to ecm. We have two users.
src/platformtheme/kdeplatformtheme.cpp
59	can we really make that static? What about runtime changes?
310	what happens if it becomes unavailable at runtime?

It's not much code on our side because Qt has the code nowadays. :)

cmake/modules/FindQt5PlatformSupport.cmake
2	+1 to that. I had one issue regarding the include path, though, but I can't reproduce it anymore and I forgot what I actually did but the path I need is /usr/include/x86_64-linux-gnu/qt5/QtPlatformSupport/5.7.0/QtPlatformSupport/ but I sometimes got /usr/include/x86_64-linux-gnu/qt5/QtPlatformSupport/5.7.0/ but I'm sure we can figur that out.
src/platformtheme/kdeplatformtheme.cpp
59	That's what Qt does and I don't think Qt is equipped to re-create a platform menu at runtime; at least that's what I was told when I raised the very same concern in the upstream Qt codereview that added this. We could perhaps do it for newly created menus but when the service becomes unavailable at runtime there's no way for us to re-create existing menus.

graesslin added inline comments.Oct 18 2016, 5:22 AM

src/platformtheme/kdeplatformtheme.cpp
59	hmm then we need to make sure that changing the option cannot be done at runtime - it has to require a session restart.

To expose the menu id and object path as X/Wayland properties on the window I would need to implement QDBusMenuBar myself as I need to implement handleReparent as well as access m_objectPath which is private. I'll see how it goes.

In D3085#57496, @broulik wrote:

To expose the menu id and object path as X/Wayland properties on the window I would need to implement QDBusMenuBar myself as I need to implement handleReparent as well as access m_objectPath which is private. I'll see how it goes.

I think you can hack it without needing to implement. We know that it's exported on the DBusConnection and we know what the object is looking like. I'm sure we can get to it without needing to reimplement.

Trying to apply the patch results in conflicts in autotests/CMakeLists.txt

Trying to compile nevertheless without autotests gives me a compile error:

/home/martin/src/kf5/kde/workspace/plasma-integration/src/platformtheme/kdeplatformtheme.cpp:47:36: fatal error: private/qdbusmenubar_p.h: No such file or directory
 #include <private/qdbusmenubar_p.h>
                                    ^
compilation terminated.
src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/build.make:62: recipe for target 'src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/kdeplatformtheme.cpp.o' failed
make[2]: *** [src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/kdeplatformtheme.cpp.o] Error 1
CMakeFiles/Makefile2:136: recipe for target 'src/platformtheme/CMakeFiles/KDEPlasmaPlatformTheme.dir/all' failed

broulik updated this revision to Diff 7549.Oct 19 2016, 3:06 PM

broulik updated this object.

broulik edited the test plan for this revision. (Show Details)

Add missing qdbusmenubar_p.h file as well as some typos and cleanups

Linking autotests fails :/

Use find QtPlatformSupport from https://git.reviewboard.kde.org/r/129260/
Fix autotests
Some cleanup

Move setWindowProperty to x11/wayland integration to unclutter the code a bit

Adds an Alt key listener which, on long-pressing Alt, will activate the menu of the active window. This only works in QApplications with a QMainWindow, I'm open to suggestions how to improve this. Also, Alt+left-click for moving around the window still causes the timer to fire

you will need the change from 052fa2e4ee329810f62c29e546254fb45bf8a375 as well I fear. Otherwise it won't compile with Qt 5.8.

graesslin added inline comments.Oct 28 2016, 2:00 PM

src/platformtheme/CMakeLists.txt
10–13	HAVE_X11 is just trying to find XLib. There is no need to bind find XCB to find XLib. You can just move it out of the if. Also the HAVE_X11 is I think wrong, but that's unrelated ;-)
62	XCB::XCB
src/platformtheme/kdeplatformtheme.cpp
115	Maybe FocusOut delivers it?
src/platformtheme/x11integration.cpp
59–60	can we cache the atom? That's causing a roundtrip every time it's invoked.

dmitryshachnev added a subscriber: dmitryshachnev.Nov 3 2016, 8:31 PM

in KXMLGui applications you still get a traditional menu bar in addition to the global menu, it seems it explicitly sets visible true on the menu bar - in VLC it hides its own menu bar.

This will be fixed in Qt 5.7.1, see this commit.

davidedmundson commandeered this revision.Dec 15 2016, 3:37 PM

davidedmundson added a reviewer: broulik.

Rebased on master with merge conflict fixed
Fixed cmake to work with Qt 5.7 and Qt5.8

Working Qt5.8 build
Qt5.7 support..hopefully
remove our own alt key handling, Qt has it in 5.7.1 in a slightly neater way
Atom caching

davidedmundson retitled this revision from RFC: Use DBusMenu if available to Use DBusMenu if available.Dec 20 2016, 1:55 PM

broulik accepted this revision.Jan 6 2017, 6:55 PM

broulik edited edge metadata.

This revision is now accepted and ready to land.Jan 6 2017, 6:55 PM

One teeny tiny catastrophe remains.

In the spec calling AboutToShow returns whether the menu changed or not and if you need to refresh it.
Our client code relies on this happening

This QPT always returns false.
The relevant code is in the QPT, but the Qt private library we're linking.

Without this all the menus that use aboutToShow don't work at all.

Spent half the day on it, not got a good solution yet.

Meh. For the title bar button shouldn't matter, though, as there I create a new importer everytime you click the button, so the menu is always up-to-date. Dunno about the applet.

huber added a subscriber: huber.Jan 8 2017, 1:46 PM

Dunno about the applet.

The applet requires it. Dynamic bookmarks just don't work otherwise.

You figured that stuf out now, right?

Closed by commit R135:14a546e53bfa: Use DBusMenu if available (authored by davidedmundson). · Explain WhyJan 11 2017, 1:33 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		CMakeLists.txt (21 lines)
D	M	COPYING.LGPL-2 (481 lines)
A	M	COPYING.LGPL-3 (165 lines)
D	M	COPYING.LIB (510 lines)
M		autotests/CMakeLists.txt (7 lines)
A	M	cmake/modules/FindQt5PlatformSupport.cmake (121 lines)
A	M	cmake/modules/FindQt5ThemeSupport.cmake (122 lines)
M		src/platformtheme/CMakeLists.txt (21 lines)
M		src/platformtheme/kdeplatformtheme.h (8 lines)
M		src/platformtheme/kdeplatformtheme.cpp (86 lines)
M		src/platformtheme/kwaylandintegration.h (2 lines)
M		src/platformtheme/kwaylandintegration.cpp (11 lines)
A	M	src/platformtheme/qdbusmenubar.cpp (187 lines)
A	M	src/platformtheme/qdbusmenubar_p.h (110 lines)
M		src/platformtheme/x11integration.h (5 lines)
M		src/platformtheme/x11integration.cpp (27 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	7457			Oct 17 2016, 1:20 PM	★	★
Diff 2	7549			Oct 19 2016, 3:04 PM	★	★
Diff 3	7569			Oct 20 2016, 1:15 PM	★	★
Diff 4	7652			Oct 25 2016, 1:58 PM	★	★
Diff 5	7726			Oct 28 2016, 1:44 PM	★	★
Diff 6	9040	d7b9319	Rebased on master with merge conflict fixed	Dec 15 2016, 3:38 PM	★	★
Diff 7	9146	d7b9319	- Working Qt5.8 build	Dec 18 2016, 10:42 PM	★	★
Diff 8	10040	102551b	R135:14a546e53bfa48aa165eca65238b4963e47636ee	Jan 11 2017, 1:29 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
39056189d4d3	da495be97c6a	e24632b15e94	David Edmundson	Atom caching	Dec 16 2016, 5:37 PM
e24632b15e94	7624acee36c9	88569f672c27	David Edmundson	remove our own alt key handling, Qt has it in 5.7.1 and arguably better	Dec 16 2016, 4:32 PM
88569f672c27	219c404c4fdc	06eee572aa30	David Edmundson	Qt5.7 support..hopefully	Dec 16 2016, 4:32 PM
06eee572aa30	9f8b3f0d2989	2c3982c3eea4	David Edmundson	Working Qt5.8 build	Dec 16 2016, 12:29 PM
2c3982c3eea4	4ea00c2b768a	d7b9319e8ba0	David Edmundson	This is part one in a series to restore global menu support in Plasma. (Show More…)	Dec 15 2016, 3:35 PM

Use DBusMenu if available
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 10040

CMakeLists.txt

COPYING.LGPL-2

COPYING.LGPL-3

COPYING.LIB

autotests/CMakeLists.txt

cmake/modules/FindQt5PlatformSupport.cmake

cmake/modules/FindQt5ThemeSupport.cmake

src/platformtheme/CMakeLists.txt

src/platformtheme/kdeplatformtheme.h

src/platformtheme/kdeplatformtheme.cpp

src/platformtheme/kwaylandintegration.h

src/platformtheme/kwaylandintegration.cpp

src/platformtheme/qdbusmenubar.cpp

src/platformtheme/qdbusmenubar_p.h

src/platformtheme/x11integration.h

src/platformtheme/x11integration.cpp

Use DBusMenu if availableClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 10040

CMakeLists.txt

COPYING.LGPL-2

COPYING.LGPL-3

COPYING.LIB

autotests/CMakeLists.txt

cmake/modules/FindQt5PlatformSupport.cmake

cmake/modules/FindQt5ThemeSupport.cmake

src/platformtheme/CMakeLists.txt

src/platformtheme/kdeplatformtheme.h

src/platformtheme/kdeplatformtheme.cpp

src/platformtheme/kwaylandintegration.h

src/platformtheme/kwaylandintegration.cpp

src/platformtheme/qdbusmenubar.cpp

src/platformtheme/qdbusmenubar_p.h

src/platformtheme/x11integration.h

src/platformtheme/x11integration.cpp

Use DBusMenu if available
ClosedPublic
Actions

Revision Contents
Changeset List