[Kde-games-devel] Review Request: Hint in Kiriki

[Parker Coates]

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



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

    This still hasn't been fixed/removed.



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7741>

    This phrasing is slightly awkward. Maybe "Asking for a hint will disqualify the current game from entering the high score list." would be better?



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7742>

    Personally, I like such dialogs to have informative titles. Maybe "i18n("Confirm Hint Request")"?



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7743>

    Button labels typically capitalise each word, so this should be "Give Hint Anyway".



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7744>

    Did you consider adding a "Do not ask again" checkbox to the dialog? It's generally a good idea for "nagging" dialogs like this one.



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7746>

    I think it makes sense to insert a "m_scoresWidget->selectionModel()->clearSelection();" here. That way there won't be stuff selected in both the dice and the score widget at the same time.



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7747>

    There's no need to create a whole new QItemSelectionModel here. We can just modify the existing selection. We can also take a shortcut and highlight the entire row with just a single index. I would suggest something like this:
    
    int row = scoreRows[ComputerScoring(m_scores -> currentPlayer())];
    QModelIndex mi = m_scores->index(row, 0);
    m_highlightedRowIndex  = mi.row();
    m_scoresWidget -> setItemDelegateForRow(m_highlightedRowIndex, m_delegateHighlighted);
    
    QItemSelectionModel::SelectionFlags flags = QItemSelectionModel::Clear
                                                | QItemSelectionModel::Select
                                                | QItemSelectionModel::Rows;
    m_scoresWidget->selectionModel()->select( QItemSelection(mi, mi), flags );



/trunk/KDE/kdegames/kiriki/src/kiriki.cpp
<http://svn.reviewboard.kde.org/r/3793/#comment7748>

    Personally I would move the "m_hintGiven = false;" to kiriki::newGame() as that's where things are typically reset.


The biggest remaining issue that I can see that there is currently no restriction on when hints can be requested. One can trigger the Hint action during a computer players turn and also after the game has ended. Ideally the hint action would be disabled in such conditions, but at a minimum it should be ignored if triggered when it makes no sense to give a hint.

- Parker


On 2010-07-15 16:13:54, Luiz Romário Santana Rios wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/3793/
> -----------------------------------------------------------
> 
> (Updated 2010-07-15 16:13:54)
> 
> 
> Review request for KDE Games.
> 
> 
> Summary
> -------
> 
> This patch implements hint in Kiriki, using the computer itself to give it. Either some dice or a row are highlighted to give the hint.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/kiriki/src/diceswidget.h 1119312 
>   /trunk/KDE/kdegames/kiriki/src/diceswidget.cpp 1119312 
>   /trunk/KDE/kdegames/kiriki/src/kiriki.h 1119312 
>   /trunk/KDE/kdegames/kiriki/src/kiriki.cpp 1119312 
>   /trunk/KDE/kdegames/kiriki/src/lateralwidget.h 1119312 
>   /trunk/KDE/kdegames/kiriki/src/lateralwidget.cpp 1119312 
> 
> Diff: http://svn.reviewboard.kde.org/r/3793/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Luiz Romário
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-games-devel/attachments/20100914/700100ba/attachment-0001.htm