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
arcpatch-D8919_1
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 ↗(On Diff #22670)

why toLatin1? I would expect a toUtf8?

src/client/protocols/appmenu.xml
6–17 ↗(On Diff #22670)

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

src/client/registry.h
585 ↗(On Diff #22670)

5.XX

1473 ↗(On Diff #22670)

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 ↗(On Diff #22670)

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 ↗(On Diff #22670)

Please add some documentation.

95 ↗(On Diff #22670)

documentation missing.

davidedmundson added inline comments.Nov 20 2017, 8:55 PM
src/client/appmenu.cpp
178 ↗(On Diff #22670)

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 ↗(On Diff #22670)

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 ↗(On Diff #22670)

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 ↗(On Diff #22670)

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
37–38

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