[Freeciv-Dev] Re: [PATCH] Merge of Juha Litola's help dialog and my turns_per_research

[Jeff Mallatt <jjm@xxxxxxxxxxxx>]

At 2000/01/29 16:23 , s830@xxxxxxxxx wrote:
>Here is a patch that merges Litola's help dialog changes and my
>turns_per_research.
>
>For gtk only, this patch displays "Research every %d turns, %d turns left"
>in the science dialog. I also displays "Research every %d turns" in the
>tax-rate dialog.
>
>A small issue is that if you have the science report open and change
>sci-rate at the same time, the numbers don't get updated.
>I'll see about this later.

These are great features, but there are a number of problems.

First, it's easier to integrate patches if each patch does one thing.
Adding number of turns to research to both Tax Rates and Science Report is
one thing, changing the Help toggle to a Help button could be separate --
it doesn't have to be, it's a judgement call.  However, in the current case
the Help button change is much closer to being done than the number of
turns change.  Tying them together means that I can't commit the Help
button patch until the number of turns patch is ready.

As for problems in the Help button change, while _one_ toggle sufficed, we
need _two_ buttons -- one to open help onto the Goal and another to open
help onto the Researching tech.  Also, the popup_help_dialog_active
function is both declared and defined as:
    int popup_help_dialog_active()
but, Freeciv is a C program, and we want to use strict prototypes, so if it
takes no arguments it needs to be declared/defined as:
    int popup_help_dialog_active(void)

The "number of turns" display in the Science Report is basically okay (with
the small problem you mentioned).  Maybe the "Research every ## turns."
could go in the header, with just "###/### ## turns" in the Researching
box?  Also, whatever was added has made the dialog very wide!

The "number of turns" display in the Tax Rates dialog, however, really
doesn't work.  The basic problem is that you send a PACKET_PLAYER_RATES
packet off, but don't wait for the corresponding several PACKET_CITY_INFO's
and a PACKET_PLAYER_INFO to come back to inform the client of the new state
of everything.  The "number of turns" displayed is, after the initial one,
always the one for the *previous* set of rates.  You might be able to key
off of the PACKET_PLAYER_INFO, but there is a problem in trying to do that:
 In hande_player_rates(), send_player_info() is called before
global_city_refresh(), and the latter is what triggers the sending of the
PACKET_CITY_INFO's.  Swapping the two calls may fix the problem, but we
must be sure this doesn't cause any other problems.  Finally, this is a lot
of network traffic for a fairly minor feature.

However, the above isn't really the real problem.  I think it's very bad to
actually be changing the true tax rates when in the Tax Rates dialog.  For
example, I had a nice, happy (rapturing, actually) civilization, and
decided to open the Tax Rates dialog and just play "what if".  As I was
looking at what the fastest I could do research was, and hence I had zero
luxuries, the Turn Timeout timer ticked down to zero!  Suddenly, I had a
civilization that was very unhappy (all cities in disorder, in fact).  This
is not good.

So, we need something to fix all these problems.  Moving some of the rates
calculation code from server/ to common/, and parameterizing it may be the
easiest.  No protocol changes, no dangerous changes in ordering of existing
function calls, and no network overloads.  From server/cityturn.c, we'd
need parts of set_tax_income(), add_buildings_effect() and
unhappy_city_check() to be moved to common/city.[hc].  Then, the new
functions could be called from these old ones left in cityturn.c, and from
the new functions you've added to tech.[hc].  Note that this is all just a
suggestion -- I'm not even sure it'll work, and it's certainly not
fleshed-out enough to be considered a design. ;-)

jjm