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

[Shantanu Tushar Jha]

On Wed, Mar 31, 2010 at 12:30 AM, Christophe Olinger
<olingerc at binarylooks.com> wrote:
>
> 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?

Yes, it will be very nice :)

>
> 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
>>>
>>>
>>
> _______________________________________________
> Plasma-devel mailing list
> Plasma-devel at kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>



-- 
Shantanu Tushar    (UTC +0530)
http://www.shantanutushar.com