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

Tue Apr 20 10:04:24 CEST 2010

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

I included some comments of the code so that we better see which parts need discussion and to explain a bit my approach of things.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp
<http://reviewboard.kde.org/r/3552/#comment4593>

    I probably do not need to remove the layouts, since I have deleted them before and created new ones.
    I needed to use the delete approach because the layouts kept on getting big white spaces when I switched layouts. As soon as all states had been activated at least once, the widgets stopped jumping. The above approach fixed that but it feels slower I think.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp
<http://reviewboard.kde.org/r/3552/#comment4594>

    I forgot to iterate over the other layouts. With the new delete layouts approach in the above function, I might not need this function anymore.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.h
<http://reviewboard.kde.org/r/3552/#comment4595>

    I think I do not need this anymore

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.h
<http://reviewboard.kde.org/r/3552/#comment4596>

    The player needs to send both the current MediaCenter::Media and the media object to the states. The first is needed so the states can tell the player which media file the user is currently looking at. The second is needed so that the states can attach the seek and volume slider of the current state to the player.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.h
<http://reviewboard.kde.org/r/3552/#comment4597>

    Not needed? Recheck this.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/3552/#comment4598>

    Check if I really need this together with a signal.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/3552/#comment4599>

    To be sure that these kind of checks work, we need to limit the states to a certain media type. This needs to be done on the browser level but also the playlist level.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/3552/#comment4600>

    On every state change I clear the m_medias list and repopulate it with the current state's playlist items. Since I do not have a playlist in the picture mode I populate the list with the activated item.
    Later I will fill the list with all pictures in the current directory to be able to press next, previous and have a slideshow.

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/3552/#comment4601>

    Slideshow not implemented yet. I first need to find a way to populate the m_medias list with the content of the current directory (see signal in browser applet)

trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp
<http://reviewboard.kde.org/r/3552/#comment4602>

    Check if this is needed at all.

trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp
<http://reviewboard.kde.org/r/3552/#comment4603>

    As soon as we have a home state I can put this there.

trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp
<http://reviewboard.kde.org/r/3552/#comment4605>

    Since I now delete the layouts on each state change, I also need to readd the Main Subcomponents.

trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp
<http://reviewboard.kde.org/r/3552/#comment4604>

    Remove this

trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp
<http://reviewboard.kde.org/r/3552/#comment4606>

    Do I still need this with the delete layouts approach?

trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp
<http://reviewboard.kde.org/r/3552/#comment4607>

    No need to clear probably.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.h
<http://reviewboard.kde.org/r/3552/#comment4608>

    I could also send the widget type instead of the zone. The control applet would then have an if/then for each widget type and thus would have more control of the actual layout. However, this would mean a lot of if/thens.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.h
<http://reviewboard.kde.org/r/3552/#comment4609>

    This is part of the background states UI

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp
<http://reviewboard.kde.org/r/3552/#comment4610>

    Is this correct? I have never used static variables before.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp
<http://reviewboard.kde.org/r/3552/#comment4611>

    Currently I do this on every state change. Maybe I can create a list as a member variable and fill it once at initialization of the state object and only hand out this list at state changes. This could speed up things a bit.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp
<http://reviewboard.kde.org/r/3552/#comment4612>

    I will have to send the jumpToState buttons (except home) to the browser in the home state mode, or will we have the control applet visible in the home state. Depends on what we actually want to show in the home state.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp
<http://reviewboard.kde.org/r/3552/#comment4613>

    Currently, the background state buttons take up space in the controller even when hidden. Where do we want to have these? Maybe in a layout with a fixed size to prevent jumping around of the other widgets when the background state widgets are removed or added?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp
<http://reviewboard.kde.org/r/3552/#comment4614>

    How do I tell the controller to adapt to its content? Or do we want to keep this size fixed?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.cpp
<http://reviewboard.kde.org/r/3552/#comment4615>

    Okay, I still need the PlaybackState function in the mediaplayer.
    We only consider a state as background when it is actually playing something.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.cpp
<http://reviewboard.kde.org/r/3552/#comment4616>

    This is executed when clicking on "stop"

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.cpp
<http://reviewboard.kde.org/r/3552/#comment4617>

    As soon as we have slideshow possibility, should we always pause that when switching away from this mode? This would mean that we can never have the picture state as background state. Do we need it?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h
<http://reviewboard.kde.org/r/3552/#comment4618>

    The way I did it nowm the control applet is reduced to only handle its own appearance, nothing else. All the logic is handled by the state objects. I though this would make it easier to use QML with PMC?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h
<http://reviewboard.kde.org/r/3552/#comment4619>

    This will go into the picture state later.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h
<http://reviewboard.kde.org/r/3552/#comment4620>

    Do I still need these?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h
<http://reviewboard.kde.org/r/3552/#comment4621>

    Still needed?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h
<http://reviewboard.kde.org/r/3552/#comment4622>

    Still needed now that the sliders are in the state objects?

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.h
<http://reviewboard.kde.org/r/3552/#comment4623>

    All states that actually handle the VideoWidget need MediaObject information. This will only ever be the video state and the music state so I did not add these functions to the MediaCenterState API.

trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.cpp
<http://reviewboard.kde.org/r/3552/#comment4624>

    When we click the background state button to return to the video state, I want to show the videoplayer if it is actually playing something.

- Christophe

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