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

[Christophe Olinger]

Thanks Aaron. This sounds great. I'll try to focus on that on my next  
hacking sessions. I might have a lot of questions though.

Shantanu, is it ok for you if i focus on this?

Also, i'll forget about a main home screen and focus on a browser  
showing all available dataengines grouped into the media modes.

Cheers.

You are all great guys and its a joy working with you.

Cheers


Chris


On 30 Mar 2010, at 18:59, "Aaron Seigo" <aseigo at kde.org> wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3396/#review4790
> -----------------------------------------------------------
>
>
> First the administrivia ;) Please watch the whitespace; for core  
> plasma code we use http://techbase.kde.org/Policies/Kdelibs_Coding_Style 
>  ... that means things such as 4 space indents rather than tabs and  
> "if (foo)" rather than "if(foo)". we are pretty strict about keeping  
> the code style consistent as it makes things so much tidier and  
> easier to work on.
>
> Now for the actually useful and interesting bits: looking at this  
> patch it occurs to me that this is a state machine. As such, I'd  
> really recommend revisiting this whole idea as a QStateMachine with  
> a set of QAbstractStates that define each mode. each of these modes  
> would define which UI components to show in a slot connected to  
> their own entered() signal, and queue for hiding these components on  
> their own exited() signal.
>
> in fact, what i'd recommend is creating a QAbstractState subclass  
> that provides unified access to things such as "the volume control".  
> this could be in the form of an enumeration (let's call it Component  
> for the purpose of this disucssion) which would then be used to  
> determine a layout. each mode would then be a subclass of this new  
> class, and switching between them would be a matter of changing the  
> state of the QStateMachine inside MediaController.
>
> what this class ultimately needs to do is allow a layout to be  
> defined, perhaps something like this:
>
> static const int VideoModeIcon = MediaCenterState::CustomComponent +  
> 1;
>
> BrowseVideoState::BrowseVideoState(QObject *parent)
>     : MediaCenterState(parent),
>       m_iconVideosMode(new Plasma::IconWidget(this))
> {
>    addToNavBar(VolumeComponent);
>    addToNavBar(VideoModeIcon);
>    ... etc ...
> }
>
> QGraphicsWidget *BrowseVideoState::customComponent(int component)
> {
>    if (component == VideoModeIcon) {
>        return m_videoModeWidget;
>    }
>
>    return 0;
> }
>
> in this example, addToNavBar would add components to an internal  
> list. MediaCenterState's job would then be to take the list of  
> current items in the layout, the list of new items in the layout and  
> remove the items that are no longer there while adding the ones that  
> are new.
>
> in this way only unique items will be added or removed, and this  
> will look rather slick when animated. it also means that adding new  
> states is just a matter of creating a new MediaCenterState subclass  
> and adding it to the StateMachine. this lends itself very nicely to  
> any future plugin system that may be desired. it also opens the door  
> for more than just "addToNavBar" (or whatever it ends up being  
> called) but definition of any sort of PMC UI changes that should be  
> enacted. a "showBrowser(bool)" could be added, for instance, to  
> control whether or not the file browser panel should pop out  
> automatically or not. a "homeApplet(const QString &pluginName)"  
> could be added to define what Plasma::Applet to load when entering  
> that state. it would also make adding a generic state for any full  
> screen Plasmoid loading completely trivial.
>
> i don't think a full QML based system is needed or even desired in  
> this case, since PMC should be ultimately in control of the actual  
> presentation and layout. the MediaCenterState classes would just  
> define an order of items and therefore also the transitions between  
> them (thanks to QStateMachine).
>
> thoughts?
>
>
> /trunk/playground/base/plasma/MediaCenterComponents/applets/ 
> mediacontroller/controller.cpp
> <http://reviewboard.kde.org/r/3396/#comment4284>
>
>    more state machine stuff :)
>
>
>
> /trunk/playground/base/plasma/MediaCenterComponents/applets/ 
> mediacontroller/controller.cpp
> <http://reviewboard.kde.org/r/3396/#comment4280>
>
>    don't need to check if it's empty; just call clear()
>
>
>
> /trunk/playground/base/plasma/MediaCenterComponents/containments/ 
> mediacontainment/mediacontainment.cpp
> <http://reviewboard.kde.org/r/3396/#comment4283>
>
>    this is for all intents and purposes a state machine. we should  
> probably treat it as such.
>
>
>
> /trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/ 
> mediacenter.h
> <http://reviewboard.kde.org/r/3396/#comment4282>
>
>    this will be troublesome if extensibility is ever desired; this  
> looks like a great use case for QStateMachine, in fact.
>
>
>
> /trunk/playground/base/plasma/MediaCenterComponents/shells/ 
> plasmediacenter/mainwindow.cpp
> <http://reviewboard.kde.org/r/3396/#comment4281>
>
>    honestly, this should just go full screen. i don't think there is  
> a real use case for a windowed mode.
>
>    so this line could probably just be:
>
>         showFullScreen();
>
>
> - Aaron
>
>
> On 2010-03-27 15:48:14, Christophe Olinger wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> http://reviewboard.kde.org/r/3396/
>> -----------------------------------------------------------
>>
>> (Updated 2010-03-27 15:48:14)
>>
>>
>> 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 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/applets/ 
>> mediabrowser/mediabrowser.h 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/applets/ 
>> mediabrowser/mediabrowser.cpp 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/applets/ 
>> mediacontroller/controller.h 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/applets/ 
>> mediacontroller/controller.cpp 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/containments/ 
>> mediacontainment/mediacontainment.h 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/containments/ 
>> mediacontainment/mediacontainment.cpp 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/containments/ 
>> mediacontainment/medialayout.h 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/containments/ 
>> mediacontainment/medialayout.cpp 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/libs/ 
>> mediacenter/browser.cpp 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/libs/ 
>> mediacenter/mediacenter.h 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/libs/ 
>> mediacenter/playbackcontrol.h 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/libs/ 
>> mediacenter/playbackcontrol.cpp 1108007
>>  /trunk/playground/base/plasma/MediaCenterComponents/shells/ 
>> plasmediacenter/mainwindow.cpp 1108007
>>
>> 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
>>
>>
>