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

[Aaron Seigo]

-----------------------------------------------------------
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
> 
>