----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3552/#review5677 -----------------------------------------------------------
Ship it! I basically like how it works. There are minor issues from the code point of view and from the usability one but please commit it as soon as possible so that other developers can start contributing without the risk of conflicting with your changes. From now on please go for little patches, one for each field they affect on the codebase so that changes can become easier to track. trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.h <http://reviewboard.kde.org/r/3552/#comment5342> also here you can go for const reference parameter trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.h <http://reviewboard.kde.org/r/3552/#comment5343> ..and here trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.cpp <http://reviewboard.kde.org/r/3552/#comment5344> i think this is due to KDirLister so we should investigate probably through the folderview code. trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.cpp <http://reviewboard.kde.org/r/3552/#comment5345> foreach (const KFileItem &item, items) { otherwise it is a really bad thing ;) trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowserlibs/modelpackage.h <http://reviewboard.kde.org/r/3552/#comment5374> why is it under protected? shouldn't it be under private? trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localmusic/localmusicpackage.h <http://reviewboard.kde.org/r/3552/#comment5346> constify it - Alessandro On 2010-05-09 19:09:50, Christophe Olinger wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/3552/ > ----------------------------------------------------------- > > (Updated 2010-05-09 19:09:50) > > > 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/CMakeLists.txt > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/abstractmediaitemview.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowser.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowserlibs/modelpackage.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/mediabrowserlibs/modelpackage.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/CMakeLists.txt > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localfiles/CMakeLists.txt > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localfiles/localconfig.ui > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localfiles/localfiles.desktop > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localfiles/localfilespackage.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localfiles/localfilespackage.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localmusic/CMakeLists.txt > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localmusic/localmusic.desktop > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localmusic/localmusicconfig.ui > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localmusic/localmusicpackage.h > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localmusic/localmusicpackage.cpp > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localpictures/CMakeLists.txt > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localpictures/localpictures.desktop > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localpictures/localpicturesconfig.ui > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localpictures/localpicturespackage.h > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localpictures/localpicturespackage.cpp > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localvideos/CMakeLists.txt > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localvideos/localvideos.desktop > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localvideos/localvideosconfig.ui > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localvideos/localvideospackage.h > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/modelpackages/localvideos/localvideospackage.cpp > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/viewitem.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediabrowser/viewitem.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediacontroller/controller.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/CMakeLists.txt > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/mediainfobar.h > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/mediainfobar.cpp > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediainfobar/plasma-applet-mediainfobar.desktop > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/config.ui > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/mediaplayer.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/pictureviewer.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/mediaplayer/pictureviewer.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/playlist/playlistapplet.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/playlist/playlistapplet.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/playlist/playlistwidget.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/applets/playlist/playlistwidget.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/containments/mediacontainment/mediacontainment.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/CMakeLists.txt > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/browser.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/browser.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/homestate.h > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/homestate.cpp > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/infodisplay.h > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/infodisplay.cpp > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenter.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/mediacenterstate.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/medialayout.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/musicstate.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/picturestate.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playbackcontrol.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/player.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/player.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/playlist.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/libs/mediacenter/videostate.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/shells/plasmediacenter/CMakeLists.txt > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/shells/plasmediacenter/backgrounddialog.ui > PRE-CREATION > > trunk/playground/base/plasma/MediaCenterComponents/shells/plasmediacenter/main.cpp > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/shells/plasmediacenter/mainwindow.h > 1117494 > > trunk/playground/base/plasma/MediaCenterComponents/shells/plasmediacenter/mainwindow.cpp > 1117494 > > 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
