[Differential] [Commented On] D3089: WIP: Restore global menu support

[graesslin (Martin Gräßlin)]

graesslin added inline comments.

INLINE COMMENTS

> abstract_client.h:667
> +    void applicationMenuActiveChanged(bool);
> +    void requestShowApplicationMenu();
>  

is that a signal (context is missing in diff)? If yes it sounds more like a slot.

> abstract_client.h:1017
> +
> +    bool m_applicationMenuActive;
> +    QString m_applicationMenuServiceName;

default init missing

> appmenu.cpp:68-71
> +    return Workspace::self()->findClient([&](const Client *c) {
> +        return c->applicationMenuServiceName() == serviceName
> +        && c->applicationMenuObjectPath() == menuObjectPath.path();
> +    });

careful here: that will only find (X11) Clients but not ShellClient. You might want a variant which finds AbstractClients.

> client.cpp:2150-2151
> +{
> +    const QString &serviceName = QString::fromUtf8(Xcb::StringProperty(m_client, atoms->kde_net_wm_appmenu_service_name));
> +    const QString &objectPath = QString::fromUtf8(Xcb::StringProperty(m_client, atoms->kde_net_wm_appmenu_object_path));
> +

this causes two roundtrips to the X server. Please split into variants which reduce to one roundtrip in the case of the update  (from events.cpp) and to zero roundtrips in case of manage.

> client.h:334
>  
> +    void updateApplicationMenu();
> +

I don't like that it has the same name as a method in parent class

> shell_client.h:138
>  
> +    void updateApplicationMenu();
> +

I don't like that it has the same name as a method in parent class

REPOSITORY
  rKWIN KWin

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: graesslin, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161026/e861aa06/attachment-0001.html>