[Freeciv-Dev] (PR#10026) bad behavior with cityrep's "add first"

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [jdorje - Fri Sep 10 18:48:30 2004]:
> 
> I kept finding my worklists really long and full of useless items I 
> didn't remember adding.
> 
> It turns out if you "add first" an item, and that city can't build that 
> item (like adding a marketplace to a city with a marketplace) it instead 
> duplicates the current item.
> 
> Of course you can't add a marketplace to a group of selected cities 
> unless at least one of those cities can build the marketplace.
> 
> This is with the effects patch, but I doubt this makes a difference.

This patch should solve it.

I add a new function, city_queue_insert into citydlg_common.h.  This
function is used when adding entries to the queue in the cityreport.  It
does some minimal checking that should prevent almost all problems (I hope).

However since the client doesn't have all the info the server has, and
the operation of inserting something at the head of the queue is not
atomic, there is still the possibility for failure.  The most likely
case is still where we try to insert something that we can't build.  In
this case the current production gets moved down to the second item in
the queue (the top of the worklist), then the city production is changed
to the new item.  But this last step fails so the queue ends up being
incorrectly modified.

jason

? drawn_sprites.diff
? freeciv.spec
? ftwl.diff
? settler_recursion_crash
? client/tilespec.diff
Index: client/citydlg_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/citydlg_common.c,v
retrieving revision 1.47
diff -u -r1.47 citydlg_common.c
--- client/citydlg_common.c     13 Sep 2004 15:54:49 -0000      1.47
+++ client/citydlg_common.c     16 Sep 2004 20:15:36 -0000
@@ -480,6 +480,8 @@
 
 /**************************************************************************
   Set the worklist for a given city.  Return the request ID.
+
+  Note that the worklist does NOT include the current production.
 **************************************************************************/
 int city_set_worklist(struct city *pcity, struct worklist *pworklist)
 {
@@ -494,6 +496,64 @@
 }
 
 /**************************************************************************
+  Insert an item into the city's worklist.
+
+  Note that the queue DOES include the current production.
+**************************************************************************/
+bool city_queue_insert(struct city *pcity, int position,
+                      bool item_is_unit, int item_id)
+{
+  if (position == 0) {
+    int old_id;
+    bool old_is_unit;
+
+    /* Insert as current production. */
+    if (item_is_unit && !can_build_unit_direct(pcity, item_id)) {
+      return FALSE;
+    }
+    if (!item_is_unit && !can_build_improvement_direct(pcity, item_id)) {
+      return FALSE;
+    }
+
+    old_id = pcity->currently_building;
+    old_is_unit = pcity->is_building_unit;
+    if (!worklist_insert(&pcity->worklist, old_id, old_is_unit, 0)) {
+      return FALSE;
+    }
+
+    city_set_worklist(pcity, &pcity->worklist);
+    city_change_production(pcity, item_is_unit, item_id);
+  } else if (position >= 1
+            && position <= worklist_length(&pcity->worklist)) {
+    /* Insert into middle. */
+    if (item_is_unit && !can_eventually_build_unit(pcity, item_id)) {
+      return FALSE;
+    }
+    if (!item_is_unit && !can_eventually_build_improvement(pcity, item_id)) {
+      return FALSE;
+    }
+    if (!worklist_insert(&pcity->worklist, item_is_unit, item_id,
+                        position - 1)) {
+      return FALSE;
+    }
+    city_set_worklist(pcity, &pcity->worklist);
+  } else {
+    /* Insert at end. */
+    if (item_is_unit && !can_eventually_build_unit(pcity, item_id)) {
+      return FALSE;
+    }
+    if (!item_is_unit && !can_eventually_build_improvement(pcity, item_id)) {
+      return FALSE;
+    }
+    if (!worklist_append(&pcity->worklist, item_is_unit, item_id)) {
+      return FALSE;
+    }
+    city_set_worklist(pcity, &pcity->worklist);
+  }
+  return TRUE;
+}
+
+/**************************************************************************
   Get the city current production and the worklist, like it should be.
 **************************************************************************/
 void city_get_queue(struct city *pcity, struct worklist *pqueue)
Index: client/citydlg_common.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/citydlg_common.h,v
retrieving revision 1.25
diff -u -r1.25 citydlg_common.h
--- client/citydlg_common.h     6 Sep 2004 02:05:30 -0000       1.25
+++ client/citydlg_common.h     16 Sep 2004 20:15:36 -0000
@@ -62,6 +62,8 @@
 
 int city_change_production(struct city *pcity, bool is_unit, int build_id);
 int city_set_worklist(struct city *pcity, struct worklist *pworklist);
+bool city_queue_insert(struct city *pcity, int position,
+                      bool item_is_unit, int item_id);
 void city_get_queue(struct city *pcity, struct worklist *pqueue);
 void city_set_queue(struct city *pcity, struct worklist *pqueue);
 bool city_can_buy(const struct city *pcity);
Index: client/gui-gtk-2.0/cityrep.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk-2.0/cityrep.c,v
retrieving revision 1.58
diff -u -r1.58 cityrep.c
--- client/gui-gtk-2.0/cityrep.c        1 May 2004 17:28:47 -0000       1.58
+++ client/gui-gtk-2.0/cityrep.c        16 Sep 2004 20:15:36 -0000
@@ -32,7 +32,7 @@
 #include "unit.h"
 
 #include "chatline.h"
-#include "citydlg.h"
+#include "citydlg_common.h"
 #include "cityrepdata.h"
 #include "civclient.h"
 #include "clinet.h"
@@ -46,9 +46,11 @@
 #include "repodlgs.h"
 #include "climisc.h"
 
-#include "cityrep.h"
 #include "cma_fec.h"
 
+#include "citydlg.h"
+#include "cityrep.h"
+
 #define NEG_VAL(x)  ((x)<0 ? (x) : (-x))
 #define CMA_NONE       (-1)
 #define CMA_CUSTOM     (-2)
@@ -338,10 +340,8 @@
   gtk_tree_model_get(model, it, 1, &id, -1);
   pcity = find_city_by_id(id);
 
-  /* try to add the object to the worklist. */
-  if (worklist_append(&pcity->worklist, cid_id(cid), cid_is_unit(cid))) {
-    city_set_worklist(pcity, &pcity->worklist);
-  } /* perhaps should warn the user if not successful? */
+  (void) city_queue_insert(pcity, cid_is_unit(cid), cid_id(cid), -1);
+  /* perhaps should warn the user if not successful? */
 }
 
 /****************************************************************
@@ -360,22 +360,12 @@
   cid cid = GPOINTER_TO_INT(data);
   gint id;
   struct city *pcity;  
-  int old_id;
-  bool old_is_build_id_unit_id;
 
   gtk_tree_model_get(model, it, 1, &id, -1);
   pcity = find_city_by_id(id);
 
-  old_id = pcity->currently_building;
-  old_is_build_id_unit_id = pcity->is_building_unit;
-
-  /* first try and insert the old production into the worklist. */
-  if (worklist_insert(&pcity->worklist, old_id, old_is_build_id_unit_id, 0)) {
-    city_set_worklist(pcity, &pcity->worklist);
-
-    /* next change the current production to the new production. */
-    city_change_production(pcity, cid_is_unit(cid), cid_id(cid));
-  }
+  (void) city_queue_insert(pcity, 0, cid_is_unit(cid), cid_id(cid));
+  /* perhaps should warn the user if not successful? */
 }
 
 /****************************************************************
@@ -396,12 +386,10 @@
   gtk_tree_model_get(model, it, 1, &id, -1);
   pcity = find_city_by_id(id);
 
-  if (worklist_insert(&pcity->worklist, cid_id(cid), cid_is_unit(cid), 0)) {
-    city_set_worklist(pcity, &pcity->worklist);
-  }
+  city_queue_insert(pcity, 1, cid_is_unit(cid), cid_id(cid));
+  /* perhaps should warn the user if not successful? */
 }
 
-
 /****************************************************************
 ...
 *****************************************************************/