[Freeciv-Dev] Re: Pop cost patch (resending via bug system) (PR#897)

[Arien Malec <arien_malec@xxxxxxxxx>]

OK, updated patch attached. There are some oddities in the way the diff happend
(functions appear to move on diff, but don't really), but it all comes out
right in the end.

I've fixed the issues noted below, and also done the following things:

1) Renamed the version of unit_add_build... that returns the enum
unit_add_build_city_result
REASON: all of the can_... functions return a boolean, so naming this function
can_... is highly misleading

2) Reinstated the can_unit_add_to_city and can_unit_build_city functions, and
added a can_unit_add_build_city function as well, and made the GUI callers use
those functions
REASON: Makes things cleaner for the callers who just care about the boolean.
Most of the callers want the can_unit_add_build_city function, and were doing
an || (sometimes reduplicating the logic), so the combined boolean is cleaner
for them. (It is also more efficient, since we don't have to redo the work)

Arien
--- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Aug 23, 2001 at 08:19:52AM -0700, Arien Malec wrote:
> > 
> > --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > city_add_or_build_error is a static method. So there should be no
> > > problems. Add a comment in header and it would be fine. If there is
> > > some error we get a null exception.
> > 
> > I'll add the documentation.
> > 
> > > > diff -u -r1.6 cities.ruleset
> > > > --- data/default/cities.ruleset       2001/05/24 22:12:09     1.6
> > > > +++ data/default/cities.ruleset       2001/08/22 21:51:25
> > > > @@ -12,7 +12,7 @@
> > > >  options="1.9"
> > > >
> > > >  [parameters]
> > > > -add_to_size_limit  = 8               ; cities >= this cannot be added
> to.
> > > > +add_to_size_limit  = 18              ; cities >= this cannot be added
> to.
> > > >
> > > >  ;
> > > >  ; City styles define the way cities are drawn
> > > 
> > > Whats this?
> > 
> > Oops: testing cruft. I'll fix.
> >  
> > > > +static void city_build (struct player *pplayer, struct unit *punit,
> char
> > > *name)
> > > > +{
> > > > +  char *city_name;
> > > > +  if(!(city_name=get_sane_name(name))) {
> > > 
> > > I don't like assigment in a if condition.
> > 
> > Neither do I, but that's what it looked like before the cut 'n' paste, and
> I
> > was trying to adapt myself to the Freeciv style (which is really every
> person
> > to himself, I suppose). I'll fix.
> > 
> > > It really looks like we need a language style person.
> > 
> > Just a more complete style guide. Most of these things are questions of
> style,
> > and the style used doesn't matter so much as being consistent about it. I
> > prefer explicit to implicit, so I'd rather put the assignment on its own
> line,
> > and I'd use if (pointer == NULL) and (char == '\0') rather than if
> (pointer) or
> > if (char), but I can work either way...
> 
> Since style highly depends on personal taste I foresee a huge
> discussion. It is also impossible to make everybody happy. We should
> make a list of open issues and then just decide. Maybe by voting and a
> final decision by the maintainers?
> 
> > But some things matter a great deal: one of the really ugly things about
> > Freeciv currently is that documentation on functions (if it exists) is in
> the
> > implementation file, and not in the header file.
> 
> Yes this forces the programmer to always look at the method.
> 
> > There are certainly other big uglies out there (undocumented magic
> > numbers, etc., etc.)
> 
>  Raimar
> 
> -- 
>  email: rf13@xxxxxxxxxxxxxxxxx
>   Microsoft does have a year 2000 problem. I'm part of it. I'm running Linux.

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Index: client/gui-gtk/menu.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/menu.c,v
retrieving revision 1.55
diff -u -r1.55 menu.c
--- client/gui-gtk/menu.c       2001/07/02 19:51:20     1.55
+++ client/gui-gtk/menu.c       2001/08/23 17:14:35
@@ -304,9 +304,7 @@
       struct unit *punit = get_unit_in_focus();
       /* Enable the button for adding to a city in all cases, so we
         get an eventual error message from the server if we try. */
-      if (can_unit_build_city(punit)
-         || (unit_flag(punit->type, F_CITIES)
-             && map_get_city(punit->x, punit->y))) {
+      if (can_unit_add_build_city(punit)) {
        key_unit_build_city();
       } else {
        key_unit_build_wonder();
@@ -1000,9 +998,7 @@
       /* Enable the button for adding to a city in all cases, so we
         get an eventual error message from the server if we try. */
       menus_set_sensitive("<main>/_Orders/_Build City",
-                         (can_unit_build_city(punit) ||
-                           (unit_flag(punit->type, F_CITIES)
-                           && map_get_city(punit->x, punit->y)) ||
+                         (can_unit_add_build_city(punit) ||
                            unit_can_help_build_wonder_here(punit)));
       menus_set_sensitive("<main>/_Orders/Build _Road",
                           (can_unit_do_activity(punit, ACTIVITY_ROAD) ||
Index: client/gui-mui/gui_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/gui_main.c,v
retrieving revision 1.54
diff -u -r1.54 gui_main.c
--- client/gui-mui/gui_main.c   2001/08/18 20:33:06     1.54
+++ client/gui-mui/gui_main.c   2001/08/23 17:14:36
@@ -1270,7 +1270,8 @@
 
       set(main_menu, MUIA_Window_Menustrip, NULL);
 
-      menu_entry_sensitive(MENU_ORDER_BUILD_CITY, (can_unit_build_city(punit) 
|| can_unit_add_to_city(punit)));
+      menu_entry_sensitive(MENU_ORDER_BUILD_CITY,
+                          (can_unit_add_build_city(punit));
       menu_entry_sensitive(MENU_ORDER_ROAD, can_unit_do_activity(punit, 
ACTIVITY_ROAD) || can_unit_do_activity(punit, ACTIVITY_RAILROAD));
       menu_entry_sensitive(MENU_ORDER_IRRIGATE, can_unit_do_activity(punit, 
ACTIVITY_IRRIGATE));
       menu_entry_sensitive(MENU_ORDER_MINE, can_unit_do_activity(punit, 
ACTIVITY_MINE));
Index: client/gui-xaw/menu.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/menu.c,v
retrieving revision 1.45
diff -u -r1.45 menu.c
--- client/gui-xaw/menu.c       2001/06/29 19:39:04     1.45
+++ client/gui-xaw/menu.c       2001/08/23 17:14:37
@@ -309,8 +309,7 @@
       tinfo = get_tile_type(ttype);
 
       menu_entry_sensitive(MENU_ORDER, MENU_ORDER_BUILD_CITY, 
-                          (can_unit_build_city(punit) ||
-                           can_unit_add_to_city(punit)));
+                          (can_unit_add_build_city(punit)));
       menu_entry_sensitive(MENU_ORDER, MENU_ORDER_ROAD, 
                           can_unit_do_activity(punit, ACTIVITY_ROAD) ||
                           can_unit_do_activity(punit, ACTIVITY_RAILROAD));
Index: common/unit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.c,v
retrieving revision 1.125
diff -u -r1.125 unit.c
--- common/unit.c       2001/08/13 12:25:26     1.125
+++ common/unit.c       2001/08/23 17:14:38
@@ -388,60 +388,77 @@
   return 0;
 }
 
+/**************************************************************************
+...
+**************************************************************************/
+int kills_citizen_after_attack(struct unit *punit) {
+  return (game.killcitizen >> ((int)unit_types[punit->type].move_type-1)) & 1;
+}
 
 /**************************************************************************
 ...
 **************************************************************************/
-int can_unit_build_city(struct unit *punit)
+int can_unit_add_to_city (struct unit *punit)
 {
-  if(!unit_flag(punit->type, F_CITIES))
-    return 0;
-
-  if(!punit->moves_left)
-    return 0;
+  return (unit_add_build_city_result(punit) == AB_ADD_OK);
+}
 
-  return city_can_be_built_here(punit->x, punit->y);
+/**************************************************************************
+...
+**************************************************************************/
+int can_unit_build_city (struct unit *punit)
+{
+  return (unit_add_build_city_result(punit) == AB_BUILD_OK);
 }
 
 /**************************************************************************
 ...
 **************************************************************************/
-int kills_citizen_after_attack(struct unit *punit) {
-  return (game.killcitizen >> ((int)unit_types[punit->type].move_type-1)) & 1;
+int can_unit_add_build_city (struct unit *punit)
+{
+  enum add_build_city_result r = unit_add_build_city_result (punit);
+  return (r == AB_BUILD_OK || r == AB_ADD_OK);
 }
 
 /**************************************************************************
 ...
 **************************************************************************/
-int can_unit_add_to_city(struct unit *punit)
+enum add_build_city_result
+unit_add_build_city_result (struct unit *punit)
 {
   struct city *pcity;
-
-  if(!unit_flag(punit->type, F_CITIES))
-    return 0;
-  if(!punit->moves_left)
-    return 0;
-
   pcity = map_get_city(punit->x, punit->y);
 
-  if(!pcity)
-    return 0;
+  if(!unit_flag(punit->type, F_CITIES))
+    return AB_NOT_ADDABLE_UNIT;
+  if(!punit->moves_left) {
+    if (!pcity)
+      return AB_NO_MOVES_BUILD;
+    else
+      return AB_NO_MOVES_ADD;
+  }
+  if(!pcity) {
+    if (city_can_be_built_here (punit->x, punit->y))
+      return AB_BUILD_OK;
+    else
+      return AB_NOT_BUILD_LOC;
+  }
   if(pcity->size >= game.add_to_size_limit)
-    return 0;
+    return AB_TOO_BIG;
   if(pcity->owner != punit->owner)
-    return 0;
+    return AB_NOT_OWNER;
 
   if(improvement_exists(B_AQUEDUCT)
      && !city_got_building(pcity, B_AQUEDUCT) 
      && pcity->size >= game.aqueduct_size)
-    return 0;
+    return AB_NO_AQUEDUCT;
   
   if(improvement_exists(B_SEWER)
      && !city_got_building(pcity, B_SEWER)
      && pcity->size >= game.sewer_size)
-    return 0;
+    return AB_NO_SEWER;
 
-  return 1;
+  return AB_ADD_OK;
 }
 
 /**************************************************************************
Index: common/unit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.h,v
retrieving revision 1.72
diff -u -r1.72 unit.h
--- common/unit.h       2001/08/13 12:25:27     1.72
+++ common/unit.h       2001/08/23 17:14:38
@@ -66,6 +66,24 @@
   MR_OTHER, MR_INVALID_TYPE_FOR_CITY_TAKE_OVER, MR_NO_WAR, MR_ZOC
 };
 
+enum add_build_city_result {
+  AB_BUILD_OK,         /* Unit OK to build city */
+  AB_ADD_OK,           /* Unit OK to add to city */
+  AB_NOT_BUILD_LOC,    /* City is not allowed to be built at this
+                         location */
+  AB_NOT_ADDABLE_UNIT, /* Unit is not one that can be added to cities */
+  AB_NO_MOVES_BUILD,   /* Unit does not have moves left to build a
+                         city */
+  AB_NO_MOVES_ADD,     /* Unit does not have moves left to add to
+                         city */
+  AB_NOT_OWNER,        /* Owner of unit is not owner of city */
+  AB_TOO_BIG,          /* City is too big to be added to */
+  AB_NO_AQUEDUCT,      /* Adding takes city past limit for aquaduct
+                         but city has no aquaduct */
+  AB_NO_SEWER          /* Adding takes city past limit for sewer but
+                         city has no sewer */
+};
+
 struct unit_ai {
   int control; /* 0: not automated    1: automated */
   enum ai_unit_task ai_role;
@@ -185,8 +203,10 @@
 int is_air_unit(struct unit *punit);
 int is_heli_unit(struct unit *punit);
 int is_ground_unit(struct unit *punit);
-int can_unit_build_city(struct unit *punit);
-int can_unit_add_to_city(struct unit *punit);
+int can_unit_add_to_city (struct unit *punit);
+int can_unit_build_city (struct unit *punit);
+int can_unit_add_build_city (struct unit *punit);
+enum add_build_city_result unit_add_build_city_result (struct unit *punit);
 int kills_citizen_after_attack(struct unit *punit);
 
 char *unit_activity_text(struct unit *punit);
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.193
diff -u -r1.193 unithand.c
--- server/unithand.c   2001/08/08 09:46:26     1.193
+++ server/unithand.c   2001/08/23 17:14:39
@@ -52,6 +52,12 @@
 
 #include "unithand.h"
 
+static void city_add_or_build_error(struct player *pplayer,
+                                   struct unit *punit,
+                                   enum add_build_city_result res);
+static void city_add_unit (struct player *pplayer, struct unit *punit);
+static void city_build (struct player *pplayer, struct unit *punit, char 
*name);
+
 /**************************************************************************
 ...
 **************************************************************************/
@@ -402,105 +408,150 @@
 }
 
 /**************************************************************************
-...
+ This function assumes that there is a valid city at punit->(x,y) for
+ certain values of add_build_city_result.  It should only be called
+ after a call to unit_add_build_city_result, which does the
+ consistency checking
 **************************************************************************/
-void handle_unit_build_city(struct player *pplayer, 
-                           struct packet_unit_request *req)
+static void city_add_or_build_error(struct player *pplayer,
+                                   struct unit *punit,
+                                   enum add_build_city_result res)
 {
-  struct unit *punit;
-  char *unit_name;
   struct city *pcity;
-  char *name;
-  
-  punit = player_find_unit_by_id(pplayer, req->unit_id) ;
-  if(!punit)
-    return;
+  char *unit_name;
   
   unit_name = get_unit_type(punit->type)->name;
+  /* Given that res came from unit_add_build_city_result, pcity
+     will be non-null for all required status values */
   pcity = map_get_city(punit->x, punit->y);
-  
-  if (!unit_flag(punit->type, F_CITIES)) {
-    char *us = get_units_with_flag_string(F_CITIES);
+
+  switch (res) {
+  case AB_NOT_BUILD_LOC:
     notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                    _("Game: Only %s can build or add to a city."), us);
-    free(us);
+                    _("Game: Can't place city here."));
     return;
-  }  
-
-  if(!punit->moves_left)  {
-    if (pcity) {
+  case AB_NOT_ADDABLE_UNIT:
+    {
+      char *us = get_units_with_flag_string(F_CITIES);
       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                      _("Game: %s unit has no moves left to add to %s."),
-                      unit_name, pcity->name);
-    } else {
-      notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                      _("Game: %s unit has no moves left to build city."),
-                      unit_name);
+                      _("Game: Only %s can build or add to a city."), us);
+      free(us);
+      return;
     }
+  case AB_NO_MOVES_ADD:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                    _("Game: %s unit has no moves left to add to %s."),
+                    unit_name, pcity->name);
     return;
-  }
-    
-  if (pcity) {
-    if (can_unit_add_to_city(punit)) {
-      pcity->size++;
-      if (!add_adjust_workers(pcity)) {
-       auto_arrange_workers(pcity);
-       sync_cities();
-      }
-      wipe_unit(punit);
-      send_city_info(0, pcity);
-      notify_player_ex(pplayer, pcity->x, pcity->y, E_NOEVENT, 
-                      _("Game: %s added to aid %s in growing."), 
-                      unit_name, pcity->name);
-    } else {
-      if(pcity->size >= game.add_to_size_limit) {
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: %s is too big to add %s."),
-                        pcity->name, unit_name);
-      }
-      else if(improvement_exists(B_AQUEDUCT)
-        && !city_got_building(pcity, B_AQUEDUCT) 
-        && pcity->size >= game.aqueduct_size) {
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: %s needs %s to grow, so you cannot add %s."),
-                        pcity->name, get_improvement_name(B_AQUEDUCT),
-                        unit_name);
-      }
-      else if(improvement_exists(B_SEWER)
-             && !city_got_building(pcity, B_SEWER)
-             && pcity->size >= game.sewer_size) {
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: %s needs %s to grow, so you cannot add %s."),
-                        pcity->name, get_improvement_name(B_SEWER),
-                        unit_name);
-      }
-      else {
-       /* this shouldn't happen? */
-       freelog(LOG_ERROR, "Cannot add %s to %s for unknown reason",
-               unit_name, pcity->name);
-       notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
-                        _("Game: cannot add %s to %s."),
-                        unit_name, pcity->name);
-      }
-    }
+  case AB_NO_MOVES_BUILD:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                    _("Game: %s unit has no moves left to build city."),
+                    unit_name);
     return;
+  case AB_TOO_BIG:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: %s is too big to add %s."),
+                    pcity->name, unit_name);
+    return;
+  case AB_NO_AQUEDUCT:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: %s needs %s to grow, so you cannot add %s."),
+                    pcity->name, get_improvement_name(B_AQUEDUCT),
+                    unit_name);
+    return;
+  case AB_NO_SEWER:
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: %s needs %s to grow, so you cannot add %s."),
+                    pcity->name, get_improvement_name(B_SEWER),
+                    unit_name);
+    return;
+  default:
+    /* Shouldn't happen */
+    freelog(LOG_ERROR, "Cannot add %s to %s for unknown reason",
+           unit_name, pcity->name);
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
+                    _("Game: cannot add %s to %s."),
+                    unit_name, pcity->name);
   }
-    
-  if(!can_unit_build_city(punit)) {
-      notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                      _("Game: Can't place city here."));
-      return;
+  return;
+}
+
+/**************************************************************************
+ This function assumes that there is a valid city at punit->(x,y) It
+ should only be called after a call to a function like
+ unit_add_build_city_result, which does the checking before a return
+ value of AB_ADD_OK
+**************************************************************************/
+static void city_add_unit (struct player *pplayer,
+                          struct unit *punit)
+{
+  struct city *pcity;
+  char *unit_name;
+  pcity = map_get_city(punit->x, punit->y);
+  unit_name = get_unit_type(punit->type)->name;
+
+  pcity->size++;
+  if (!add_adjust_workers(pcity)) {
+    auto_arrange_workers(pcity);
+    sync_cities();
   }
-      
-  if(!(name=get_sane_name(req->name))) {
+  wipe_unit(punit);
+  send_city_info(0, pcity);
+  notify_player_ex(pplayer, pcity->x, pcity->y, E_NOEVENT, 
+                  _("Game: %s added to aid %s in growing."), 
+                  unit_name, pcity->name);
+  return;
+}
+
+/**************************************************************************
+ This function assumes a certain level of consistency checking: There
+ is no city under punit->(x,y), and that location is a valid one on
+ which to build a city. It should only be called after a call to a
+ function like unit_add_build_city_result, which does the checking
+**************************************************************************/
+static void city_build (struct player *pplayer, struct unit *punit, char *name)
+{
+  char *city_name;
+
+  city_name=get_sane_name(name);
+  if(!city_name) {
     notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT, 
                     _("Game: Let's not build a city with such a stupid 
name."));
     return;
   }
-
-  create_city(pplayer, punit->x, punit->y, name);
+  create_city(pplayer, punit->x, punit->y, city_name);
   wipe_unit(punit);
+  return;
+}
+
+
+/**************************************************************************
+...
+**************************************************************************/
+void handle_unit_build_city(struct player *pplayer, 
+                           struct packet_unit_request *req)
+{
+  struct unit *punit;
+  enum add_build_city_result res;
+  
+  punit = player_find_unit_by_id(pplayer, req->unit_id) ;
+  if(!punit)
+    return;
+  
+  res = unit_add_build_city_result (punit);
+
+  if (res == AB_BUILD_OK)
+    city_build (pplayer, punit, req->name);
+  else if (res == AB_ADD_OK)
+    city_add_unit (pplayer, punit);
+  else
+    city_add_or_build_error (pplayer, punit, res);
+
+  return;
 }
+
+  
+
 
 /**************************************************************************
 ...