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
