D8919: Add explicit AppMenu protocol

[Martin Flöser]

graesslin added a comment.


  Looks good, just a few minor comments.

INLINE COMMENTS

> appmenu.cpp:178
> +    Q_ASSERT(isValid());
> +    org_kde_kwin_appmenu_set_address(d->appmenu, serviceName.toLatin1(), objectPath.toLatin1());
> +}

why toLatin1? I would expect a toUtf8?

> appmenu.xml:6-17
> +    This program is free software: you can redistribute it and/or modify
> +    it under the terms of the GNU Lesser General Public License as published by
> +    the Free Software Foundation, either version 2.1 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of

random suggestion: use a wayland-protocols compliant license right from the start? Just in case it ever goes upstream...

> registry.h:585
> +     * @see createAppMenuManager
> +     * @since 5.5
> +     **/

5.XX

> registry.h:1473
> +     * @param name The name of the removed interface
> +     * @since 5.41
> +     **/

5.XX, though I think you don't need to change it. I'm quite certain we make 5.41

> appmenu_interface.h:53
> +     */
> +    AppMenuInterface* appMenuForSurface(SurfaceInterface *);
> +

a conceptional alternative could be to delegate this through the SurfaceInterface like how it was done for idleInhibit protocol. I'm fine with both approaches.

> appmenu_interface.h:78-81
> +    struct InterfaceAddress {
> +        QString serviceName;
> +        QString objectPath;
> +    };

Please add some documentation.

> appmenu_interface.h:95
> +Q_SIGNALS:
> +    void addressChanged(KWayland::Server::AppMenuInterface::InterfaceAddress);
> +

documentation missing.

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #plasma
Cc: graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171120/819cf7a0/attachment.html>