----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/plasma-devel
