Add explicit AppMenu protocol
ClosedPublic

Authored by davidedmundson on Nov 20 2017, 4:49 PM.

Details

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

Diff Detail

Repository
R127 KWayland
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
davidedmundson created this revision.Nov 20 2017, 4:49 PM
Restricted Application added projects: Plasma on Wayland, Frameworks. · View Herald TranscriptNov 20 2017, 4:49 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript

Looks good, just a few minor comments.

src/client/appmenu.cpp
178

why toLatin1? I would expect a toUtf8?

src/client/protocols/appmenu.xml
6–17

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
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.

78–81

Please add some documentation.

95

documentation missing.

davidedmundson added inline comments.Nov 20 2017, 8:55 PM
src/client/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.

src/server/appmenu_interface.h
53

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.

graesslin added inline comments.Nov 20 2017, 9:01 PM
src/client/appmenu.cpp
178

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
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.

davidedmundson marked 8 inline comments as done.

Docs++

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptNov 24 2017, 12:21 PM
graesslin added inline comments.Nov 25 2017, 6:22 PM
src/client/protocols/appmenu.xml
38–39

Could you please add "The string must be encoded in latin-1".

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptNov 25 2017, 6:22 PM

Add line in docs

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptNov 28 2017, 12:19 PM
graesslin accepted this revision.Nov 29 2017, 5:29 AM
This revision is now accepted and ready to land.Nov 29 2017, 5:29 AM
Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptNov 29 2017, 5:29 AM
This revision was automatically updated to reflect the committed changes.
Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptDec 18 2017, 10:08 PM