Review Request 112888: Roughly port icon applet to plasma2

Mon Sep 23 09:47:29 UTC 2013

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112888/#review40507
-----------------------------------------------------------

plasma/generic/applets/icon/package/contents/ui/main.qml
<http://git.reviewboard.kde.org/r/112888/#comment29818>

    For license, we prefer the GPL with KDE e.V. exception. I'll leave this up to you whether you think this is OK for this code or not. (As long as it's GPL or LGPL (or more liberal), we'll accept it.)

    The preferred one would be the second option here:
    http://techbase.kde.org/Policies/Licensing_Policy#GPL_Header

plasma/generic/applets/icon/package/contents/ui/main.qml
<http://git.reviewboard.kde.org/r/112888/#comment29819>

    Use 16 here, I'd say. 1x1 icons don't make sense.

plasma/generic/applets/icon/package/contents/ui/main.qml
<http://git.reviewboard.kde.org/r/112888/#comment29821>

    This will not work well for iconswithlongnamebutwithoutspacesinthename.extension. :)

plasma/generic/applets/icon/package/metadata.desktop
<http://git.reviewboard.kde.org/r/112888/#comment29817>

    This should probably be 32,32 or 48,48. 100 is not among the standard icon sizes.

plasma/generic/applets/icon/plugin/icon_p.h
<http://git.reviewboard.kde.org/r/112888/#comment29822>

    Why Q_INVOKABLE, and not Q_PROPERTIES here? (For all three accessors). That would seem more declarative.

    This would also make the "remote" case easier, when you have to stat a remote file asynchronously and only know details after the network request.

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29829>

    m_ is for members

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29824>

    url.fileName(); is not enough here?

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29828>

    m_ is for members, use icon or iconName here.

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29823>

    trailing whitespace

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29825>

    } else ...

    (no new line)

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29827>

    m_ prefix is used for member variables. Rename it to genericName for example.

plasma/generic/applets/icon/plugin/icon_p.cpp
<http://git.reviewboard.kde.org/r/112888/#comment29826>

    no new line

Definitely going in the right direction. Let's get a few issues I've pointed out inside fixed, then merge it into frameworks-scratch and finish it up there.

- Sebastian Kügler

On Sept. 23, 2013, 8:26 a.m., Bhushan Shah wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112888/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2013, 8:26 a.m.)
> 
> 
> Review request for Plasma, Marco Martin and Sebastian Kügler.
> 
> 
> Description
> -------
> 
> Port Icon applet to plasma2
> 
> Some issues..
> 
> -> Can be resized to 1x1.
> -> Still doesn't work for folder, and external URLs.
> -> Configuration dialog
> 
> 
> Diffs
> -----
> 
>   plasma/generic/applets/CMakeLists.txt 4ace449 
>   plasma/generic/applets/icon/CMakeLists.txt 1094666 
>   plasma/generic/applets/icon/icon.h 45fa07c 
>   plasma/generic/applets/icon/icon.cpp 8288850 
>   plasma/generic/applets/icon/package/contents/config/main.xml PRE-CREATION 
>   plasma/generic/applets/icon/package/contents/ui/main.qml PRE-CREATION 
>   plasma/generic/applets/icon/package/metadata.desktop PRE-CREATION 
>   plasma/generic/applets/icon/plasma-applet-icon.desktop 8275ddd 
>   plasma/generic/applets/icon/plugin/icon_p.h PRE-CREATION 
>   plasma/generic/applets/icon/plugin/icon_p.cpp PRE-CREATION 
>   plasma/generic/applets/icon/plugin/iconplugin.h PRE-CREATION 
>   plasma/generic/applets/icon/plugin/iconplugin.cpp PRE-CREATION 
>   plasma/generic/applets/icon/plugin/qmldir PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112888/diff/
> 
> 
> Testing
> -------
> 
> 1) Compiles
> 2) Installs
> 3) Loads
> 
> -> Dropping application icon from KickOff - creates icon
> -> Dropping files from Dolphin, both executable and normal files - creates icon
> -> Dropping folder from Dolphin - creates icon applet but neither icon nor text because folder handling is not added yet.
> 
> 4) Opens URL - Clicking on the icon applet opens url with kfmclient
> 
> 
> File Attachments
> ----------------
> 
> icons in panel
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/23/snapshot1.png
> icons as launcher
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/23/snapshot2.png
> files
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/23/snapshot3.png
> folders
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/09/23/snapshot4.png
> 
> 
> Thanks,
> 
> Bhushan Shah
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130923/bdeafa44/attachment-0001.html>