Review Request 124947: [screenlocker] Share QQmlEngine between all views in the greeter

[Martin Gräßlin]

> On Aug. 27, 2015, 12:01 p.m., Mark Gaiser wrote:
> > ksmserver/screenlocker/greeter/greeterapp.cpp, line 140
> > <https://git.reviewboard.kde.org/r/124947/diff/1/?file=399227#file399227line140>
> >
> >     It might be nice to check if this pointer isn't 0 after the cast.
> >     
> >     Also, coding style.
> >     qobject_cast<KQuickAddons::QuickViewSharedEngine*>
> >     
> >     instead of 
> >     qobject_cast<KQuickAddons::QuickViewSharedEngine *>
> >     
> >     Or the other way around, but then all the others need to change ;)
> >     Not quite sure which one is appropiate here, the style guideline isn't clear on this. I think the one with the space is the adviced way (which would mean that the rest needs to change, not this line)

Yes in theory one needs to check. In practice the slot is only invoked from a signal emitted from the view. So yes in theory I agree, in practice I don't think it's needed.


> On Aug. 27, 2015, 12:01 p.m., Mark Gaiser wrote:
> > ksmserver/screenlocker/greeter/greeterapp.cpp, line 165
> > <https://git.reviewboard.kde.org/r/124947/diff/1/?file=399227#file399227line165>
> >
> >     Just a suggestion, not an issue. Can't this (and m_views where this one is inserted later on) be a smart pointer? That makes it go out of scope instead of relying on qDeleteAll(m_views) which is done in the destructor. Both methods work just fine, but i - personal preference - prefer smart pointers :)

I also prefer smart pointers, but I don't think it's a good idea in this case. I do not like to needlessly change security relevant code for things that work.


On Aug. 27, 2015, 12:01 p.m., Martin Gräßlin wrote:
> > I can't really give a +1.. I just don't know this or the implications it might have.
> > So just a style review then :)

Concerning the coding style: I don't want to mix it with the changes. If you look closely you will notice that in all outlined cases the coding style did not change. The coding style of that class is inconsistent. If you think it should be fixed I suggest you run astyle on it after my changes are in.


- Martin


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


On Aug. 27, 2015, 10:34 a.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124947/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2015, 10:34 a.m.)
> 
> 
> Review request for Plasma and David Edmundson.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> In a multi-screen setup we have multiple views showing the same Qml
> scene. Let's share the engine for all views.
> 
> 
> Diffs
> -----
> 
>   ksmserver/screenlocker/greeter/CMakeLists.txt 4fb679f838a80f11eb8e4b4f5cb5ef04dc39c0f7 
>   ksmserver/screenlocker/greeter/greeterapp.h ed278e492a9a237f65f4c1600360f84fb25bdad7 
>   ksmserver/screenlocker/greeter/greeterapp.cpp b500ba44c2b483d7372ca46840152c90ef5f798c 
> 
> Diff: https://git.reviewboard.kde.org/r/124947/diff/
> 
> 
> Testing
> -------
> 
> This makes creating a second view basically free (from timing perspective)
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150827/12dd1b51/attachment.html>