[Freeciv-Dev] Re: (PR#2516) introducing an enum citizen_type

["Jason Short via RT" <rt@xxxxxxxxxxxxxx>]

On Sun, 2002-12-08 at 04:10, Raimar Falke via RT wrote:
> On Sat, Dec 07, 2002 at 08:43:46PM -0800, Jason Short via RT wrote:
> > 
> > The current client code tracks 11 different types of citizens: 2 each
> > for happy, content, unhappy, angry citizens, plus elvises, scientists,
> > and taxmen.  The order of these 11 is hard-coded in several places.  The
> > sprites for all 11 are loaded manually, and the accesses to them are
> > often also hard-coded.
> > 
> > This is inelegant and inextensible; see PR#2495.
> > 
> > The attached patch rewrites a lot of this code.  Now there are 7 types
> > of citizens: happy, content, unhappy, angry, elvis, scientist, taxman. 
> > Each type may have one or more sprites, as defined by the tileset. 
> > Accessing the sprite is done through the get_citizen_sprite function. 
> > This means a future change to the sprite system (such as PR#2495) can be
> > done without any GUI changes.
> > 
> > The patch itself is pretty lengthy and touches every GUI.
> > 
> > A lot of the happiness code could be removed with some common
> > functions.  This would be easier to do _after_ the enum citizen_type is
> > put into place, but could be done before to make the patches slightly
> > smaller.  I would prefer that this patch go in first.
> 
> Very nice.
> 
> First point: if we also allow different city styles for the citizen
> sprites (as suggested by Rafal) shouldn't we also add a player
> parameter now or will it be added later or do you plan to do this in
> another way?

Good point.  Can we ever see enemy citizens?  If so, a player parameter
is needed, which will be passed city_owner(pdialog->pcity) in most
cases.

> > +static char *get_citizen_name(enum citizen_type citizen)
> 
> Add a const. Add a note that these shouldn't be i18n.

OK.

> >    for (n = 0; n < pcity->ppl_happy[4]; n++, i++) {
> >      gtk_pixcomm_copyto(GTK_PIXCOMM(pdialog->citizen_pixmap),
> > -                  get_citizen_sprite(5 + i % 2), i * width, 0, TRUE);
> 
> > +                  get_citizen_sprite(CITIZEN_HAPPY, i),
> 
> The i should be n IMHO. More predictable.

I disagree.  Using n, if one content citizen becomes happy all other
content citizens will have their sprites toggled.  Using i all citizens
have their graphics remain the same (unless they change status, e.g.,
content->happy).

Note that MUI does use n here (I didn't bother to change this yet), and
that behavior is intended to be unchanged by the patch (except for the
MUI bug that's fixed).

jason