Review Request: Plasma Mediacenter: Move tabbar and browsinig widgets into Browser

Alessandro Diaferia alediaferia at gmail.com
Thu May 20 23:59:50 CEST 2010 

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


Just little annotations here and there. I like the way we're doing this :-) Don't worry about the bugs you mention, i'll likely investigate them as soon as the patch goes in.


trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/browsingwidget.h
<http://reviewboard.kde.org/r/4050/#comment5415>

    ok here we have an issue..
    We want this enum to behave as QFlags so imagine it like a sequence of binary simbols. Flags mean we allow more than one flag to be active at the same time. For this to happen correctly each flag has to have a binary signature that can be OR'ed with the others (that is mostly like ADD operation).
    So, this is how it should be (showing binary signatures):
    
    NoControls        0000
    BackwardControl   0001
    ViewModeControl   0010
    ForwardControl    0100
    UpLevelControl    1000
    AllControls       1111
    
    Think of them as LEDs for example. When NoControls is chosen we want all the LEDs to be turned off, so 0000. Then each one of them can be turned on without compromising the state of other LEDs. Finally AllControls wants all of them to be turned on, so 1111. The fact that we don't simply increment the value when adding a flag is that doing so might cause ambiguities. So, for example, if we put ViewModeControl = 0010 and ForwardControl = 0011 we have troubles. This is because 0011 would mean, in this case, that, both ViewModeControl (0010) and BackwardControl (0001) are turned on, but, also, that ForwardControl is turned on.
    
    Now, when doing flag1 | flag2 you mean you want both flags to be active.
    Hopes i was clear.
    So, here we should have ForwardControl = 0x4, UpLevelControl = 0x8 and AllControls = 0x15



trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h
<http://reviewboard.kde.org/r/4050/#comment5407>

    i'd go for the following methods:
    void setShowingBrowsingWidgets(bool);
    bool isShowingBrowsingWidgets() const;



trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h
<http://reviewboard.kde.org/r/4050/#comment5404>

    the member variable should be placed under private.



trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h
<http://reviewboard.kde.org/r/4050/#comment5409>

    Shouldn't we need also an accessing functions to the list of added view modes?



trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h
<http://reviewboard.kde.org/r/4050/#comment5408>

    This might end up good as a slot probably.



trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.cpp
<http://reviewboard.kde.org/r/4050/#comment5417>

    The stuff i mentioned above about flags should fix this.



trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/browser.h
<http://reviewboard.kde.org/r/4050/#comment5410>

    Here goes the consideration i mentioned earlier about setShowingBrowsingWidgets and isShowing... also this might probably be a slot.


- Alessandro


On 2010-05-19 10:50:29, Christophe Olinger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4050/
> -----------------------------------------------------------
> 
> (Updated 2010-05-19 10:50:29)
> 
> 
> Review request for Plasma and Alessandro Diaferia.
> 
> 
> Summary
> -------
> 
> Review request for plasma-mediacenter
> Now that the uberpatch is in, hacking has become easier again :-)
> I decided to put the browsing control button (goPrevious) back into the browser, along with the tabbar. The tabbar can be used to change viewmodes later, e.g. by Artist, by Album, by Tag,...I have added an API that can be used to add pages to the tabbar. The states handle the adding of pages on entry. (Later we can add the connection between the tabbar and the modelpackage/dataengines of the browser).
> 
> Question: do we need states for playing and browsing? (also for the floating mode?) At the moment there is one state for each, but with differences between playing and browsing
> 
> Question: I am thinking of playing around with QML as soon as kubuntu has a qt4.7 package. Do you think I should? This would mean a lot of refactoring. I wanted to just use it for the two panels at the beginning. Maybe also the playlist.
> 
> Current bugs (just so that I do not forget them and maybe one of you wants to look at them ;-)
> Video Mode: When entering the videomode and clicking on play (with a video in the playlist) the videoplayer covers only half of the screen. When the video is stopped and again play is pressed, the videoplayer size is correct. Also, when I ply via the playlist (clicking on the item in the playlist) the videoplayer size is always correct?
> Picture mode: I do not understand how to correctly position the widgets in the bottom bar. They are at the wrong position on state entry but as soon as I change a picture, they jump to the correct positions. Switching to the floating mode makes the widgets be a wrong positions also :-/
> 
> We need a way to tell the browser to change the modelpackage. I guess via the plugin factory, but that is over my head ATM.
> 
> 
> Diffs
> -----
> 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/browsingwidget.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/browsingwidget.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localpictures/localpictures.desktop 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localvideos/localvideos.desktop 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/mediainfobar.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/mediainfobar.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/browser.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/homestate.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.cpp 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.h 1128382 
>   trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.cpp 1128382 
> 
> Diff: http://reviewboard.kde.org/r/4050/diff
> 
> 
> Testing
> -------
> 
> All states were thoroughly tested and the above bugs were found (among others) There are lots of TODOs and FIXMEs in the code still. It's a WIP :-)
> 
> 
> Thanks,
> 
> Christophe
> 
>

More information about the Plasma-devel mailing list