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

[Alessandro Diaferia]

2010/5/27 Christophe Olinger <olingerc at binarylooks.com>

> Wow, thanks. My 4th accepted patch. Yay! I'll try to commit this
> evening or latest tomorrow.
>
> Concerning the bugs, I can collect them on our techbase page for now.
>

Yeah, great idea! So we give them more visibility among eventual other
contributors.
I'd likely try to figure out which of them can represent junior jobs for new
contributors :-)

>
> Cheers,
>

Ciao :-)

>
> Christophe
>
>
>
>
> On Wed, May 26, 2010 at 11:15 PM, Alessandro Diaferia
> <alediaferia at gmail.com> wrote:
> >
> > -----------------------------------------------------------
> > 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
> >>
> >>
> >
> >
>



-- 
Alessandro Diaferia
KDE Developer
KDE e.V. member
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100527/a72b7210/attachment-0001.htm