[Kde-games-devel] Review Request: Demo mode in Kiriki

[Parker Coates]

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


Hello, Luiz. Thanks for the patch. Below are some issues I found while reading through the patch. (I haven't actually tested it yet.) There are quite a few points below, but please don't be discouraged. If you have any questions, feel free to ask.


/trunk/KDE/kdegames/kiriki/src/kiriki.h
<http://reviewboard.kde.org/r/3573/#comment4457>

    This variable isn't actually needed. Instead of setting m_justStarted in the constructor and checking it in every time newGame() is called, just check the setting at the end of the constructor and call either demo() or newGame() accordingly.



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://reviewboard.kde.org/r/3573/#comment4459>

    Does this code actually work as expected? Once the action has been disabled, how does it ever get re-enabled?



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://reviewboard.kde.org/r/3573/#comment4458>

    Adding a toolbar is unrelated to the feature added by this patch, so I think it should be removed.
    
    Personally, I don't think a toolbar is necessary for just to actions. I would imagine it was Albert's original intention to leave out the toolbar to keep the interface clean.



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://reviewboard.kde.org/r/3573/#comment4456>

    This array can just be a local variable. There's no need to create it on the heap.



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://reviewboard.kde.org/r/3573/#comment4461>

    I'm not sure this is a good idea. After finishing a game, I would not expect a new demo to start after 3 seconds without warning. I could see this being quite confusing to users.



/trunk/KDE/kdegames/kiriki/src/kiriki.kcfg
<http://reviewboard.kde.org/r/3573/#comment4463>

    I'm not sure if enabling this by default is a good idea or not. I think it could be confusing for user if they don't realise that it's a demo that they're seeing. The might sit there waiting for "their" turn, which will never come. ;)



/trunk/KDE/kdegames/kiriki/src/kirikiui.rc
<http://reviewboard.kde.org/r/3573/#comment4462>

    As mentioned above, I don't think the addition of the toolbar is a good idea, so all changes to the ui.rc should be reverted.
    
    But in any case, you incremented the ui.rc version from 1 to 5. I know how this can happen while developing, as one is constantly trying out new versions, but before committing, you should check that the version numbers increment in a sensible order.



/trunk/KDE/kdegames/kiriki/src/row.cpp
<http://reviewboard.kde.org/r/3573/#comment4460>

    You are correct that using an initialiser list is a good idea here, but this change is unrelated to the original goal of this patch.
    
    Please try to keep each patch down to a single bug fix, feature, or improvement as it makes reviewing much easier and keeps the VCS history cleaner. Six ten line patches are generally much preferred to one thirty line patch.


- Parker


On 2010-04-12 17:53:41, Luiz Romário Santana Rios wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3573/
> -----------------------------------------------------------
> 
> (Updated 2010-04-12 17:53:41)
> 
> 
> Review request for KDE Games.
> 
> 
> Summary
> -------
> 
> This patch implements demonstration game in Kiriki. Almost complete.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/kiriki/src/configPage.ui 1113356 
>   /trunk/KDE/kdegames/kiriki/src/kiriki.h 1113356 
>   /trunk/KDE/kdegames/kiriki/src/kiriki.cpp 1113356 
>   /trunk/KDE/kdegames/kiriki/src/kiriki.kcfg 1113356 
>   /trunk/KDE/kdegames/kiriki/src/kirikiui.rc 1113356 
>   /trunk/KDE/kdegames/kiriki/src/row.cpp 1113356 
>   /trunk/KDE/kdegames/kiriki/src/scores.cpp 1113356 
> 
> Diff: http://reviewboard.kde.org/r/3573/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Luiz Romário
> 
>