Review Request: Added support to multiple render threads to Plasma::Wallpaper.

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3092/#review4348
-----------------------------------------------------------


"This patch adds support to multiple render threads in Plasma::Wallpaper"

sounds like it could be useful; is there a use case in particular that you were working on that this improves?

"and fixes a small bug that has been happening when there wasn't any wallpaper in the wallpaper cache."

what was the fix? (i didn't notice it in the patch :)


/trunk/KDE/kdelibs/plasma/private/wallpaper_p.h
<http://reviewboard.kde.org/r/3092/#comment3826>

    static members should be prefixed with s_ so they are easily identified in the code as such.



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3835>

    the bookkeeping around the "currentRenderer" seems very prone to error as it needs to be kept in sync with the queueing. instead of keeping the current renderer around, how about including a pointer to it in the WallpaperRenderThread::done signal? we already have the render token, so we don't need to care about identifying the WallpaperRenderThread object itself in the renderCompleted slot, it's only needed there to do some cleanup bookkeeping.



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3828>

    spacing: } else if (..) {



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3827>

    spacing: } else {



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3834>

    this can easily lead to unnecessary rendering; if Wallpaper::render(..) is called repeatedly in quick succession, each of those calls will potentially result in a request that is queued which will then be rendered later.
    
    instead of appending a new item to the queue everytime, if the queue is not empty it's probably better to go through and check to see if any renders are already queued for this object (request.parent == this) and if so just modify that queued request instead of adding a new one.



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3829>

    spacing; would also be nice to flip the if/else for readability so this is if (renderQueue.isEmpty()) { } else {}



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3832>

    this code is dangerous: if a Wallpaper object is created, it queues a rendering, then is deleted before that rendering happens, request.parent will be a dangling pointer and this will crash.
    
    parent should probably be a QWeakPointer and the value checked for before using it.



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3833>

    this should probably be explicitly marked as being a Qt::UniqueConnection to prevent multiple connections of the same renderer to a Wallpaper object, which could potentially happen if the connection is made from another Wallpaper object.



/trunk/KDE/kdelibs/plasma/wallpaper.cpp
<http://reviewboard.kde.org/r/3092/#comment3831>

    spacing of {}s


- Aaron


On 2010-03-03 15:47:34, Davide Bettio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3092/
> -----------------------------------------------------------
> 
> (Updated 2010-03-03 15:47:34)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch adds support to multiple render threads in Plasma::Wallpaper and fixes a small bug that has been happening when there wasn't any wallpaper in the wallpaper cache.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/private/wallpaper_p.h 1097236 
>   /trunk/KDE/kdelibs/plasma/wallpaper.cpp 1097236 
> 
> Diff: http://reviewboard.kde.org/r/3092/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Davide
> 
>