<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40480 >

Both the aifill and autotoggle setting were using the setting
value validation callbacks to perform side-effects, contrary
to the expected behaviour of these callbacks in the body
of the set_command function. That is to say, they would make
changes to the server state even if check==TRUE (i.e. when
the set command should only check if the arguments are valid
and not actually perform any actions).

This results in the incorrect behaviour as reported here:

(The patch in PR#40475 being used to allow voting of the
aifill command.)

Attached patch removes the validate callbacks and moves
their side-effects into the body of the set command where
the conditions for their execution are properly checked.
The drawback is that since a 'setting' has no unique 
identifier (unlike commands, e.g. CMD_SET), then a rather
brittle check on the setting value address is made in
order to determine which setting side-effects should be
executed. For example, to know if we are to handle the
aifill side-effects, we check

op->int_value == &game.info.aifill

This has the disadvantage of being a hard-coded reference
to the pointer given in the setting definition, but other-
wise is as fast as a unique identifier check without
requiring a re-design of the setting data structures to
incorporate unique identifiers.

A FIXME comment is left in the relevant code section for
anyone who would care to think of a better work-around or
redesign the setting data structures to remove the need
for the pointer reference.

diff --git a/server/settings.c b/server/settings.c
index e1bb83b..3937593 100644
--- a/server/settings.c
+++ b/server/settings.c
@@ -46,27 +46,6 @@ const char *sset_level_names[] = {N_("None"),
 const int OLEVELS_NUM = ARRAY_SIZE(sset_level_names);
-  A callback invoked when autotoggle is set.
-static bool autotoggle_callback(bool value, const char **reject_message)
-  reject_message = NULL;
-  if (!value) {
-    return TRUE;
-  }
-  players_iterate(pplayer) {
-    if (!pplayer->ai.control && !pplayer->is_connected) {
-      toggle_ai_player_direct(NULL, pplayer);
-      send_player_info_c(pplayer, game.est_connections);
-    }
-  } players_iterate_end;
-  return TRUE;
   Verify that a given allowtake string is valid.  See
@@ -186,16 +165,6 @@ static bool maxplayers_callback(int value, const char **error_string)
   return TRUE;
-  Create/remove players when aifill is set.
-static bool aifill_callback(int value, const char **error_string)
-  aifill(value);
-  error_string = NULL;
-  return TRUE;
 #define GEN_BOOL(name, value, sclass, scateg, slevel, to_client,	\
 		 short_help, extra_help, func, default)			\
   {name, sclass, to_client, short_help, extra_help, SSET_BOOL,		\
@@ -435,7 +404,7 @@ struct settings_s settings[] = {
 	  N_("Total number of players (including AI players)"),
 	  N_("If set to a positive value, then AI players will be "
 	     "automatically created or removed to keep the total "
-	     "number of players at this amount."), aifill_callback,
+	     "number of players at this amount."), NULL,
   /* Game initialization parameters (only affect the first start of the game,
@@ -870,7 +839,7 @@ struct settings_s settings[] = {
 	   N_("Whether AI-status toggles with connection"),
 	   N_("If this is set to 1, AI status is turned off when a player "
 	      "connects, and on when a player disconnects."),
-	   autotoggle_callback, GAME_DEFAULT_AUTO_AI_TOGGLE)
   GEN_INT("endyear", game.info.end_year,
diff --git a/server/stdinhand.c b/server/stdinhand.c
index f9096ff..9bab7b9 100644
--- a/server/stdinhand.c
+++ b/server/stdinhand.c
@@ -2960,6 +2960,23 @@ static bool set_command(struct connection *caller, char *str, bool check)
   if (!check && do_update) {
+    /* Handle immediate side-effects of special setting changes. */
+    /* FIXME: Redesign setting data structures so that this can
+     * be done in a less brittle way. */
+    if (op->int_value == &game.info.aifill) {
+      aifill(*op->int_value);
+    } else if (op->bool_value == &game.info.auto_ai_toggle) {
+      if (*op->bool_value) {
+        players_iterate(pplayer) {
+          if (!pplayer->ai.control && !pplayer->is_connected) {
+            toggle_ai_player_direct(NULL, pplayer);
+            send_player_info_c(pplayer, game.est_connections);
+          }
+        } players_iterate_end;
+      }
+    }
     send_server_setting(NULL, cmd);
      * send any modified game parameters to the clients -- if sent
Freeciv-dev mailing list

Reply via email to