From kde-panel-devel Thu May 27 09:32:47 2010 From: Alessandro Diaferia Date: Thu, 27 May 2010 09:32:47 +0000 To: kde-panel-devel Subject: Re: Review Request: Plasma Mediacenter: use new plasma animations, Message-Id: X-MARC-Message: https://marc.info/?l=kde-panel-devel&m=127495282025590 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============1272164163==" --===============1272164163== Content-Type: multipart/alternative; boundary=001485f27432d5e8cb0487901283 --001485f27432d5e8cb0487901283 Content-Type: text/plain; charset=UTF-8 2010/5/27 Christophe Olinger > 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 > 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 > > > > > > 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 > > > > > > please prefer member accessors to array-type ones: > m_pictureMedias.first(); > > > > > > > > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp > > > > > > 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 > > > > > > 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 --001485f27432d5e8cb0487901283 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

2010/5/27 Christophe Olinger <olingerc@binaryloo= ks.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.
=C2=A0
Yeah, great idea! So we give them more vis= ibility among eventual other contributors.
I'd likely try to = figure out which of them can represent junior jobs for new contributors :-)= =C2=A0

Cheers,

Ciao :-)=C2=A0

Christophe




On Wed, May 26, 2010 at 11:15 PM, Alessandro Diaferia
<alediaferia@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 th= ing and i wanted this to be done as soon as possible. I still didn't ha= ve 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 do= n'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>
>
> =C2=A0 =C2=A0please add a little comment and specify we're returni= ng 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>
>
> =C2=A0 =C2=A0please prefer member accessors to array-type ones: m_pict= ureMedias.first();
>
>
>
> trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/me= dialayout.cpp
> <http://reviewboard.kde.org/r/4152/#comment5504>
>
> =C2=A0 =C2=A0you can put this as a static const qreal MARGINFACTOR =3D= 0.2; at the beginning of the code
>
>
>
> trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/me= dialayout.cpp
> <http://reviewboard.kde.org/r/4152/#comment5505>
>
> =C2=A0 =C2=A0this line doesn't do anything? you probably want to d= o m_controlAutohide =3D !set;
>
>
> - Alessandro
>
>
> On 2010-05-26 08:19:03, Christophe Olinger wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> h= ttp://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 t= he currentPlayback state
>> - Renamed layoutZone enum members
>>
>> Bugfixes:
>> - Fixed bug where video player would be fully expanded horizontall= y, 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 vide= os
>> - Layout errors of bottom bar (will fix that in the next days)
>>
>>
>> Diffs
>> -----
>>
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/applets/= mediacontroller/controller.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/applets/= mediainfobar/mediainfobar.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/applets/= mediaplayer/mediaplayer.h 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/applets/= mediaplayer/mediaplayer.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/homestate.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/mediacenter.h 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/mediacenterstate.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/medialayout.h 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/medialayout.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/musicstate.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/picturestate.h 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/picturestate.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/player.h 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/private/mediahandler.cpp 1130736
>> =C2=A0 trunk/playground/base/plasma/MediaCenterComponents/libs/med= iacenter/videostate.cpp 1130736
>>
>> Diff: http://reviewboard.kde.org/r/4152/diff
>>
>>
>> Testing
>> -------
>>
>>
>> Thanks,
>>
>> Christophe
>>
>>
>
>



--
Alessandro = Diaferia
KDE Developer
KDE e.V. member

--001485f27432d5e8cb0487901283-- --===============1272164163== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel --===============1272164163==--