Review Request: Initial work on the flexible controller of the Plasma Media Center

[Christophe Olinger]

> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > I like how the MC looks like this way. Please be sure to fix what i pointed out. Of course i'd also wait for at least Marco's suggestions about this work.

Cool. Let's see If QML makes it even easier to prettify it.


> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > /trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.cpp, line 104
> > <http://reviewboard.kde.org/r/3396/diff/1/?file=21632#file21632line104>
> >
> >     Why is this needed?

This is a leftover of an experiment. I'll remove it. I tried to have the panel adapt to the current MediaType selected. Now that we decided to go with specific modes, this is no longer needed (although i am not sure yet how to handle small videos taken by picture cameras that end up in the picture folder).


> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > /trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h, line 54
> > <http://reviewboard.kde.org/r/3396/diff/1/?file=21631#file21631line54>
> >
> >     Please remove whitespaces all aroud the code.

Will do.


> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > /trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.cpp, line 54
> > <http://reviewboard.kde.org/r/3396/diff/1/?file=21632#file21632line54>
> >
> >     Having the browsing widgets only in your panel is just fine imho. You can write a method to enable/disable them in the browser API probably.

I'll write an API and tell the mediacontainment to inactivate them in case a controller is present.


> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > /trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp, line 105
> > <http://reviewboard.kde.org/r/3396/diff/1/?file=21634#file21634line105>
> >
> >     As you said in the comment: please remove magic numbers. Put them as static const int s_name at the beginning of this file.

Will do that.


> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > /trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp, line 178
> > <http://reviewboard.kde.org/r/3396/diff/1/?file=21634#file21634line178>
> >
> >     What do you really want to achieve with this loop? clear() method can be used to clear a list.

Lack of experience and knowledge. Of course if there is an easier way how to empty a list (.clear()) I'll use that.


> On 2010-03-26 09:30:17, Alessandro Diaferia wrote:
> > /trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/medialayout.cpp, line 22
> > <http://reviewboard.kde.org/r/3396/diff/1/?file=21638#file21638line22>
> >
> >     This is not the way to go. This layout class should only interact with the API and not with actual and specific implementations such as the MediaController. However i do not see in this code any specific MediaController object created so you can remove this header. Or just revert the specific code if i'm blind :)

Hmm, this must be another leftover of an experiment. I'll have a closer look once at home again and eventually remove the includes.


- Christophe


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


On 2010-03-25 19:01:32, Christophe Olinger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3396/
> -----------------------------------------------------------
> 
> (Updated 2010-03-25 19:01:32)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch extends the controller applet by having 6 different layout modes which are adapted to what the media center is currently used for, i.e. browsing pictures, playing videos, etc. It sends a signal to the containment with the current mode. The containment then relayouts the other applets and configures them for the current Mode. These modes are defined as enum in the libs.
> *The browser no longer has any controls. Those are now in the controller.
> *The controller also has a show/hide playlist button and a toggle autohide button for itself.
> *The different modes do not have sensible functions yet. I also need to work on configuring the applets for each mode, like telling the browser to hide, or the player to show.
> *The controller is not really beautiful. I want animations for show(hide icons. I want the modeswitch button in a "drawer" perhaps. The toggle buttons need effects.
> 
> 
> Diffs
> -----
> 
>   /trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.cpp 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.cpp 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.h 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.h 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/medialayout.h 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/medialayout.cpp 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/browser.cpp 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenter.h 1107457 
>   /trunk/playground/base/plasma/MediaCenterComponents/shells/plasmediacenter/mainwindow.cpp 1107457 
> 
> Diff: http://reviewboard.kde.org/r/3396/diff
> 
> 
> Testing
> -------
> 
> I tested the controller itself. The actual effect on the other applets when changing modes still needs work.
> 
> 
> Thanks,
> 
> Christophe
> 
>