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