Review Request: Plasma Mediacenter: use new plasma animations, fix some bugs

[Alessandro Diaferia]

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

Ship it!


This is a good patch: the move to new animator API is a really good thing and i wanted this to be done as soon as possible. I still didn't have a look at the video widget size bug as i still don't have videos on my new installation. Anyway i'd say go for this patch and i'll try to fix the bug in the next patch i'm working on.

Also i'd like you to take note of such bugs you encounter as we don't have a component on bko still (i probably should ask for one).

Here follows little issues i noticed.


trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/4152/#comment5502>

    please add a little comment and specify we're returning to browsing mode as soon as slide-show is finished



trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/4152/#comment5503>

    please prefer member accessors to array-type ones: m_pictureMedias.first();



trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp
<http://reviewboard.kde.org/r/4152/#comment5504>

    you can put this as a static const qreal MARGINFACTOR = 0.2; at the beginning of the code



trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp
<http://reviewboard.kde.org/r/4152/#comment5505>

    this line doesn't do anything? you probably want to do m_controlAutohide = !set;


- Alessandro


On 2010-05-26 08:19:03, Christophe Olinger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4152/
> -----------------------------------------------------------
> 
> (Updated 2010-05-26 08:19:03)
> 
> 
> Review request for Plasma and Alessandro Diaferia.
> 
> 
> Summary
> -------
> 
> This patch covers the following:
> 
> Features:
> - Use new plasma animation classes
> - In floating mode, fade player between pictures (avoids painting artefacts)
> - Return to browser after showing last picture of a slideshow
> - Start at first picture again after slideshow has been played and is started again
> - Background states are handled onExit of each state by checking the currentPlayback state
> - Renamed layoutZone enum members
> 
> Bugfixes:
> - Fixed bug where video player would be fully expanded horizontally, but not vertically
> - Slideshow now works correctly
> - Bottom Panel should be correctly placed now at startup
> 
> Evil bugs:
> - Video player always remains at 1024 x 600 :-/. Alessandro, could you have a look please?
> - Qt 4.7 beta 1 does not solve the blue skin bug when playing videos
> - Layout errors of bottom bar (will fix that in the next days)
> 
> 
> Diffs
> -----
> 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/mediainfobar.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.h 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/homestate.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenter.h 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.h 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.h 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/player.h 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/private/mediahandler.cpp 1130736 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.cpp 1130736 
> 
> Diff: http://reviewboard.kde.org/r/4152/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christophe
> 
>