----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112447/#review39164 -----------------------------------------------------------
the Plasma/Shell package definition should live in libplasma along with the others. this will remove the need for the PluginLoader subclass and ensure that Corona can be used by any application that simply links to libplasma, which is obviously a requirement. the other classes should perhaps find a more appropriate home as well rather than creating a whole new library. really, this is not the "Plasma View Library" but the "if you are writing a QML shell with libplasma you will find these couple of classes useful..." library. most optimal would be if this could be tucked away with the rest of the QML support for plasma, perhaps in the script engine library itself. the config classes also scream out to be made internal and not public API. as the main class used is a QQuickView, putting this in the QML support would be sensible and then all of this could be nicely hidden away. the classes also seem to be in need of some API review before we commit to binary compatibility for them ... something for one of the upcoming monday meetings? src/plasmaview/configview.h <http://git.reviewboard.kde.org/r/112447/#comment28893> this TODO seems to be done src/plasmaview/configview.h <http://git.reviewboard.kde.org/r/112447/#comment28894> is this TODO still valid given that this code is moving into a library? src/plasmaview/configview.cpp <http://git.reviewboard.kde.org/r/112447/#comment28895> seems less than useful debug output at this point ... src/plasmaview/containmentconfigview_p.h <http://git.reviewboard.kde.org/r/112447/#comment28898> another stray TODO? src/plasmaview/containmentconfigview_p.cpp <http://git.reviewboard.kde.org/r/112447/#comment28901> why is this using Plasma/Generic instead of a Plasma/QmlWallpaper (or whatever) package? sprinkling our code with setDefaultPackageRoot and filePath calls with literal values defeats the purpose of Plasma::Package: to encapsulate those details. if we were to move where the wallpaper packages were kept, then what? we search every single bit of code that might use wallpapers? no, that's why we have Plasma::Package subclasses! there should be a package type for these wallpapers. src/plasmaview/containmentconfigview_p.cpp <http://git.reviewboard.kde.org/r/112447/#comment28900> why calling m_containmnet->containment()? it already is a containment, so ... if !m_containment->isContainment() we have a problem so perhaps that could be added as an assert in the ctor, but calling m_containment->containment() should never be necessary when we are dealing with a Plasma::Containment* - Aaron J. Seigo On Sept. 2, 2013, 12:53 p.m., Giorgos Tsiapaliokas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112447/ > ----------------------------------------------------------- > > (Updated Sept. 2, 2013, 12:53 p.m.) > > > Review request for Plasma. > > > Description > ------- > > This patch creates a new library out of the shell dir. > We need this library in order to implement plasmoidviewer 2.0 > > > Diffs > ----- > > src/CMakeLists.txt 281d146 > src/plasmaview/CMakeLists.txt PRE-CREATION > src/plasmaview/PlasmaViewConfig.cmake.in PRE-CREATION > src/plasmaview/configview.h PRE-CREATION > src/plasmaview/configview.cpp PRE-CREATION > src/plasmaview/containmentconfigview_p.h PRE-CREATION > src/plasmaview/containmentconfigview_p.cpp PRE-CREATION > src/plasmaview/currentcontainmentactionsmodel_p.h PRE-CREATION > src/plasmaview/currentcontainmentactionsmodel_p.cpp PRE-CREATION > src/plasmaview/includes/PlasmaView/ConfigView PRE-CREATION > src/plasmaview/includes/PlasmaView/ContainmentConfigView PRE-CREATION > src/plasmaview/includes/PlasmaView/ShellPluginLoader PRE-CREATION > src/plasmaview/includes/PlasmaView/View PRE-CREATION > src/plasmaview/shellpackage_p.h PRE-CREATION > src/plasmaview/shellpackage_p.cpp PRE-CREATION > src/plasmaview/shellpluginloader.h PRE-CREATION > src/plasmaview/shellpluginloader.cpp PRE-CREATION > src/plasmaview/view.h PRE-CREATION > src/plasmaview/view.cpp PRE-CREATION > src/shell/CMakeLists.txt 3da019f > src/shell/configview.h 2e8f68f > src/shell/configview.cpp fea5a73 > src/shell/containmentconfigview.h 619fa14 > src/shell/containmentconfigview.cpp 235a33f > src/shell/currentcontainmentactionsmodel.h db94da1 > src/shell/currentcontainmentactionsmodel.cpp c955bef > src/shell/shellcorona.cpp ffdbfe8 > src/shell/shellpackage.h 99dc460 > src/shell/shellpackage.cpp 74aea5c > src/shell/shellpluginloader.h 1d3dade > src/shell/shellpluginloader.cpp 8b2e1dd > src/shell/view.h 7e6b2d9 > src/shell/view.cpp a0c6168 > > Diff: http://git.reviewboard.kde.org/r/112447/diff/ > > > Testing > ------- > > The code is locate at > g...@git.kde.org:clones/plasma-framework/tsiapaliwkas/pf5.git . > > how to test it > > * git clone g...@git.kde.org:clones/plasma-framework/tsiapaliwkas/pf5.git > * cd pf5 > * git checkout split7 > * build it and install it > * use plasma-shell > > I haven't noticed any issues. > > > Thanks, > > Giorgos Tsiapaliokas > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
