[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: Initial work on the flexible controller of the
From: "Alessandro Diaferia" <alediaferia () gmail ! com>
Date: 2010-03-30 21:48:59
Message-ID: 20100330214859.22081.53746 () localhost
[Download RAW message or body]
> On 2010-03-30 16:59:21, Aaron Seigo wrote:
> > 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?
Wow! This really seems the right-way-of-doing-things of what I suggested above to \
Christophe about handling modes. Too bad i'm not experienced in QState* but i'd \
really like seeing it implemented this way.
This way the layouting stuff becomes really clean and easily extensible. Please \
Christophe, go for it. :-)
Thanks Aaron for this precious hint!
- Alessandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3396/#review4790
-----------------------------------------------------------
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@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic