D8919: Add explicit AppMenu protocol
This revision was automatically updated to reflect the committed changes. Closed by commit R127:a0180f387eaf: Add explicit AppMenu protocol (authored by davidedmundson). Restricted Application edited projects, added Plasma; removed Plasma on Wayland. CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D8919?vs=23083=24087#toc REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8919?vs=23083=24087 REVISION DETAIL https://phabricator.kde.org/D8919 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_wayland_appmenu.cpp autotests/client/test_wayland_surface.cpp src/client/CMakeLists.txt src/client/appmenu.cpp src/client/appmenu.h src/client/protocols/appmenu.xml src/client/registry.cpp src/client/registry.h src/server/CMakeLists.txt src/server/appmenu_interface.cpp src/server/appmenu_interface.h src/server/display.cpp src/server/display.h To: davidedmundson, #plasma, graesslin Cc: broulik, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8919: Add explicit AppMenu protocol
graesslin accepted this revision. This revision is now accepted and ready to land. Restricted Application edited projects, added Plasma on Wayland; removed Plasma. REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D8919 To: davidedmundson, #plasma, graesslin Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
D8919: Add explicit AppMenu protocol
davidedmundson updated this revision to Diff 23083. davidedmundson added a comment. Restricted Application edited projects, added Plasma; removed Plasma on Wayland. Add line in docs REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8919?vs=22890=23083 BRANCH master REVISION DETAIL https://phabricator.kde.org/D8919 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_wayland_appmenu.cpp src/client/CMakeLists.txt src/client/appmenu.cpp src/client/appmenu.h src/client/protocols/appmenu.xml src/client/registry.cpp src/client/registry.h src/server/CMakeLists.txt src/server/appmenu_interface.cpp src/server/appmenu_interface.h src/server/display.cpp src/server/display.h To: davidedmundson, #plasma Cc: broulik, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8919: Add explicit AppMenu protocol
graesslin added inline comments. Restricted Application edited projects, added Plasma on Wayland; removed Plasma. INLINE COMMENTS > appmenu.xml:37-38 > + > + > + > + Could you please add "The string must be encoded in latin-1". REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D8919 To: davidedmundson, #plasma Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
D8919: Add explicit AppMenu protocol
davidedmundson updated this revision to Diff 22890. davidedmundson marked 8 inline comments as done. davidedmundson added a comment. Restricted Application edited projects, added Plasma; removed Plasma on Wayland. Docs++ REPOSITORY R127 KWayland CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8919?vs=22670=22890 BRANCH arcpatch-D8919_1 REVISION DETAIL https://phabricator.kde.org/D8919 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_wayland_appmenu.cpp src/client/CMakeLists.txt src/client/appmenu.cpp src/client/appmenu.h src/client/protocols/appmenu.xml src/client/registry.cpp src/client/registry.h src/server/CMakeLists.txt src/server/appmenu_interface.cpp src/server/appmenu_interface.h src/server/display.cpp src/server/display.h To: davidedmundson, #plasma Cc: broulik, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D8919: Add explicit AppMenu protocol
graesslin added inline comments. INLINE COMMENTS > davidedmundson wrote in appmenu.cpp:178 > Wayland strings can be UTF-8 but DBus service names and object paths can only > be ASCII. > > I figured it was best to avoid any potential locale issues. Fair enough. I wasn't aware of the DBus restriction. I suggest to add it to the protocol definition. E.g. in Wayland protocols we find: "The string must be encoded in UTF-8. " So I suggest to do something similar, just to be sure. > appmenu.xml:35-38 > + > + > + > + I just noticed that this is missing documentation. The wayland protocol generator is not happy if the documentation is missing and emits warnings. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D8919 To: davidedmundson, #plasma Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
D8919: Add explicit AppMenu protocol
davidedmundson added inline comments. INLINE COMMENTS > graesslin wrote in appmenu.cpp:178 > why toLatin1? I would expect a toUtf8? Wayland strings can be UTF-8 but DBus service names and object paths can only be ASCII. I figured it was best to avoid any potential locale issues. > graesslin wrote in appmenu_interface.h:53 > 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. I'm also fine with either, kwayland has a bit of both. This mostly copies the ServerSideDecoration style. My rationale was that this isn't double-buffered against the surface commits, so didn't proxy it, but I don't mind either way. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D8919 To: davidedmundson, #plasma Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
D8919: Add explicit AppMenu protocol
broulik added a comment. lgtm REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D8919 To: davidedmundson, #plasma Cc: broulik, graesslin, plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
D8919: Add explicit AppMenu protocol
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
D8919: Add explicit AppMenu protocol
davidedmundson created this revision. davidedmundson added a reviewer: Plasma. Restricted Application added projects: Plasma on Wayland, Frameworks. Restricted Application added subscribers: Frameworks, plasma-devel. REVISION SUMMARY A protocol that attaches to a surface and contains two strings which can change. The intended use is for clients to link a DBus Appmenu object with a surface. This is in preparation for the Qt Extended Surface deprecation which currently handles this in Kwin. TEST PLAN Attached unit test REPOSITORY R127 KWayland BRANCH master REVISION DETAIL https://phabricator.kde.org/D8919 AFFECTED FILES autotests/client/CMakeLists.txt autotests/client/test_wayland_appmenu.cpp src/client/CMakeLists.txt src/client/appmenu.cpp src/client/appmenu.h src/client/protocols/appmenu.xml src/client/registry.cpp src/client/registry.h src/server/CMakeLists.txt src/server/appmenu_interface.cpp src/server/appmenu_interface.h src/server/display.cpp src/server/display.h To: davidedmundson, #plasma Cc: plasma-devel, #frameworks, leezu, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
