| graesslin |
| Plasma |
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.
Attached unit test
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Looks good, just a few minor comments.
| src/client/appmenu.cpp | ||
|---|---|---|
| 179 | why toLatin1? I would expect a toUtf8? | |
| src/client/protocols/appmenu.xml | ||
| 7–18 | random suggestion: use a wayland-protocols compliant license right from the start? Just in case it ever goes upstream... | |
| src/client/registry.h | ||
| 585 | 5.XX | |
| 1473 | 5.XX, though I think you don't need to change it. I'm quite certain we make 5.41 | |
| src/server/appmenu_interface.h | ||
| 54 | 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. | |
| 79–82 | Please add some documentation. | |
| 96 | documentation missing. | |
| src/client/appmenu.cpp | ||
|---|---|---|
| 179 | 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. | |
| src/server/appmenu_interface.h | ||
| 54 | 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. | |
| src/client/appmenu.cpp | ||
|---|---|---|
| 179 | 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. | |
| src/client/protocols/appmenu.xml | ||
| 36–39 | I just noticed that this is missing documentation. The wayland protocol generator is not happy if the documentation is missing and emits warnings. | |
| src/client/protocols/appmenu.xml | ||
|---|---|---|
| 38–39 | Could you please add "The string must be encoded in latin-1". | |