Add application menu dbus paths to org_kde_plasma_window interface
ClosedPublic

Authored by cblack on Feb 17 2020, 6:21 PM.

Details

Summary

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.

Diff Detail

Repository
R127 KWayland
Branch
cblack/appmenu-listener
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 22594
Build 22612: arc lint + arc unit
cblack created this revision.Feb 17 2020, 6:21 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 17 2020, 6:21 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
cblack requested review of this revision.Feb 17 2020, 6:21 PM
cblack planned changes to this revision.Feb 17 2020, 6:22 PM

whoops, I seem to have forgotten how to use arc

cblack updated this revision to Diff 75855.Feb 17 2020, 6:24 PM

Figure out how to use arc correctly

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.

cblack updated this revision to Diff 76035.Feb 20 2020, 2:21 AM

Use utf8 encoding on client side

cblack marked an inline comment as done.Feb 20 2020, 2:21 AM

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.

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?

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?

cblack added inline comments.Feb 24 2020, 9:12 PM
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?

cblack added a comment.Mar 1 2020, 9:05 PM

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.

zzag added inline comments.Mar 2 2020, 8:20 AM
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.

zzag requested changes to this revision.Mar 2 2020, 11:30 AM
zzag added inline comments.
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);
This revision now requires changes to proceed.Mar 2 2020, 11:30 AM
cblack updated this revision to Diff 76804.Mar 2 2020, 9:20 PM

Address feedback

cblack marked 6 inline comments as done.Mar 2 2020, 9:21 PM
cblack updated this revision to Diff 76805.Mar 2 2020, 9:21 PM

Tick up PWM version

cblack marked 3 inline comments as done.Mar 2 2020, 9:22 PM
zzag added inline comments.Mar 2 2020, 9:35 PM
src/server/plasmawindowmanagement_interface.cpp
85

You forgot to fix coding style here ;-)

cblack updated this revision to Diff 76811.Mar 2 2020, 10:46 PM

Coding style

cblack marked an inline comment as done.Mar 2 2020, 10:47 PM
davidedmundson accepted this revision.Mar 3 2020, 9:54 AM
cblack added a comment.Mar 3 2020, 6:59 PM

@zzag does anything else look off to you?

zzag added inline comments.Mar 3 2020, 9:21 PM
src/client/plasmawindowmanagement.cpp
372

Please move it to the top of the method and remove the semicolon.

src/client/plasmawindowmanagement.h
526–527

Please add documentation.

cblack updated this revision to Diff 76885.Mar 3 2020, 9:35 PM

Address feedback

zzag added a comment.Mar 3 2020, 9:36 PM

Looks good to me, but please don't land without corresponding KWin patch.

src/client/plasmawindowmanagement.h
688

You missed this one.

cblack updated this revision to Diff 76886.Mar 3 2020, 9:38 PM

Add missing documentation

zzag accepted this revision.Mar 3 2020, 9:45 PM

Thanks. Even though the patch is accepted, please do not land it yet. https://phabricator.kde.org/D27464#621521

This revision is now accepted and ready to land.Mar 3 2020, 9:45 PM
This revision was automatically updated to reflect the committed changes.

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?

zzag added a comment.EditedMar 10 2020, 3:57 PM

Didn't we have a dedicated protocol for window menu in plasma-integration?

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"?

In D27464#625405, @zzag wrote:

Could you please clarify what you mean by "plasma surface interface"?

The thing we use in plasmashell to position panels and everything absolutely

zzag added a comment.Mar 10 2020, 4:21 PM

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?

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?