Review Request 120329: PlasmaShell and PlasmaSurface interfaces

[Martin Gräßlin]

> On Sept. 23, 2014, 7:51 a.m., Martin Gräßlin wrote:
> > src/client/plasma_shell.h, line 25
> > <https://git.reviewboard.kde.org/r/120329/diff/1/?file=314685#file314685line25>
> >
> >     why the QtCore/ prefix?
> 
> Pier Luigi Fiorini wrote:
>     force of habit, guess that on Qt is often done this way plus this seems the correct way for KDE too: https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes

Sounds like the policy is outdated. After the Qt5 port we didn't use it any more. In fact we have porting scripts to just remove all prefixes.


> On Sept. 23, 2014, 7:51 a.m., Martin Gräßlin wrote:
> > src/client/plasma_shell.h, line 162
> > <https://git.reviewboard.kde.org/r/120329/diff/1/?file=314685#file314685line162>
> >
> >     why is this in PlasmaShell? Shouldn't it be in the PlasmaSurface?
> 
> Pier Luigi Fiorini wrote:
>     This was meant to be used for any surface should we need to move it to arbitrary global coords.

So this is for any wl_surface from Plasma? I don't think that this is a good solution as it will expose quite some heavy work on the compositor side. I think it's better if Plasma has to create a dedicated PlasmaShellSurface for each window it wants to position.


> On Sept. 23, 2014, 7:51 a.m., Martin Gräßlin wrote:
> > src/client/plasma_surface.h, lines 29-31
> > <https://git.reviewboard.kde.org/r/120329/diff/1/?file=314687#file314687line29>
> >
> >     why wrapped in QT_BEGIN_NAMESPACE? I have never seen that in KDE code
> 
> Pier Luigi Fiorini wrote:
>     Qt can be built with a custom namespace, that macro should take care of that.

well we don't use that in KDE code.


> On Sept. 23, 2014, 7:51 a.m., Martin Gräßlin wrote:
> > src/client/plasma_surface.h, line 125
> > <https://git.reviewboard.kde.org/r/120329/diff/1/?file=314687#file314687line125>
> >
> >     I'm not sure whether we need this. Plasma is using kscreen and not QScreen.
> 
> Pier Luigi Fiorini wrote:
>     There are other components to be ported that doesn't use KScreen, for instance ksplashqml.
>     Besides without QScreen how can we know the wl_output?
>     Also, using the QScreen or Wayland backends on Plasma should allow us to map KScreen::Output to QScreen (if I recall plasmashell already do that).

> There are other components to be ported that doesn't use KScreen, for instance ksplashqml.

I don't think ksplashqml will use PlasmaSurface (why should it?). It's a perfect use case for the fullscreen shell protocol.


- Martin


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


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/c4a5d432/attachment.html>