Review Request 126301: Add protocol for server side decoration
Sebastian Kügler
sebas at kde.org
Fri Dec 11 13:54:39 UTC 2015
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126301/#review89348
-----------------------------------------------------------
src/client/protocols/server-decoration.xml (line 22)
<https://git.reviewboard.kde.org/r/126301/#comment61093>
server-side (probably relevant in other places, please grep and replace :))
src/client/protocols/server-decoration.xml (line 31)
<https://git.reviewboard.kde.org/r/126301/#comment61094>
... or will provide client-side decorations.
src/client/protocols/server-decoration.xml (line 48)
<https://git.reviewboard.kde.org/r/126301/#comment61096>
docs/descriptions for this enum?
Also, unhappiness with the too generic name "mode".
src/client/protocols/server-decoration.xml (line 54)
<https://git.reviewboard.kde.org/r/126301/#comment61095>
s/decoration/surface ?
src/client/protocols/server-decoration.xml (line 61)
<https://git.reviewboard.kde.org/r/126301/#comment61097>
client-side
src/client/server_decoration.h (line 70)
<https://git.reviewboard.kde.org/r/126301/#comment61089>
ready-to-use
src/client/server_decoration.h (line 179)
<https://git.reviewboard.kde.org/r/126301/#comment61090>
apidocs missing
It would be useful to explain the modes here, and possibly mention than None means that the server expects the client to do CSD.
src/client/server_decoration.h (line 187)
<https://git.reviewboard.kde.org/r/126301/#comment61091>
I still don't like this "Mode", but it's probably proximity-bias of the output sort. Decorations have nothing to do with mode settings. Using the same name makes grepping harder.
decorationMode or decoratorMode would IMO be better.
src/client/server_decoration.cpp (line 154)
<https://git.reviewboard.kde.org/r/126301/#comment61092>
qCWarning()?
src/server/CMakeLists.txt (line 100)
<https://git.reviewboard.kde.org/r/126301/#comment61098>
I don't get the name here, maybe surface_decoration would be better? (It's not about the server, it's about decoration a surface.)
Naming of course bleeds all over the whole protocol, so consider this a generic comment. :)
src/server/display.cpp (line 330)
<https://git.reviewboard.kde.org/r/126301/#comment61099>
Also, I'm confused.
you connect to d, so d ends up being [this], which you then don't use, but you use d, and delete that?
Please revisit so some muggle like me understands this. :)
src/server/server_decoration_interface.h (line 37)
<https://git.reviewboard.kde.org/r/126301/#comment61100>
APIDOX missing for the whole class.
src/server/server_decoration_interface.h (line 62)
<https://git.reviewboard.kde.org/r/126301/#comment61101>
APIDOX missing.
src/server/server_decoration_interface.cpp (line 75)
<https://git.reviewboard.kde.org/r/126301/#comment61104>
Perhaps qCWarning() for now?
src/server/server_decoration_interface.cpp (line 198)
<https://git.reviewboard.kde.org/r/126301/#comment61109>
qCWarning()?
- Sebastian Kügler
On Dec. 10, 2015, 2:03 p.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126301/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 2:03 p.m.)
>
>
> Review request for Plasma and Sebastian Kügler.
>
>
> Repository: kwayland
>
>
> Description
> -------
>
> [client] Add implementation for ServerSideDecoration
>
>
> [server] Add implementation for server side decoration protocol
>
>
> [autotest] Add tests for ServerSideDecoration protocol
>
>
> Diffs
> -----
>
> autotests/client/test_wayland_registry.cpp 772da821d634efa1c72d13a7c081994bd78ab7fd
> autotests/client/CMakeLists.txt 014b5e0798618394ecf11c8b8cfa796dcf9c37f3
> autotests/client/test_server_side_decoration.cpp PRE-CREATION
> src/client/CMakeLists.txt 1d2b5492954e07fc77b2209a123ef8e43e340e8a
> src/client/protocols/server-decoration.xml PRE-CREATION
> src/client/registry.h c079852a4d96471face2a06795a531abf2e4d8c0
> src/client/registry.cpp 71640e184da4699bdc27a993b710b1f761c919d2
> src/client/server_decoration.h PRE-CREATION
> src/client/server_decoration.cpp PRE-CREATION
> src/server/CMakeLists.txt cd5c7bb1a93a470fd58cb9c6d86068da961addeb
> src/server/display.h 154e0f0b4c00375ca7d7f0a980392877fe743f50
> src/server/display.cpp 81ea7316e3b77f6db95748339d3dfaa303333d33
> src/server/server_decoration_interface.h PRE-CREATION
> src/server/server_decoration_interface.cpp PRE-CREATION
> src/tools/mapping.txt 9dc8204d65092aa86f6ced8ae5a131a2f89018d0
>
> Diff: https://git.reviewboard.kde.org/r/126301/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Martin Gräßlin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20151211/bac88737/attachment-0001.html>
More information about the Plasma-devel
mailing list