[Freeciv-Dev] Re: (PR#4426) Adding append command to gui-gtk-2.0

["jjc@xxxxxxxxxxxxxxxxxx" <jjc@xxxxxxxxxxxxxxxxxx>]

This is the second time I have requested clarification.
On 2003 Sep 2 vasc wrote:       
>I like the idea, but I will not apply any wide changes to the worklist
>code in the client until the common worklist code stops being braindead.
>
>i.e. it makes no sense current production is a separate struct field.
>The current production should simply be the first element in the
>worklist queue.

Question 1.
How is this relevent to the current patch?

The portions of this patch that modify the file common/worklists.[ch] add
two new functions: 
bool worklist_append(struct worklist *pwl, int id, bool is_unit);
bool worklist_insert(struct worklist *pwl, int id, bool is_unit, int idx);
Both these functions do not touch city.currently_building,
city.is_building_unit or city.shield_stock.
As such, they will continue to work unaltered if the city.currently_building
and city.is_building_unit are eliminated by vasc's proposed change.

The first and next commands will have to change how they work.  The first 
command will switch to become a worklist_insert(*,*,*,0) and the 
next will switch to become a worklist_insert(*,*,*,1), neither of which 
is a particually difficult change compared to other places that will
need to be changed for vasc's proposed change.

In otherwords, this change does not add much complication to changing 
the worklist/current production in the future.

Question 2.
Is the method for doing current production braindead?

The current method is to have a seperate worklist and current production
status.  The worklist is stored in city.worklist and the current production
is held in city.is_building_unit, city.currently_building and city.shield_stock
When a city finishes building a unit, the first item from the worklist
is popped off the worklist and put into currently building.  

It would seem to make sense to eliminate the redundant city.is_building_unit
and city.currently_building from the city struct and just add another slot
to the worklist and make the first item in the worklist the current item.

However, the a seperate current production does make a bit of sense,
since the current production is unique.  The current production is
usually partially done.  If you made the first item on the worklist
the current production, it would complicate the code to do something
like say swap the first and the second items, since the code would
have to remember to change the number of shields stored (if say you
switched from settlers to temple).  Since there are these special
cases when dealing with the current production, I am not certain that
making the first element in the current production will actually
significantly simplify code.  I am on the other hand certain that
making this change will be a fair amount of work.

In short, the worklist is the list of items waiting to be produced,
and the current production is currently being produced, and changing
the current production is different from changing a item on a queue,
so this is not necessarily braindead to have seperate variables.  I
believe that more justification needs to be made for such a
fundemental change.

Other discussion.

The only other relevent comment about this patch is from John Wheeler and was:
"I just made a clean sync with CVS, and trying to play without this patch was 
very painful.  At this point I consider it a 'must-have'."

Conclusion
I do not believe there have been any substantial objections to putting this
patch into cvs, so I would request this be considered.

-- 
Josh Cogliati