Review Request 120329: PlasmaShell and PlasmaSurface interfaces

[Martin Gräßlin]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120329/#review67255
-----------------------------------------------------------



src/client/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120329/#comment46931>

    there is no need to add private include dirs. This should be done by adding the appropriate target_link_libraries



src/client/CMakeLists.txt
<https://git.reviewboard.kde.org/r/120329/#comment46932>

    what's the state of getting this into ECM?



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46945>

    why the QtCore/ prefix?



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46933>

    trailing whitespace



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46937>

    why is this in PlasmaShell? Shouldn't it be in the PlasmaSurface?



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46936>

    trailing whitespace



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46935>

    trailing whitespace



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46934>

    why are these wl_surface and not PlasmaShellSurface/



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46944>

    const QList<wl_surface*>&



src/client/plasma_shell.h
<https://git.reviewboard.kde.org/r/120329/#comment46943>

    const QList<Surface*>&



src/client/plasma_shell.cpp
<https://git.reviewboard.kde.org/r/120329/#comment46942>

    note: this causes the QList to detach. You want to pass in the surfaces as const&



src/client/plasma_shell.cpp
<https://git.reviewboard.kde.org/r/120329/#comment46941>

    KDE coding style requires {} for single conditions. This is a difference to the qt style



src/client/plasma_surface.h
<https://git.reviewboard.kde.org/r/120329/#comment46940>

    why wrapped in QT_BEGIN_NAMESPACE? I have never seen that in KDE code



src/client/plasma_surface.h
<https://git.reviewboard.kde.org/r/120329/#comment46939>

    I'm not sure whether we need this. Plasma is using kscreen and not QScreen.



src/client/plasma_surface.cpp
<https://git.reviewboard.kde.org/r/120329/#comment46938>

    we normally do not use Q_EMIT but emit


- Martin Gräßlin


On Sept. 23, 2014, 7:39 a.m., Pier Luigi Fiorini wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120329/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 7:39 a.m.)
> 
> 
> Review request for Plasma and Martin Gräßlin.
> 
> 
> Repository: kwayland
> 
> 
> Description
> -------
> 
> PlasmaShell and PlasmaSurface interfaces
> 
> 
> Diffs
> -----
> 
>   autotests/client/test_wayland_registry.cpp 54aa9a560153d00924d4e73c75f029ed1d1ad788 
>   src/client/CMakeLists.txt e00f4573ad22efc9b5776b5ef900854c04f8afd6 
>   src/client/plasma_shell.h PRE-CREATION 
>   src/client/plasma_shell.cpp PRE-CREATION 
>   src/client/plasma_surface.h PRE-CREATION 
>   src/client/plasma_surface.cpp PRE-CREATION 
>   src/client/registry.h 103be0aec9cae6d76c62fd32481eaaafa5a161f0 
>   src/client/registry.cpp 17d738415e395fb638751ac6429d1fc0e3ededd9 
> 
> Diff: https://git.reviewboard.kde.org/r/120329/diff/
> 
> 
> Testing
> -------
> 
> Work in progress Plasma port to Wayland.
> 
> 
> Thanks,
> 
> Pier Luigi Fiorini
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140923/920a9a01/attachment-0001.html>