Details

Reviewers

davidedmundson
mart

Group Reviewers

Plasma

Commits

R871:90de42f2976c: Clean up KAppMenu
R120:90de42f2976c: Clean up KAppMenu

Summary

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

Massively cleanup KAppMenu by removing outdated and unused code. The global menu has been removed as it will eventually be provided by a plasmoid (famous last words).

The biggest change, however, is that instead of relying on Windows IDs (which won't work on Wayland) we always speak of DBus service name and object path which will be added as properties to the respective windows. To support 3rd party applications, we also set the property in the RegisterWindow function. This way KWin doesn't need any fallback code and can just rely on those properties.

Test Plan

Added the following to ~/.config/kdeglobals

[Appmenu Style]
Style=Decoration

Applications like Kwrite, Dolphin, as well as VLC start exporting their manus on DBus now. The "showMenu" method on the org.kde.kappmenu dbus interface can be used to show the application menu. The menu button also shows up for Chrome and Firefox.

The about to show hack seems to still be neccessary for Firefox and Chrome - as soon as opening a sub menu it closes in both of them. Needs further investigation, probably reintroducing the aforementioned hack. :/

Diff Detail

Repository

R120 Plasma Workspace

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 7461.Oct 17 2016, 2:30 PM

broulik retitled this revision from to RFC: Clean up KAppMenu.

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 R120 Plasma Workspace.

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

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

broulik updated this revision to Diff 7570.Oct 20 2016, 1:24 PM

broulik updated this object.

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

deleteLater() the importer - fixes activating menu items

mart accepted this revision.Oct 21 2016, 8:54 AM

mart added a reviewer: mart.

This revision is now accepted and ready to land.Oct 21 2016, 8:54 AM

Remove GtkIcons, it's not like GTK applications support this stuff anymore...
Introduce showMenu signal so KWin can reliably know whether the menu actually showed (sometimes when the menu doesn't show up for reasons, KWin would get stuck thinking it's open, so we explicitly emit a signal when we popup'd the menu)
Listen to ItemActivationRequested signal we emit in Plasma-Integration, when called with 0,0 arguments we emit a generic "show menu" request
Re-introduce Unity about to show hack but it doesn't actually fix the situation with Firefox for me :/

graesslin added a subscriber: graesslin.Oct 28 2016, 2:11 PM

graesslin added inline comments.

appmenu/appmenu.cpp
88–89	This causes a roundtrip every time it gets invoked. I suggest to cache the returned atom. You can just do something like: xcb_atom_t myAtom = XCB_ATOM_NONE; if(myAtom == XCB_ATOM_NONE) { // cookie-reply-dance myAtom = reply->atom; } if (myAtom == XCB_ATOM_NONE) { // whoops something failed with fetching the atom return; } xcb_change_property(....)

appmenu/appmenu.cpp

88–89

This causes a roundtrip every time it gets invoked. I suggest to cache the returned atom. You can just do something like:

xcb_atom_t myAtom = XCB_ATOM_NONE;
if(myAtom == XCB_ATOM_NONE) {
    // cookie-reply-dance
    myAtom = reply->atom;
}
if (myAtom == XCB_ATOM_NONE) {
    // whoops something failed with fetching the atom
    return;
}
xcb_change_property(....)

Cache atom to avoid roundtrip to the x server
Add "reconfigured" signal so interested parties (like KWin) get notified when the app menu config changes
Drop Gtk Icons mapping - it's not like GTK would support DBusMenu anymore anyway...

Ping

davidedmundson accepted this revision.Jan 5 2017, 4:03 PM

davidedmundson added a reviewer: davidedmundson.

Closed by commit R120:90de42f2976c: Clean up KAppMenu (authored by broulik). · Explain WhyJan 5 2017, 4:15 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		appmenu/CMakeLists.txt (13 lines)
M		appmenu/appmenu.h (112 lines)
M		appmenu/appmenu.cpp (413 lines)
M		appmenu/appmenu_dbus.h (29 lines)
M		appmenu/appmenu_dbus.cpp (8 lines)
D	M	appmenu/glowbar.h (51 lines)
D	M	appmenu/glowbar.cpp (109 lines)
D	M	appmenu/gtkicons.h (146 lines)
M		appmenu/kdbusimporter.h (40 lines)
D	M	appmenu/menubar.h (87 lines)
D	M	appmenu/menubar.cpp (166 lines)
D	M	appmenu/menubutton.h (54 lines)
D	M	appmenu/menubutton.cpp (85 lines)
M		appmenu/menuimporter.h (9 lines)
M		appmenu/menuimporter.cpp (92 lines)
D	M	appmenu/menuwidget.h (143 lines)
D	M	appmenu/menuwidget.cpp (387 lines)
M		appmenu/org.kde.kappmenu.xml (23 lines)
D	M	appmenu/shadows.h (51 lines)
D	M	appmenu/shadows.cpp (381 lines)
D	M	appmenu/topmenubar.h (81 lines)
D	M	appmenu/topmenubar.cpp (176 lines)
M		appmenu/verticalmenu.h (22 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	7461			Oct 17 2016, 2:28 PM	★	★
Diff 2	7570			Oct 20 2016, 1:20 PM	★	★
Diff 3	7572			Oct 20 2016, 1:26 PM	★	★
Diff 4	7727			Oct 28 2016, 1:56 PM	★	★
Diff 5	7784			Oct 31 2016, 3:54 PM	★	★
Diff 6	9767	ee24578	R120:90de42f2976c7220469d04debe185bb854c44082	Jan 5 2017, 4:13 PM	★	★

Clean up KAppMenu
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 9767

appmenu/CMakeLists.txt

appmenu/appmenu.h

appmenu/appmenu.cpp

appmenu/appmenu_dbus.h

appmenu/appmenu_dbus.cpp

appmenu/glowbar.h

appmenu/glowbar.cpp

appmenu/gtkicons.h

appmenu/kdbusimporter.h

appmenu/menubar.h

appmenu/menubar.cpp

appmenu/menubutton.h

appmenu/menubutton.cpp

appmenu/menuimporter.h

appmenu/menuimporter.cpp

appmenu/menuwidget.h

appmenu/menuwidget.cpp

appmenu/org.kde.kappmenu.xml

appmenu/shadows.h

appmenu/shadows.cpp

appmenu/topmenubar.h

appmenu/topmenubar.cpp

appmenu/verticalmenu.h

Clean up KAppMenuClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 9767

appmenu/CMakeLists.txt

appmenu/appmenu.h

appmenu/appmenu.cpp

appmenu/appmenu_dbus.h

appmenu/appmenu_dbus.cpp

appmenu/glowbar.h

appmenu/glowbar.cpp

appmenu/gtkicons.h

appmenu/kdbusimporter.h

appmenu/menubar.h

appmenu/menubar.cpp

appmenu/menubutton.h

appmenu/menubutton.cpp

appmenu/menuimporter.h

appmenu/menuimporter.cpp

appmenu/menuwidget.h

appmenu/menuwidget.cpp

appmenu/org.kde.kappmenu.xml

appmenu/shadows.h

appmenu/shadows.cpp

appmenu/topmenubar.h

appmenu/topmenubar.cpp

appmenu/verticalmenu.h

Clean up KAppMenu
ClosedPublic
Actions

Revision Contents
Changeset List