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

[Carson Black]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:5e7d26dc5e38: [libtaskmanager] Add 
ApplicationMenu{ObjectPath,ServiceName} roles to model (authored by cblack).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=78032=78033

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma, broulik
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 78032.
cblack marked 2 inline comments as done.
cblack added a comment.


  Address last feedback

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77950=78032

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma, broulik
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik accepted this revision.
broulik added a comment.
This revision is now accepted and ready to land.


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

INLINE COMMENTS

> broulik wrote in abstracttasksmodel.h:99
> `@since 5.19`

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

> xwindowtasksmodel.cpp:472
> +{
> +auto info = windowInfo(window);
> +return QString::fromUtf8(info->applicationMenuServiceName());

`const KWindowInfo *` to match the rest of the code

REPOSITORY
  R120 Plasma Workspace

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma, broulik
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 77950.
cblack marked 4 inline comments as done.
cblack added a comment.


  Address feedback

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77930=77950

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 77930.
cblack added a comment.


  Drop unused linkage

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77924=77930

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> CMakeLists.txt:60
>  Qt5::X11Extras
> +XCB::XCB
>  KF5::IconThemes)

Unused

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added a comment.


  Nice job

INLINE COMMENTS

> abstracttasksmodel.h:99
>  LastActivated,/**< The timestamp of the last time a task was the 
> active task. */
> +ApplicationMenuServiceName, /**< The DBus service name for the 
> application's menu.
> + May be empty. */

`@since 5.19`

> xwindowtasksmodel.cpp:25
>  #include "xwindowsystemeventbatcher.h"
> +#include 
>  

Unused

> xwindowtasksmodel.cpp:48
>  
> +#include "xcb/xcb.h"
> +

Unused

> xwindowtasksmodel.cpp:473
> +// returns QPair for WId window
> +QPair XWindowTasksModel::Private::appMenu(WId window)
> +{

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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 77924.
cblack added a comment.


  Use new KWindowInfo APIs

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77835=77924

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/CMakeLists.txt
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 77835.
cblack added a comment.


  Don't double lookup cached appmenus

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77834=77835

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/CMakeLists.txt
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack added a comment.


  In D27959#629133 , @broulik wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 77834.
cblack marked 5 inline comments as done.
cblack added a comment.


  Address feedback

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77681=77834

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/CMakeLists.txt
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added a comment.


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

INLINE COMMENTS

> xwindowtasksmodel.cpp:472
>  
> +static const QByteArray s_x11AppMenuServiceNamePropertyName = 
> QByteArrayLiteral("_KDE_NET_WM_APPMENU_SERVICE_NAME");
> +static const QByteArray s_x11AppMenuObjectPathPropertyName = 
> QByteArrayLiteral("_KDE_NET_WM_APPMENU_OBJECT_PATH");

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

> xwindowtasksmodel.cpp:475
> +
> +#if HAVE_X11
> +static QHash s_atoms;

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

> xwindowtasksmodel.cpp:486
> +QByteArray value;
> +if (!s_atoms.contains(name)) {
> +const xcb_intern_atom_cookie_t atomCookie = xcb_intern_atom(c, 
> false, name.length(), name.constData());

Avoid double look-up, use iterator

> xwindowtasksmodel.cpp:494
> +s_atoms[name] = atomReply->atom;
> +if (s_atoms[name] == XCB_ATOM_NONE) {
> +return value;

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

> xwindowtasksmodel.cpp:523
> +}
> +return qMakePair(QStringLiteral(""), QStringLiteral(""));
> +}

Use null `QString()`

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack updated this revision to Diff 77681.
cblack added a comment.


  Add X11 data to model

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27959?vs=77322=77681

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/CMakeLists.txt
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack added a comment.


  Ping.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack added a comment.


  In D27959#625053 , @broulik wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack added a comment.


  In D27959#625055 , @broulik wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D27959

To: cblack, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

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

[Carson Black]

cblack created this revision.
cblack added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
cblack requested review of this revision.

REVISION SUMMARY
  The tasks model now exposes a window's application menu object path and 
service name.
  Model only implemented for Wayland; as I don't have a clue what to do on XOrg.

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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  cblack/appmenu-libtaskmanager (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D27959

AFFECTED FILES
  libtaskmanager/abstracttasksmodel.h
  libtaskmanager/waylandtasksmodel.cpp
  libtaskmanager/xwindowtasksmodel.cpp

To: cblack, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart