shithub: puzzles

Info  •  Files  •  Log  •  Branches

Forbid undo-of-new-game after midend_set_config.

This is another situation in which the midend's state, at the time
midend_new_game tries to save it, is not such as to generate a viable
serialisation. In this case, it's because after the user enters a game
id into the Game > Specific or Game > Random Seed dialog box,
midend_set_config will have _already_ started overwriting important
fields of the midend such as me->desc and me->seed, before
midend_new_game is even entered.

In fact this caused an assertion failure (thanks to Lennard Sprong for
reporting it), because one of those fields was set to NULL, so the
resulting serialisation buffer was incomplete, leading to a
deserialisation error later. But I was lucky: if I hadn't been, the
serialisation might have been complete, and valid according to the
deserialisation code, but would have contained a mixture of the new
game's description and the old one's move chain, which would surely
have had far stranger results.

For the moment, I'm fixing this by simply not storing a serialisation
in that situation. Perhaps a nicer approach might be to store one
before starting to overwrite midend fields, and then have
midend_new_game swap it in if it's already been generated. That way
you _could_ still undo past an action like this. But preventing the
assertion failure is a good start.

--- a/midend.c

+++ b/midend.c

@@ -69,6 +69,7 @@

     struct midend_state_entry *states;

     struct midend_serialise_buf newgame_undo, newgame_redo;

+    int newgame_can_store_undo;

     game_params *params, *curparams;

     game_drawstate *drawstate;

@@ -166,6 +167,7 @@

     me->newgame_undo.size = me->newgame_undo.len = 0;

     me->newgame_redo.buf = NULL;

     me->newgame_redo.size = me->newgame_redo.len = 0;

+    me->newgame_can_store_undo = FALSE;

     me->params = ourgame->default_params();

     me->game_id_change_notify_function = NULL;

     me->game_id_change_notify_ctx = NULL;

@@ -410,17 +412,20 @@

 void midend_new_game(midend *me)

 {

     me->newgame_undo.len = 0;

-    if (me->nstates != 0) {

+    if (me->newgame_can_store_undo) {

         /*

          * Serialise the whole of the game that we're about to

          * supersede, so that the 'New Game' action can be undone

-         * later. But if nstates == 0, that means there _isn't_ a

-         * current game (not even a starting position), because this

-         * is the initial call to midend_new_game when the midend is

-         * first set up; in that situation, we want to avoid writing

-         * out any serialisation, because it would be useless anyway

-         * and just confuse us into thinking we had something to undo

-         * to.

+         * later.

+         *

+         * We omit this in various situations, such as if there

+         * _isn't_ a current game (not even a starting position)

+         * because this is the initial call to midend_new_game when

+         * the midend is first set up, or if the midend state has

+         * already begun to be overwritten by midend_set_config. In

+         * those situations, we want to avoid writing out any

+         * serialisation, because they will be either invalid, or

+         * worse, valid but wrong.

          */

         midend_purge_states(me);

         midend_serialise(me, newgame_serialise_write, &me->newgame_undo);

@@ -536,6 +541,8 @@

     if (me->game_id_change_notify_function)

         me->game_id_change_notify_function(me->game_id_change_notify_ctx);

+

+    me->newgame_can_store_undo = TRUE;

 }

 int midend_can_undo(midend *me)

@@ -1679,6 +1686,8 @@

     }

     sfree(par);

+

+    me->newgame_can_store_undo = FALSE;

     return NULL;

 }