Review Request: Return 80% functionality to PMC, further evolving of state architecture

[Aaron Seigo]

> On 2010-04-19 07:59:49, Alessandro Diaferia wrote:
> > I just had a quick look at the patch, i'm rather busy atm :/
> > 
> > Anyway, i still have concerns about the subcomponents placement stuff.
> > Rather than having WidgetAndZone approach imho we should enhance MediaController API in order to have methods like the followings:
> > 
> >     void insertSubComponent(MediaCenter::SubComponentType, QGraphicsWidget *);
> > 
> >     /**
> >      * This method is used to have access to a MediaController SubComponent.
> >      * @see MediaCenter::SubComponentType to know which classes are associated
> >      * with each SubComponentType.
> >      */
> >     QGraphicsWidget* subComponent(MediaCenter::SubComponentType) const;
> > 
> >     void setSubComponentLocation(MediaCenter::SubComponentType, Plasma::Location);
> >     Plasma::Location subComponentLocation() const;
> > 
> > This way we can insert subcomponents into it specifying their preferred position. Then, we might associate a Plasma widget to each SubComponentType in order to allow type casting when returning the component through the subComponent() method.
> > I don't know if we should somehow force the specific class to the specific SubComponentType.
> > Anyway i think subcomponents should be all specified inside the mediacenter.h header file or at least mediacenterstate.h but not specifically for each state.
> > 
> > I'll keep reviewing the rest of the patch.
> 
> Christophe Olinger wrote:
>     I agree that the current approach using the 4 zones is quite limited and might not give us enough control over how PMC looks later on. To implement Alessandro's idea I would have to do two things:
>     - First:
>     At the moment the state configures the subComponents and adds them to a list. The containment asks for that list on each state change and then tells the controller to take that list and layout the subComponents. The list contains QPairs made of a widget and a zone.
>     Alessandro's approach would mean to not have the containment ask for a list and give that to the controller but instead the state itself would send the widget along with a type to the controller immediately after configuring it. This approach would be easy to implement, but I am not sure if there are advantages to sending something multiple times or collecting it in a list and sending it once. We could also just replace the "zone" information in the QPair with the actual widget type.
>     - Second:
>     The applet currently has several layouts and adds widgets to these layouts according to the zone information. If instead of a zone I send a widget type, I would not have 4 if/thens (for 4 zones) but as many if/thens as there are widget types defined. This would give us a much more fine grained control over the layout, but OTOH would mean lots of if/thens.
>     One advantage of having the widget type available would be to actually replace a widget with another widget of the same type if later a developer decicdes to for example write a new volume slider. Is the latter actually true? Is it even needed?
>     
>     The other comment is about having the widget types defined in one spot. Instead of having an enum in each state, have one big enum somewhere else. Currently this is very easy, because there is no overlapping of names, and the state objects have access to mediacenter.h or/and mediacenterstate.h anyway. The widgets themselves would still be created in the state objects themselves.
>     
>     What do you think?

there are two different issues here: passing around lists of QPairs and using MediaCenter::AppletZone.

passing QPairs around isn't very pretty and it isn't necessary. one could use a QHash<QGraphicsWidget, MediaCenter::AppletZone> instead, but even then i'd probably just go with the simplified style of API that Allesandro suggests: addSubComponent(QGraphicsWidget *widget, MediaCenter::Appletzone zone); on state change, a series of addSubComponent calls would occur from the new state and the old state's components would be removed. there really is no need for lists in the API.

however, using MediaCenter::AppletZone (though personally i'll call it MediaCenter::LayoutZone) is definitely "the right way" to go.


- Aaron


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


On 2010-04-19 19:43:39, Christophe Olinger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3552/
> -----------------------------------------------------------
> 
> (Updated 2010-04-19 19:43:39)
> 
> 
> Review request for Plasma and Alessandro Diaferia.
> 
> 
> Summary
> -------
> 
> The state calsses now have less functions. Only one for connections and one for configuration. The first one is only called once at PMC initialization, the second one is called at each state switch. Some connections can conflict between states. Those are connected at entry() and disconnected at exit(). Thanks to Alessandr's work we can now also configure the layout from within the state class (I only had to correct some namespace stuff in the medialayout class, I hope that was correct).
> This patch also gets basic functionality back. The modes are not really useful yet. Video mode is the most complete and can be used to view everything at th moment.
> Next step: Clean this up a bit
> Think about subclassing plasma widgets to get the widget type into the widget. This is necessary to be able to tell the controller to layout into zones.
> Start work on information bar.
> 
> 
> Diffs
> -----
> 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/config.ui 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenter.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/player.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/player.cpp 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.h 1113370 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.cpp 1113370 
> 
> Diff: http://reviewboard.kde.org/r/3552/diff
> 
> 
> Testing
> -------
> 
> Lots and lots. Seems to be quite slow, but I think that is a problem in the player. It always iterates over all the queue even if it should only show a picture.
> 
> 
> Thanks,
> 
> Christophe
> 
>