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

[Mark Gaiser]

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



ksmserver/screenlocker/greeter/greeterapp.cpp (line 140)
<https://git.reviewboard.kde.org/r/124947/#comment58460>

    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)



ksmserver/screenlocker/greeter/greeterapp.cpp (line 165)
<https://git.reviewboard.kde.org/r/124947/#comment58461>

    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 :)



ksmserver/screenlocker/greeter/greeterapp.cpp (line 346)
<https://git.reviewboard.kde.org/r/124947/#comment58462>

    coding style.
    
    QuickViewSharedEngine *view
    
    instead of 
    QuickViewSharedEngine * view



ksmserver/screenlocker/greeter/greeterapp.cpp (line 350)
<https://git.reviewboard.kde.org/r/124947/#comment58463>

    idem


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 :)

- Mark Gaiser


On aug 27, 2015, 8: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, 8: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/6b43ef71/attachment.html>