Review Request 120806: Port fifteenPuzzle applet to qml and plasma 5.

Mon Oct 27 16:45:11 UTC 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120806/#review69208
-----------------------------------------------------------


Just a bunch of pedantic code style comments from my side :)


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48369>

    You don't need ; after QML statements


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48368>

    Is this needed? What about just not setting a maximum{Width,Height} at all?


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48367>

    pieces = [];
    
    new Array() is more work for the engine because it has to figure out what "Array" is, rather than just knowing that's an array literal.


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48370>

    What about
    property Component piece: Piece {}
    and then piece.createObject() instead of creating this component all the time?


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48371>

    i never reaches 0


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48372>

    Be consistent with pre/suffix ++


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48373>

    Really needed?


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48374>

    Better create a Date object out of the seconds and then use getMinutes() et al


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48375>

    Date also has a toLocaleString()


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48376>

    Why not just anchor it everywhere?


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48377>

    Use Plasma Components in applets (except in settings dialog). These also follow text color and other theme settings


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48378>

    anchors.centerIn: parent


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48379>

    Both default to false already


applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml
<https://git.reviewboard.kde.org/r/120806/#comment48380>

    onTriggered: ++main.seconds


applets/fifteenPuzzle/package/contents/ui/Piece.qml
<https://git.reviewboard.kde.org/r/120806/#comment48381>

    Use PlasmaComponents.Label (theme font size, family, etc)


applets/fifteenPuzzle/package/contents/ui/Piece.qml
<https://git.reviewboard.kde.org/r/120806/#comment48383>

    visible: plasmoid.configuration.showNumerals
    
    You're not using that property anywhere else


applets/fifteenPuzzle/package/contents/ui/Piece.qml
<https://git.reviewboard.kde.org/r/120806/#comment48382>

    onClicked?


applets/fifteenPuzzle/package/contents/ui/main.qml
<https://git.reviewboard.kde.org/r/120806/#comment48384>

    You're not using these. I've seen stray imports in other places, too.


- Kai Uwe Broulik


On Okt. 27, 2014, 4:05 nachm., Jeremy Whiting wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120806/
> -----------------------------------------------------------
> 
> (Updated Okt. 27, 2014, 4:05 nachm.)
> 
> 
> Review request for Plasma and David Edmundson.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> Config options from previous c++ version kept though names are a bit diferent.
> Images not yet supported.
> Puzzle starts at 0 in the top left corner, maybe should start with 1 in the corner though.
> 
> This is my first port to plasma qml, so I may have used some of the non suggested/recommended components, if so let me know and I'll fix it.
> 
> 
> Diffs
> -----
> 
>   applets/fifteenPuzzle/src/fifteenPuzzleConfig.ui ff82f331db4cee2d66f526954be63f0f5d81d250 
>   applets/fifteenPuzzle/src/fifteenPuzzle.cpp 8a1528988f3e693d20179db4a209309b0aad87fd 
>   applets/CMakeLists.txt 63e6e25628d18ee474231acd2a21711841dee592 
>   applets/fifteenPuzzle/CMakeLists.txt 04d5e55fd246684855d49484f1233dac054a0124 
>   applets/fifteenPuzzle/Messages.sh PRE-CREATION 
>   applets/fifteenPuzzle/icons/CMakeLists.txt 106884f432c1d1e0b0584959af854c79ede4ea6d 
>   applets/fifteenPuzzle/icons/hisc-app-fifteenpuzzle.svgz  
>   applets/fifteenPuzzle/images/blanksquare.svg  
>   applets/fifteenPuzzle/package/contents/config/config.qml PRE-CREATION 
>   applets/fifteenPuzzle/package/contents/config/main.xml PRE-CREATION 
>   applets/systemloadviewer/package/contents/ui/ColorPicker.qml 92062db546dcff67f930d4888180f4e753798c27 
>   applets/fifteenPuzzle/src/piece.h d0e58d0f9d38d4a1ef2110b974b3f4f6938293e1 
>   applets/fifteenPuzzle/src/piece.cpp 2efb72ecf69d9beaa53367bc2f3c9cee88238f28 
>   applets/fifteenPuzzle/src/fifteenPuzzle.h cf7885380f0e152d51cf2dc7557444e9b425b596 
>   applets/fifteenPuzzle/package/contents/ui/configAppearance.qml PRE-CREATION 
>   applets/fifteenPuzzle/package/contents/ui/main.qml PRE-CREATION 
>   applets/fifteenPuzzle/plasma-applet-fifteenPuzzle.desktop 513cc0084df7247a520807620361b0426623727e 
>   applets/fifteenPuzzle/src/Messages.sh bab24ae73049f37d9693cf062eaaa98ca1e6bab0 
>   applets/fifteenPuzzle/src/fifteen.h 2a27f5b109988003de45fb64c457484ebdfdbc8b 
>   applets/fifteenPuzzle/src/fifteen.cpp ebdcf2c0756a17ea174c0fc5fd106e157b223063 
>   applets/fifteenPuzzle/package/contents/ui/ColorPicker.qml PRE-CREATION 
>   applets/fifteenPuzzle/package/contents/ui/FifteenPuzzle.qml PRE-CREATION 
>   applets/fifteenPuzzle/package/contents/ui/Piece.qml PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/120806/diff/
> 
> 
> Testing
> -------
> 
> I've tested it with plasmoidviewer -a org.kde.plasma.fifteenpuzzle. 
> 
> The icon is not working in the widget adder, not sure where to install that to for it to work.
> Images not supported yet, though they are in the config, maybe should remove from config until they are supported?
> 
> 
> Thanks,
> 
> Jeremy Whiting
> 
>

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