[libtaskmanager] Add ApplicationMenu{ObjectPath,ServiceName} roles to model
ClosedPublic

Authored by cblack on Mar 9 2020, 10:11 PM.

Details

Summary

The tasks model now exposes a window's application menu object path and service name.

No update signals set up for XOrg; not sure what to listen for.

Test Plan

See that the model has more roles and see that they're wired to the application
menu obiect path and service name on Wayland and XOrg.

Diff Detail

Repository
R120 Plasma Workspace
Branch
cblack/appmenu-libtaskmanager (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23820
Build 23838: arc lint + arc unit
cblack created this revision.Mar 9 2020, 10:11 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 9 2020, 10:11 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Mar 9 2020, 10:11 PM

Can we make the dbus service role name more generic? GTK has a generic DBus prop on the window, too.

broulik added a comment.EditedMar 9 2020, 10:22 PM

For X check out the appmenu applet: https://cgit.kde.org/plasma-workspace.git/tree/applets/appmenu/plugin/appmenumodel.cpp#n164

I was wondering if we should maybe add that to NETWM or KWindowInfo instead of doing lowlevel X calls in libTM, which I don't think it does at this point

For X check out the appmenu applet: https://cgit.kde.org/plasma-workspace.git/tree/applets/appmenu/plugin/appmenumodel.cpp#n164

I was wondering if we should maybe add that to NETWM or KWindowInfo instead of doing lowlevel X calls in libTM, which I don't think it does at this point

I suppose you missed the FIXME comment where I literally say to check that file out?

But yeah, the low level X calls are pretty ugly and the reason why I left it a FIXME.

Can we make the dbus service role name more generic? GTK has a generic DBus prop on the window, too.

Both the org.gtk.Actions and the org.gtk.Menus dbus interfaces exposed by GIO's GApplication class are primarily used for application menus.

cblack updated this revision to Diff 77681.Mar 15 2020, 8:32 PM

Add X11 data to model

cblack edited the summary of this revision. (Show Details)Mar 15 2020, 8:33 PM
cblack edited the test plan for this revision. (Show Details)

I think you should cache the X props, otherwise every data access causes a round trip to the X server.

libtaskmanager/xwindowtasksmodel.cpp
472

We generally want to avoid global statics in library code. In the applet it's fine because it's a plugin that is loaded on demand

475

Is this check needed? I would think xwindowtasksmodel is guarded in its entirety both compile-time and run-time.

486

Avoid double look-up, use iterator

494

Avoid double hash look-up. Store atom in variable and check it

523

Use null QString()

cblack updated this revision to Diff 77834.Mar 17 2020, 3:12 PM
cblack marked 5 inline comments as done.

Address feedback

I think you should cache the X props, otherwise every data access causes a round trip to the X server.

Implemented a cache for appmenus, but as I don't know what to listen to for appmenu updates, I haven't implemented cache invalidation.

cblack updated this revision to Diff 77835.Mar 17 2020, 3:16 PM

Don't double lookup cached appmenus

I think if you added those properties KWindowInfo, you'd get change events, cache eviction, less manual X code basically for free :)

cblack updated this revision to Diff 77924.Mar 18 2020, 2:48 PM

Use new KWindowInfo APIs

Nice job

libtaskmanager/abstracttasksmodel.h
99

@since 5.19

libtaskmanager/xwindowtasksmodel.cpp
25

Unused

48

Unused

473

I think this should be split into two dedicated methods to match the rest

broulik added inline comments.Mar 18 2020, 2:59 PM
libtaskmanager/CMakeLists.txt
60

Unused

cblack updated this revision to Diff 77930.Mar 18 2020, 3:42 PM

Drop unused linkage

cblack updated this revision to Diff 77950.Mar 18 2020, 6:12 PM
cblack marked 4 inline comments as done.

Address feedback

broulik accepted this revision.Mar 19 2020, 9:10 AM

Fix those two minor nitpicks and then ship it. Well done!

libtaskmanager/abstracttasksmodel.h
99

This is plasma-workspace, not plasma-framework, so 5.19 it is.

libtaskmanager/xwindowtasksmodel.cpp
474

const KWindowInfo * to match the rest of the code

This revision is now accepted and ready to land.Mar 19 2020, 9:10 AM
cblack updated this revision to Diff 78032.Mar 19 2020, 5:32 PM
cblack marked 2 inline comments as done.

Address last feedback

This revision was automatically updated to reflect the committed changes.