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
