----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111992/#review37675 -----------------------------------------------------------
It's looking good already, a few stylistic remarks inline, and some future safety. plasma/generic/applets/activitybar/CMakeLists.txt <http://git.reviewboard.kde.org/r/111992/#comment27834> Is this really needed? If you build from top-level, this is already done earlier. Remove both, KDE4 REQUIRED and KDE4Defaults calls plasma/generic/applets/activitybar/CMakeLists.txt <http://git.reviewboard.kde.org/r/111992/#comment27835> Use the org.kde.activitybar plugin here, if comment below applies. plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27837> http://techbase.kde.org/Policies/Licensing_Policy#GPL_Header If you're OK with that, please use the second version of the GPL license, it grants fallback rights to KDE e.V. which could become important in case the GPL is deemed invalid. plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27836> (C) is not needed here, please add an email address, though, so it's possible to contact you plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27838> ; plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27843> This could better be in your main item, as it makes it easier to separate logic (dataengine stuff) from presentation (tabbar). plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27832> Component.onCompleted should go as last thing in its parent Item, that's where we look by default. plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27840> ; here, and in other places. In general, please don't omit them. plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27833> for (var i = 0; ... Uninitialized vars will bite us in Plasma2, whitespace between for and ( plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27839> ; plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27842> Move to bottom of delegate plasma/generic/applets/activitybar/package/contents/ui/main.qml <http://git.reviewboard.kde.org/r/111992/#comment27841> ; in this function plasma/generic/applets/activitybar/package/metadata.desktop <http://git.reviewboard.kde.org/r/111992/#comment27831> If you didn't take the name from a previous applet, it should be something like "org.kde.activitybar". If the name was pre-existing, drop this issue. - Sebastian Kügler On Aug. 13, 2013, 4:11 a.m., Bhushan Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111992/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2013, 4:11 a.m.) > > > Review request for kde-workspace and Marco Martin. > > > Description > ------- > > Activity bar applet ported in QML. > > > Diffs > ----- > > plasma/generic/applets/activitybar/Messages.sh e73df21 > plasma/generic/applets/activitybar/CMakeLists.txt 51a2edb > plasma/generic/applets/activitybar/activitybar.h b95cb0c > plasma/generic/applets/activitybar/activitybar.cpp e66bf04 > plasma/generic/applets/activitybar/package/contents/ui/main.qml > PRE-CREATION > plasma/generic/applets/activitybar/package/metadata.desktop PRE-CREATION > plasma/generic/applets/activitybar/plasma-applet-activitybar.desktop > b7155de > > Diff: http://git.reviewboard.kde.org/r/111992/diff/ > > > Testing > ------- > > Works, Tested in plasmoidviewer and desktop > > > Thanks, > > Bhushan Shah > >
