This patch adds an interface allowing a compositor to send
the service name and object path of a PlasmaWindow's application menu
to the client.
Details
Details
Diff Detail
Diff Detail
- Repository
- R127 KWayland
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Good start. The direction makes sense.
display.cpp and registry.cpp need updating to the new version number. Otherwise we won't be able to use this new method.
It'd be great if you could update the unit test too.
| src/server/plasmawindowmanagement_interface.cpp | ||
|---|---|---|
| 599 | We're mixing up our local 8bit and utf8 on different sides of this. | |
Comment Actions
Pardon, but I don't see anything in display/registry.cpp that looks like it takes a version number for the org_kde_plasma_window interface. I only see okp_windowmanagement in registry.cpp. Am I missing something?
Comment Actions
This is a nice simple example of something with a version bump: https://phabricator.kde.org/D27535 for where to change registry.
I was wrong about display, there is a number you need to change, but apparently it's on v9 already, so you don't need to do anything.
| src/server/plasmawindowmanagement_interface.cpp | ||
|---|---|---|
| 129 | how is this 9 already? | |
| src/server/plasmawindowmanagement_interface.cpp | ||
|---|---|---|
| 129 | Git blame shows that it's been 9 since 2018. Looks like someone made a mistake a while back? | |
Comment Actions
Would the version number already being 9 in old versions of KWayland cause any issues? I have a feeling old KWayland versions may not like it when they're sent events they don't recognize, unless libwayland/KWayland handle that usecase.
| src/client/protocols/plasma-window-management.xml | ||
|---|---|---|
| 20 | version="10" | |
| 87 | version="10" | |
| 312 | and this one should be <!-- Version 10 additions --> <event name="application_menu_changed" since="10"> | |
| src/server/plasmawindowmanagement_interface.cpp | ||
| 129 | Urgh, it seems like versioning in the plasma window management protocol is messed up. For some reason, org_kde_plasma_window_management and org_kde_plasma_window have different versions. | |
| src/client/plasmawindowmanagement.cpp | ||
|---|---|---|
| 363 | Coding style: put whitespace before * and keep using snake_case. void PlasmaWindow::Private::appmenuChangedCallback(void *data, org_kde_plasma_window *window, const char *service_name, const char *object_path) | |
| src/client/protocols/plasma-window-management.xml | ||
| 312 | Also, please drop "_changed". | |
| src/server/plasmawindowmanagement_interface.h | ||
| 234 | Coding style: put whitespace before & and use camelCase. void setApplicationMenuPaths(const QString &serviceName, const QString &objectPath); | |
| src/server/plasmawindowmanagement_interface.cpp | ||
|---|---|---|
| 85 | You forgot to fix coding style here ;-) | |
Comment Actions
Looks good to me, but please don't land without corresponding KWin patch.
| src/client/plasmawindowmanagement.h | ||
|---|---|---|
| 700 | You missed this one. | |
Comment Actions
Thanks. Even though the patch is accepted, please do not land it yet. https://phabricator.kde.org/D27464#621521
Comment Actions
Didn't we have a dedicated protocol for window menu in plasma-integration? Or is this just for reading? We surely don't want all apps to use plasma surface interface for announcing the global menu?
https://cgit.kde.org/plasma-integration.git/tree/src/platformtheme/kwaylandintegration.cpp
Or was that still using that custom Qt surface protocol that is being discontinued?
Comment Actions
That protocol is for announcing application menus.
Or is this just for reading?
Yes, it's for reading.
We surely don't want all apps to use plasma surface interface for announcing the global menu?
Could you please clarify what you mean by "plasma surface interface"?
Comment Actions
Okay, let's step back. We have a proprietary protocol that is used to announce application menus. The problem is that only kwin knows that a particular window has that particular application menu. We need a way to share this information with a global menu applet.
We surely don't want all apps to use plasma surface interface for announcing the global menu?
Could you please explain why we need to announce global menus?