[Kde-games-devel] Review Request: Roll all dice when none is selected.

[Parker Coates]

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



/trunk/KDE/kdegames/kiriki/src/diceswidget.h
<http://svn.reviewboard.kde.org/r/4882/#comment7772>

    Giving the parameter a descriptive name is always a good idea, but especially here as it isn't obvious what the parameter actually means.
    
    Actually I think the whole signal is kind of awkward and overly specific. Why not something a bit more generic like "void diceSelectionChanged(int numberSelected);"



/trunk/KDE/kdegames/kiriki/src/diceswidget.cpp
<http://svn.reviewboard.kde.org/r/4882/#comment7774>

    Since the selection changes here, you'll probably also need to emit the diceSelectionChanged signal here.



/trunk/KDE/kdegames/kiriki/src/lateralwidget.cpp
<http://svn.reviewboard.kde.org/r/4882/#comment7771>

    Unused include.



/trunk/KDE/kdegames/kiriki/src/lateralwidget.cpp
<http://svn.reviewboard.kde.org/r/4882/#comment7773>

    Could you explain why you call updateRollButton(true) here?


- Parker


On 2010-08-10 15:49:31, Luiz Romário Santana Rios wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4882/
> -----------------------------------------------------------
> 
> (Updated 2010-08-10 15:49:31)
> 
> 
> Review request for KDE Games.
> 
> 
> Summary
> -------
> 
> This patch makes "Roll" button be labeled as "Roll all" and roll all dice in the lateral widget when no die is selected.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/kiriki/src/diceswidget.h 1158542 
>   /trunk/KDE/kdegames/kiriki/src/diceswidget.cpp 1158542 
>   /trunk/KDE/kdegames/kiriki/src/lateralwidget.h 1158542 
>   /trunk/KDE/kdegames/kiriki/src/lateralwidget.cpp 1158542 
> 
> Diff: http://svn.reviewboard.kde.org/r/4882/diff
> 
> 
> Testing
> -------
> 
> Works fine here.
> 
> 
> Thanks,
> 
> Luiz Romário
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-games-devel/attachments/20100915/054c21a4/attachment-0001.htm