[Kde-games-devel] Review Request: Some wishes and fixes for kbounce
Dmitry Suzdalev
dimsuz at gmail.com
Fri Feb 26 22:32:17 CET 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3061/#review4295
-----------------------------------------------------------
Apart from my comments to a particular places in a patch here are few general notes:
- Almost everywhere you use indentation of 2 spaces, while original files have 4 of them. Please stick to original indentation level, don't make it a mess :)
- I'd suggest you to clean trailing spaces (highlighted in red by reviewboard)
- I'm not sure if you really should use that groupbox in the preferences page. IMHO it only adds bloat to the interface. I'd just use a QLabel instead.
/trunk/KDE/kdegames/kbounce/backgroundselector.cpp
<http://reviewboard.kde.org/r/3061/#comment3787>
Also: why do you set this string (and the one below) here, not in ui file?
/trunk/KDE/kdegames/kbounce/backgroundselector.cpp
<http://reviewboard.kde.org/r/3061/#comment3788>
I beleive this function should be called automatically in the generated ui class, no need to repeat it here :)
/trunk/KDE/kdegames/kbounce/backgroundselector.cpp
<http://reviewboard.kde.org/r/3061/#comment3789>
Seems that using toggled(bool) will be simpler, though the current way isn't wrong :)
/trunk/KDE/kdegames/kbounce/backgroundselector.cpp
<http://reviewboard.kde.org/r/3061/#comment3790>
Hint: You could use pix.fill(Qt::transparent)
/trunk/KDE/kdegames/kbounce/backgroundselector.cpp
<http://reviewboard.kde.org/r/3061/#comment3791>
And here: Qt::red :)
/trunk/KDE/kdegames/kbounce/board.cpp
<http://reviewboard.kde.org/r/3061/#comment3792>
I'd suggest to keep indentation at 4 spaces as in all other parts of the file
/trunk/KDE/kdegames/kbounce/board.cpp
<http://reviewboard.kde.org/r/3061/#comment3793>
And also use kFatal/kDebug instead qFatal/qDebug
/trunk/KDE/kdegames/kbounce/gamewidget.cpp
<http://reviewboard.kde.org/r/3061/#comment3794>
You messed up indentation here too. Please respect the current indentation settings.
- Dmitry
On 2010-02-26 16:10:29, Andreas Scherf wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3061/
> -----------------------------------------------------------
>
> (Updated 2010-02-26 16:10:29)
>
>
> Review request for KDE Games.
>
>
> Summary
> -------
>
> Added random background changing to kbounce. Wish: 221717
> And i fixed one issue of BUG:228063 were resizing did not pause the collision detection.
> And initial work for wish: 219507
>
>
> This addresses bugs 219507, 221717 and 228063.
> https://bugs.kde.org/show_bug.cgi?id=219507
> https://bugs.kde.org/show_bug.cgi?id=221717
> https://bugs.kde.org/show_bug.cgi?id=228063
>
>
> Diffs
> -----
>
> /trunk/KDE/kdegames/kbounce/CMakeLists.txt 1096082
> /trunk/KDE/kdegames/kbounce/backgroundselector.h PRE-CREATION
> /trunk/KDE/kdegames/kbounce/backgroundselector.cpp PRE-CREATION
> /trunk/KDE/kdegames/kbounce/backgroundselector.ui PRE-CREATION
> /trunk/KDE/kdegames/kbounce/ball.cpp 1096082
> /trunk/KDE/kdegames/kbounce/board.h 1096082
> /trunk/KDE/kdegames/kbounce/board.cpp 1096082
> /trunk/KDE/kdegames/kbounce/gamewidget.h 1096082
> /trunk/KDE/kdegames/kbounce/gamewidget.cpp 1096082
> /trunk/KDE/kdegames/kbounce/kbounce.kcfg 1096082
> /trunk/KDE/kdegames/kbounce/mainwindow.h 1096082
> /trunk/KDE/kdegames/kbounce/mainwindow.cpp 1096082
> /trunk/KDE/kdegames/kbounce/renderer.h 1096082
> /trunk/KDE/kdegames/kbounce/renderer.cpp 1096082
> /trunk/KDE/kdegames/kbounce/wall.cpp 1096082
>
> Diff: http://reviewboard.kde.org/r/3061/diff
>
>
> Testing
> -------
>
>
> Screenshots
> -----------
>
>
> http://reviewboard.kde.org/r/3061/s/322/
>
> http://reviewboard.kde.org/r/3061/s/323/
>
>
> Thanks,
>
> Andreas
>
>
More information about the kde-games-devel
mailing list