[Freeciv-Dev] Re: At endyear map in not displayed (PR#1883)

[Davide Pagnin <nightmare@xxxxxxxxxx>]

On Tue, 2002-08-27 at 18:59, Raimar Falke wrote:
> On Mon, Aug 05, 2002 at 09:08:35AM -0700, nightmare@xxxxxxxxxx wrote:
> > As well known, 1.13.0 release and further development code (1.13.1-devel)
> > doesn't display the full world map at the end of a game.
> > 
> > This misbehaviour as been introduced on 3rd July, when the 
> > CLIENT_GAME_OVER_STATE has been introduced in the code.
> 
> This and the no-chatline-during-nation-selection bug is fixed by the
> attached second version of the POPS/POPF patch. The problem is the
> same in the two cases: we used some more or less arbitrary packets to
> group packets for the agents (which was latter adopted by the reports
> (the chatline is a report in this context)). In two cases a packet
> which can be used as a border-packet wasn't available. So we added
> PACKET_START_TURN and CLIENT_GAME_OVER_STATE to recognize the end of
> the pregame (started by PACKET_JOIN_GAME_REPLY) and the end of a game
> in general (started by PACKET_BEFORE_NEW_YEAR).
> 
> As pointed out in the first version of the POPS patch I think that
> adding these two packets were a mistake. The patch fixes this. We
> could remove these two packets. But as usual other things use the
> packets. At least the PACKET_START_TURN.

Wouldn't be nice, if we take this occasion to write the purpose and side
effects of every type of packet we use?

> 
> A second patch (an extended version of Davide's) should be applied on
> top of this which will let the client take full advantage of the
> information of CLIENT_GAME_OVER_STATE. Till then the state is ignored.
> 
> I know that the patch is only a band-aid for the problem is was
> created for. To make it clear that I don't intend to use the patch for
> this I removed this part:
> 
> @@ -565,15 +565,18 @@
>                   err = gettimeofday(&start, &tz);
>                   assert(!err);
>  #endif
> -               connection_do_buffer(pconn);
> +               conn_list_do_buffer(&game.all_connections);
>                 start_processing_request(pconn,
>                                          pconn->server.
>                                          last_request_id_seen);
> +               prepare_for_sending_POPS(pconn);
> +
>                 command_ok = handle_packet_input(pconn, packet, type);
>                 packet = NULL;
> 
> +               send_POPF();
>                 finish_processing_request(pconn);
> -               connection_do_unbuffer(pconn);
> +               conn_list_do_unbuffer(&game.all_connections);
>                 if (!command_ok) {
>                   close_connection(pconn);
>                 }
> 
> So the name of the packets should be changed from
> PACKET_OTHER_PROCESSING_STARTED and PACKET_OTHER_PROCESSING_FINISHED
> to PACKET_FREEZE_HINT and PACKET_THAW_HINT. The emphasis is on
> HINT. These packets mark a set of other packets which are send out
> within a small timeframe and may contain inconsistent data.

I don't want to annoy you, but I found the proposed name more
insightful, and thus I vote for the name change.

> diff -Nurd -X clean/diff_ignore -x *.sav* clean/client/agents/agents.c 
> work/client/agents/agents.c
> --- clean/client/agents/agents.c      Sat Aug 24 15:38:51 2002
> +++ work/client/agents/agents.c       Tue Aug 27 16:38:06 2002
> @@ -352,10 +352,27 @@
>  /***********************************************************************
>   Called from client/packhand.c.
>  ***********************************************************************/
> +void agents_other_processing_started(void)
> +{
> +  freelog(META_CALLBACKS_LOGLEVEL, "agents_other_processing_started()");
> +  freeze();
> +}

Just a question, all the cma stuff, is polluted with this define
(META_CALLBACKS_LOGLEVEL) that I found very interesting for debug
purposes, is there any chance that a similar define can found his way in
the cvs and a freelog call can be put at beginning of any function call?

A second thing, all those 'agent_*' function are mostly wrappers for a
freelog call and otherwise void or contain a single function call
(freeze or thaw), perhaps they contribute to auto-comment code, but
aren't them overkill? 

> diff -Nurd -X clean/diff_ignore -x *.sav* clean/client/agents/cma_core.c 
> work/client/agents/cma_core.c
> --- clean/client/agents/cma_core.c    Wed Aug  7 14:38:11 2002
> +++ work/client/agents/cma_core.c     Sun Aug 25 19:59:56 2002
> @@ -1719,6 +1719,8 @@
>    bool handled;
>    int i;
>  
> +  assert(find_city_by_id(pcity->id) == pcity);
> +

What this assert is for? (I don't get how this can cope with proposed
goal of this patch).

> diff -Nurd -X clean/diff_ignore -x *.sav* clean/po/Makefile.in.in 
> work/po/Makefile.in.in
> --- clean/po/Makefile.in.in   Tue Aug 27 17:27:31 2002
> +++ work/po/Makefile.in.in    Tue Aug 27 18:31:17 2002
> @@ -65,7 +65,7 @@
>       $(MSGMERGE) $< $(srcdir)/$(PACKAGE).pot -o $*.pox
>  
>  .po.mo:
> -     $(MSGFMT) -c -o $@ $<
> +     $(MSGFMT) -o $@ $<

hmm... I have my own idea for this... but definitely I don't think that
such code "$(MSGFMT) -c -o $@ $<" is in the CVS, or perhaps I'm wrong??

Otherwise it would be nice to have the -c parameter in CVS! :-)

        Ciao, Davide

P.S. You introduced 2 new packets, I suspect that a capability string is
needed for this to cope with old server/client.