[Freeciv-Dev] (PR#2327) GUI problem with choosing government

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=2327 >

> [jdorje - Wed Jul 14 17:53:37 2004]:
> 
> > [ChrisK@xxxxxxxx - Wed Nov 13 20:09:27 2002]:
> > 
> > Stable CVS Gtk 1.2 ~10 NOV 2002
> > 
> > This happened to a game-mate recently:
> > 
> > While chatting, the "choose government" popup appears, and grabs
> > the keystrokes. A <space> or <return> chooses Despotism.
> > 
> > An expensive and frustrating mistake.
> > 
> > We could
> > 
> > - disable the keys for the popup at all
> > - disable <space> and <return>, and add meta keys instead
> > - someone wanted to make a new popup
> 
> Nothing involving the government happens until the end of the turn.  So
> until the turn ends it should be possible to choose a new government
> without returning to anarchy.

Here is a patch that implements this.

The pplayer->revolution ("turns until revolution is over") variable is
replaced with a pplayer->revolution_finishes ("turn # when the
revolution is over").  This is explained best in the comment in
update_revolution.

Savegames should be forward- and backward-compatible.

I tested it in every case I could think of, and it seems to work quite
well.  The new method is also logically cleaner, which should make it
possible to simplify the code greatly (see below).

This is a significant change!  Please review it.  Design people, you
have one last chance to object (everyone seems to be in favor of the
idea so far).

I will attach a useful savegame (one in which a revolution is ready) as
a comment.

-----

Ideas for future simplification:

- In most cases revolution-over need only be checked when the turn
advances.  This makes the client code simpler (it is ugly right now).

- It should now be possible to store the target government at the server
end.  The client sends a "request_new_government" packet including the
target government (which may be empty).  This packet will instigate a
revolution if necessary and/or change the government.  The distinction
between starting a revolution and choosing the government is no longer
needed because the order in which the two is done no longer matters.

jason

Index: ai/aitools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.c,v
retrieving revision 1.119
diff -u -r1.119 aitools.c
--- ai/aitools.c        14 Aug 2004 21:29:42 -0000      1.119
+++ ai/aitools.c        23 Aug 2004 05:25:29 -0000
@@ -634,10 +634,14 @@
   if (gov == pplayer->government) {
     return;
   }
-  pplayer->revolution = 0;
   pplayer->government = game.government_when_anarchy;
+
+  /* The AI cheats by having instant revolutions.
+   * TODO: turn this into a player handicap. */
+  pplayer->revolution_finishes = game.turn;
+
   handle_player_government(pplayer, gov);
-  pplayer->revolution = -1; /* yes, I really mean this. -- Syela */
+  /* The revolution_finishes value need not be reset at this point. */
   city_list_iterate(pplayer->cities, pcity) {
     auto_arrange_workers(pcity); /* update cities */
   } city_list_iterate_end;
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.396
diff -u -r1.396 packhand.c
--- client/packhand.c   20 Aug 2004 20:13:17 -0000      1.396
+++ client/packhand.c   23 Aug 2004 05:25:29 -0000
@@ -1614,7 +1614,7 @@
   pplayer->is_alive=pinfo->is_alive;
 
   pplayer->ai.barbarian_type = pinfo->barbarian_type;
-  pplayer->revolution=pinfo->revolution;
+  pplayer->revolution_finishes = pinfo->revolution_finishes;
   if(pplayer->ai.control!=pinfo->ai)  {
     pplayer->ai.control=pinfo->ai;
     if(pplayer==game.player_ptr)  {
@@ -1625,7 +1625,8 @@
   }
   
   if (pplayer == game.player_ptr
-      && (pplayer->revolution < 1 || pplayer->revolution > 5)
+      && pplayer->revolution_finishes >= 0
+      && pplayer->revolution_finishes <= game.turn
       && pplayer->government == game.government_when_anarchy
       && (!game.player_ptr->ai.control || ai_popup_windows)
       && can_client_change_view()
Index: common/capstr.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
retrieving revision 1.178
diff -u -r1.178 capstr.c
--- common/capstr.c     20 Aug 2004 20:13:17 -0000      1.178
+++ common/capstr.c     23 Aug 2004 05:25:30 -0000
@@ -78,7 +78,7 @@
                    "+change_production +tilespec1 +no_earth +trans " \
                    "+want_hack invasions bombard +killstack2 spec +spec2 " \
                    "+city_map startunits +turn_last_built +happyborders " \
-                   "+connid +love2 +ocean_num +govclean"
+                   "+connid +love2 +ocean_num +govclean +rev_fin"
 
 /* "+1.14.delta" is the new delta protocol for 1.14.0-dev.
  *
@@ -142,6 +142,9 @@
  * which are stored in ptile->continent and sent to client.
  * 
  * "govclean" removes corruption|waste_modifier
+ *
+ * "rev_fin" means the revolution_finishes value replaces the revolution
+ * value.
  */
 
 void init_our_capability(void)
Index: common/packets.def
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.def,v
retrieving revision 1.40
diff -u -r1.40 packets.def
--- common/packets.def  20 Aug 2004 20:13:17 -0000      1.40
+++ common/packets.def  23 Aug 2004 05:25:30 -0000
@@ -198,7 +198,7 @@
 type HP                = UINT8
 type PERCENT           = UINT8
 type GOLD              = UINT32
-type TURN              = UINT16
+type TURN              = SINT16
 
 /****************************************************
 The remaining lines are the definition of the packets. These are
@@ -553,7 +553,7 @@
   UINT16 future_tech;
   UINT8 tech_goal;
   BOOL is_connected;
-  UINT8 revolution;
+  TURN revolution_finishes;
   BOOL ai;
   UINT8 barbarian_type;
   uint32(unsigned int) gives_shared_vision;
Index: common/player.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.c,v
retrieving revision 1.144
diff -u -r1.144 player.c
--- common/player.c     1 Aug 2004 13:33:26 -0000       1.144
+++ common/player.c     23 Aug 2004 05:25:30 -0000
@@ -94,6 +94,7 @@
   plr->government=game.default_government;
   plr->nation = NO_NATION_SELECTED;
   plr->team = TEAM_NONE;
+  plr->revolution_finishes = -1;
   plr->capital = FALSE;
   unit_list_init(&plr->units);
   city_list_init(&plr->cities);
Index: common/player.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.h,v
retrieving revision 1.118
diff -u -r1.118 player.h
--- common/player.h     30 Jul 2004 20:40:49 -0000      1.118
+++ common/player.h     23 Aug 2004 05:25:30 -0000
@@ -188,7 +188,10 @@
   bool is_alive;
   bool is_dying; /* set once the player is in the process of dying */
   bool got_tech; /* set once the player is fully dead */
-  int revolution;
+
+  /* Turn in which the player's revolution is over; see update_revolution. */
+  int revolution_finishes;
+
   bool capital; /* used to give player init_buildings in first city. */
   int embassy;
   int reputation;
Index: server/plrhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/plrhand.c,v
retrieving revision 1.318
diff -u -r1.318 plrhand.c
--- server/plrhand.c    20 Aug 2004 00:03:35 -0000      1.318
+++ server/plrhand.c    23 Aug 2004 05:25:31 -0000
@@ -157,13 +157,35 @@
 }
 
 /**************************************************************************
-Count down if the player are in a revolution, notify him when revolution
-has ended.
+  See if the player has finished their revolution.  This function should
+  be called at the beginning of a player's phase.
 **************************************************************************/
 void update_revolution(struct player *pplayer)
 {
-  if (pplayer->revolution > 0) {
-    pplayer->revolution--;
+  /* The player's revolution counter is stored in the revolution_finishes
+   * field.  This value has the following meanings:
+   *   - If negative (-1), then the player is not in a revolution.  In this
+   *     case the player should never be in anarchy.
+   *   - If positive, the player is in the middle of a revolution.  In this
+   *     case the value indicates the turn in which the revolution finishes.
+   *     * If this value is > than the current turn, then the revolution is
+   *       in progress.  In this case the player should always be in anarchy.
+   *     * If the value is == to the current turn, then the revolution is
+   *       finished.  The player may now choose a government.  However the
+   *       value isn't reset until the end of the turn.  If the player has
+   *       chosen a government by the end of the turn, then the revolution is
+   *       over and the value is reset to -1.
+   *     * If the player doesn't pick a government then the revolution
+   *       continues.  At this point the value is <= to the current turn,
+   *       and the player can leave the revolution at any time.  The value
+   *       is reset at the end of any turn when a non-anarchy government is
+   *       chosen.
+   */
+  if (pplayer->revolution_finishes <= game.turn
+      && pplayer->government != game.government_when_anarchy) {
+    /* Reset the revolution counter.  If the player has another revolution
+     * they'll have to re-enter anarchy. */
+    pplayer->revolution_finishes = -1;
   }
 }
 
@@ -851,13 +873,13 @@
 **************************************************************************/
 void handle_player_government(struct player *pplayer, int government)
 {
-  if (pplayer->government != game.government_when_anarchy ||
-      government < 0 || government >= game.government_count ||
+  if (government < 0 || government >= game.government_count ||
       !can_change_to_government(pplayer, government)) {
     return;
   }
 
-  if (pplayer->revolution <= 5 && pplayer->revolution > 0) {
+  if (pplayer->revolution_finishes < 0
+      || pplayer->revolution_finishes > game.turn) {
     return;
   }
 
@@ -889,15 +911,18 @@
 **************************************************************************/
 void handle_player_revolution(struct player *pplayer)
 {
-  if (pplayer->revolution <= 5
-      && pplayer->revolution > 0
-      && pplayer->government == game.government_when_anarchy) {
+  if (pplayer->government == game.government_when_anarchy) {
+    /* Already having a revolution. */
     return;
   }
-  if (game.revolution_length == 0) {
-    pplayer->revolution = myrand(5) + 1;
-  } else {
-    pplayer->revolution = game.revolution_length;
+  if (pplayer->revolution_finishes < 0) {
+    /* Start a revolution from scratch (otherwise a revolution is in
+     * progress, even if the player isn't in anarchy). */
+    if (game.revolution_length == 0) {
+      pplayer->revolution_finishes = game.turn + myrand(5) + 1;
+    } else {
+      pplayer->revolution_finishes = game.revolution_length;
+    }
   }
   pplayer->government=game.government_when_anarchy;
   notify_player_ex(pplayer, -1, -1, E_REVOLT_START,
@@ -909,7 +934,7 @@
   check_player_government_rates(pplayer);
   global_city_refresh(pplayer);
   if (player_owns_active_govchange_wonder(pplayer)) {
-    pplayer->revolution = 0;
+    pplayer->revolution_finishes = game.turn;
   }
   send_player_info(pplayer, pplayer);
 }
@@ -1072,7 +1097,7 @@
     notify_player_ex(pplayer, -1, -1, E_TREATY_BROKEN,
                      _("Game: Your reputation is now %s."),
                      reputation_text(pplayer->reputation));
-    if (has_senate && pplayer->revolution == 0) {
+    if (has_senate && pplayer->revolution_finishes < 0) {
       if (myrand(GAME_MAX_REPUTATION) > pplayer->reputation) {
         notify_player_ex(pplayer, -1, -1, E_ANARCHY,
                          _("Game: The senate decides to dissolve "
@@ -1445,11 +1470,7 @@
     packet->techs_researched= plr->research.techs_researched;
     packet->researching     = plr->research.researching;
     packet->future_tech     = plr->future_tech;
-    if (plr->revolution != 0) {
-      packet->revolution    = 1;
-    } else {
-      packet->revolution    = 0;
-    }
+    packet->revolution_finishes = plr->revolution_finishes;
   } else {
     for (i = A_FIRST; i < game.num_tech_types; i++) {
       packet->inventions[i] = '0';
@@ -1462,7 +1483,7 @@
     packet->techs_researched= 0;
     packet->researching     = A_NOINFO;
     packet->future_tech     = 0;
-    packet->revolution      = 0;
+    packet->revolution_finishes = -1;
   }
 
   /* We have to inform the client that the other players also know
@@ -1798,7 +1819,7 @@
   sz_strlcpy(cplayer->username, ANON_USER_NAME);
   cplayer->is_connected = FALSE;
   cplayer->government = game.government_when_anarchy;  
-  cplayer->revolution = 1;
+  cplayer->revolution_finishes = game.turn + 1;
   cplayer->capital = TRUE;
 
   /* This should probably be DS_NEUTRAL when AI knows about diplomacy,
@@ -1854,7 +1875,7 @@
   /* change the original player */
   if (pplayer->government != game.government_when_anarchy) {
     pplayer->government = game.government_when_anarchy;
-    pplayer->revolution = 1;
+    pplayer->revolution_finishes = game.turn + 1;
   }
   pplayer->economic.gold = cplayer->economic.gold;
   pplayer->research.bulbs_researched = 0;
Index: server/sanitycheck.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/sanitycheck.c,v
retrieving revision 1.46
diff -u -r1.46 sanitycheck.c
--- server/sanitycheck.c        13 Aug 2004 15:59:13 -0000      1.46
+++ server/sanitycheck.c        23 Aug 2004 05:25:31 -0000
@@ -351,6 +351,15 @@
        assert(pplayer->diplstates[pplayer2->player_no].turns_left
               == pplayer2->diplstates[pplayer->player_no].turns_left);
     } players_iterate_end;
+
+    if (pplayer->revolution_finishes == -1) {
+      assert(pplayer->government != game.government_when_anarchy);
+    } else if (pplayer->revolution_finishes > game.turn) {
+      assert(pplayer->government == game.government_when_anarchy);
+    } else {
+      /* Things may vary in this case depending on when the sanity_check
+       * call is made.  No better check is possible. */
+    }
   } players_iterate_end;
 
   /* Sanity checks on living and dead players. */
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.176
diff -u -r1.176 savegame.c
--- server/savegame.c   18 Aug 2004 18:11:05 -0000      1.176
+++ server/savegame.c   23 Aug 2004 05:25:32 -0000
@@ -1605,8 +1605,25 @@
   }
     
   plr->capital = secfile_lookup_bool(file, "player%d.capital", plrno);
-  plr->revolution = secfile_lookup_int_default(file, 0, "player%d.revolution",
-                                              plrno);
+
+  {
+    /* The old-style "revolution" value indicates the number of turns until
+     * the revolution is complete, or 0 if there is no revolution.  The
+     * new-style "revolution_finishes" value indicates the turn in which
+     * the revolution will complete (which may be less than the current
+     * turn) or -1 if there is no revolution. */
+    int revolution = secfile_lookup_int_default(file, 0, "player%d.revolution",
+                                               plrno);
+
+    if (revolution == 0) {
+      revolution = -1;
+    } else {
+      revolution = game.turn + revolution;
+    }
+    plr->revolution_finishes
+      = secfile_lookup_int_default(file, revolution,
+                                  "player%d.revolution_finishes", plrno);
+  }
 
   update_research(plr);
 
@@ -2243,7 +2260,24 @@
                   plr->research.researching);  
 
   secfile_insert_bool(file, plr->capital, "player%d.capital", plrno);
-  secfile_insert_int(file, plr->revolution, "player%d.revolution", plrno);
+
+  secfile_insert_int(file, plr->revolution_finishes,
+                    "player%d.revolution_finishes", plrno);
+  {
+    /* Insert the old-style "revolution" value, for forward compatibility.
+     * See the loading code for more explanation. */
+    int revolution;
+
+    if (plr->revolution_finishes < 0) {
+      /* No revolution. */
+      revolution = 0;
+    } else if (plr->revolution_finishes <= game.turn) {
+      revolution = 1; /* Approximation. */
+    } else {
+      revolution = plr->revolution_finishes - game.turn;
+    }
+    secfile_insert_int(file, revolution, "player%d.revolution", plrno);
+  }
 
   /* 1.14 servers depend on technology order in ruleset. Here we are trying
    * to simulate 1.14.1 default order */
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.182
diff -u -r1.182 srv_main.c
--- server/srv_main.c   21 Aug 2004 18:32:37 -0000      1.182
+++ server/srv_main.c   23 Aug 2004 05:25:32 -0000
@@ -493,6 +493,10 @@
   flush_packets();  /* to curb major city spam */
   conn_list_do_unbuffer(&game.game_connections);
 
+  shuffled_players_iterate(pplayer) {
+    update_revolution(pplayer);
+  } shuffled_players_iterate_end;
+
   if (is_new_phase) {
     /* Try to avoid hiding events under a diplomacy dialog */
     players_iterate(pplayer) {
@@ -550,7 +554,6 @@
   /* Refresh cities */
   shuffled_players_iterate(pplayer) {
     great_library(pplayer);
-    update_revolution(pplayer);
     player_restore_units(pplayer);
     update_city_activities(pplayer);
     pplayer->research.changed_from=-1;