Review Request 111992: Activity bar in QML.

[Sebastian Kügler]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130813/5ae11c13/attachment.htm>