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<QByteArray, xcb_atom_t> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200317/e7e0324b/attachment.html>