Thanks for the suggestion. This is something someone can implement during the 2.19.x development cycle.

Created attachment 91529 [details] [review]
Adds undo button to Game menu

This is an initial implementation. This currently allows undo at any point, but I could see that perhaps we would only want to allow undo when the player hasn't made a roll on the current turn. This also only allows undo; would it make sense to allow a redo also ?

Created attachment 91772 [details] [review]
Updated undo/redo patch

This implementation supports both undo and redo.

Thanks for the patch. There is currently a string and Ui announcement period, so this patch will have to be committed during the 2.21.x development cycle.

This patch is now ready for inclusion. 

Thomas, would you like to commit it?

I have a few comments from trying it out.

1) Undo/redo remain sensitive in the menu after a new game has been started

2) roll some good dice (say 1,2,3,4,5)
   chose a score
   dice now show a new random roll (say 6,5,4,3,2)
   undo
   dice are back to 1,2,3,4,5 and score undone
   redo
   score is back, but dice now show 1,2,3,4,5

   This should come back to 6.5.4.3.2

This should be fixed before the patch is committed at least. I didn't have the time for a proper review of the code tonight. I'll look more closely at it during the weekend

I don't think that we should include redo at all. The reason to even have undo is if the user has chosen a wrong score by mistake. So just one level of undo and only if the dice have not been rolled after it. Also we want to remember the new roll so that this can't be used to cheat.

I looked at your implementation with many levels of undo/redo. It's nice work but unfortunately more than what is needed. 

You should also be sure to keep the same coding style as the code you are modifying. 

I'll probably make a new patch based on your work soon. Unless you beat me to it :)

Created attachment 97956 [details] [review]
Simplified undo patch

This is a simplified (and I hope improved) undo patch. Undo has the following characteristics:
1. Undo only refers to playing a score. You can't undo a decision to roll dice.
2. You can undo at most one human move.
3. Once the next human rolls once, you can no longer undo.
4. Any computer turns that are undone will be redone as they originally were when you play a new score.
5. The roll shown for the next human player will be restored when you play a new score. So you can't undo and redo to the same (or a different) score slot to generate a new roll for the next human player.

My last patch also doesn't display a redo option to the user. The code will still redo computer turns when appropriate, but the user can't "Redo". Instead, (s)he can just put the score back in the same category.

Perfect - and just in time to get in before 2.21.1 on monday. I'll review the patch tomorrow as I won't have the time before. But it sounds promising and your last work was good so I'm confident it'll get in. Good work Geoff!

I just tested it and it works nicely. I only have a few comments:

1) playing with computer delay turned on lets you undo while the computer is playing. This breaks the feature. I suggest we don't make the undo-option sensitive unless current player is human.

2) The comment in clist.c no longer describe the behavior.

3) Indentation. You use 4 spaces rather than 2 for the functions you add. We should use consistent indentation in the code.

All in all good work. I'll commit your patch with fixes for issues 2 and 3. Number 1 I'm leaving to you :) If you can find the time to fix it before the 2.21.1 release tomorrow night it would be great - if not we'll fix it for 2.21.2 instead.

And btw thanks for the various coding style fixes you included too. Keep up the good work :)

Committed. I'm keeping this bug open undtil the last issue has been fixed as well.

Created attachment 98049 [details] [review]
Disable Undo option when a computer is playing

This patch turns off the Undo option when a computer is playing.

Nice :) committed.