This brings back global menu support in KWin. The DBusMenu infrastructure is different that we just read the DBus service name and menu object path from the windows rather than passing around window IDs on DBus which won't work on Wayland.
Details
Details
- Reviewers
graesslin - Group Reviewers
Plasma - Commits
- R108:93938d60b84d: Restore global menu support
Together with the other three patches gives you an application menu button in the title bar.
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| CMakeLists.txt | ||
|---|---|---|
| 276–278 | I'm for not having an option, just always build enable. | |
| 453–454 | we won't be able to depend on plasma-workspace as plasma-workspace already depends on KWin. So either the org.kde.kappmenu.xml needs to be copied to KWin or kappmenu needs to go into an own repo which both kwin and plasma-workspace can depend on. | |
| appmenu.h | ||
| 46 | Even if it's just bringing the code back: an xcb_window_t in the api is a bad idea considering wayland | |
| 49–51 | same here, wid is evil | |
| 55 | and of course this one is also bad | |
Comment Actions
- Rebase on master
- Implement ShellClient so it reads the property from Wayland windows
The menu button shows up on Wayland but the menu itself doesn't - the menu is properly exported, though, I can access the menu from my applet just fine, as long as I know the DBus service name and object path, that is.
| abstract_client.h | ||
|---|---|---|
| 667 | is that a signal (context is missing in diff)? If yes it sounds more like a slot. | |
| 1017 | default init missing | |
| appmenu.cpp | ||
| 69–72 | careful here: that will only find (X11) Clients but not ShellClient. You might want a variant which finds AbstractClients. | |
| client.cpp | ||
| 2150–2151 | this causes two roundtrips to the X server. Please split into variants which reduce to one roundtrip in the case of the update (from events.cpp) and to zero roundtrips in case of manage. | |
| client.h | ||
| 334 | I don't like that it has the same name as a method in parent class | |
| shell_client.h | ||
| 138 | I don't like that it has the same name as a method in parent class | |
Comment Actions
- Cleanup a bit
- Listen to menuShown signal rather than assuming a showMenu call will be successful
- Listen to showRequest signal when an application wants the menu to show
- Implement Wayland support
Comment Actions
- Rebase to QtPlatformSupport cmake changes - cannot test with Qt 5.8
- Ship a copy of kappmenu.xml
Comment Actions
- Rebase on master
- Honor global menu setting (only show button if is "in window decoration") and listen to kappmenu reconfigured signal
Comment Actions
- Don't blindly connect applicationMenu*Enabled*Changed to hasApplicationMenuChanged because the former is just the setting but a client doesn't neccessarily have an application menu
Comment Actions
One more general thing: I'd love to see a test case for the new added code.
| abstract_client.cpp | ||
|---|---|---|
| 75–77 ↗ | (On Diff #7787) | why carry the bool in the signal? That seems to make it way more complicated here |
| 1715 ↗ | (On Diff #7787) | that comment should be together with the else, shouldn't it? |
| abstract_client.h | ||
| 1043 ↗ | (On Diff #7787) | added empty new line |
| appmenu.cpp | ||
| 102–107 ↗ | (On Diff #7787) | add a safety check in case the serviceName and/or menuObjectPath is empty and return null? |
| abstract_client.cpp | ||
|---|---|---|
| 75–77 ↗ | (On Diff #7787) | In DecoratedClient I do connect(client, &AbstractClient::hasApplicationMenuChanged, decoratedClient, &KDecoration2::DecoratedClient::hasApplicationMenuChanged); So either I need it here or there. |
Comment Actions
- Move comment
- Check for service name / object path being empty before finding a client
- Remove added empty line
Comment Actions
- Route through the "actionId" argument - this way an application can tell which submenu to highlight/open (e.g. Alt keyboard activation)
Comment Actions
For the KCM I needed
diff --git a/kcmkwin/kwindecoration/declarative-plugin/buttonsmodel.cpp b/kcmkwin/kwindecoration/declarative-plugin/buttonsmodel.cpp
index b2ac124ed..26d779015 100644
- a/kcmkwin/kwindecoration/declarative-plugin/buttonsmodel.cpp
+++ b/kcmkwin/kwindecoration/declarative-plugin/buttonsmodel.cpp
@@ -38,6 +38,7 @@ ButtonsModel::ButtonsModel(const QVector< DecorationButtonType > &buttons, QObje
ButtonsModel::ButtonsModel(QObject* parent)
: ButtonsModel(QVector<DecorationButtonType>({
DecorationButtonType::Menu,+ DecorationButtonType::ApplicationMenu,
DecorationButtonType::OnAllDesktops, DecorationButtonType::Minimize, DecorationButtonType::Maximize,
to allow me to add the menu in the decoration, is this deliberately not available?
| kcmkwin/kwindecoration/declarative-plugin/previewclient.cpp | ||
|---|---|---|
| 314 ↗ | (On Diff #7787) | why? We'd want to see how it looks in the preview? |
Comment Actions
to allow me to add the menu in the decoration, is this deliberately not available?
No, I just haven't looked at the decorations KCM at all yet. I think we should also add the menu button to the default button order so that when you enable it you get it automatically.
| kcmkwin/kwindecoration/declarative-plugin/previewclient.cpp | ||
|---|---|---|
| 314 ↗ | (On Diff #7787) | Right. Again, I haven't looked at the KCM yet, I probably would have noticed the button missing then :D |
| appmenu.cpp | ||
|---|---|---|
| 99 ↗ | (On Diff #7787) | please drop that debug message :-) |