[Freeciv-Dev] Re: Improved game starting version 6 [patch] [ready, IMO] (PR#1215)

[Daniel L Speyer <dspeyer@xxxxxxxxxxx>]

The seventh version of game starting is now available by ftp.  I've
fixed the intermittent crash-on-load bug (I think) and genrerally
cleaned things up.

Some responces to specific comments:

On Sat, 12 Jan 2002 jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:

> Daniel L Speyer wrote:
> 
> > The sixth version of the game starting patch is now available by
> > ftp.  This is mostly cleanup, because I've run out of features that need
> > including.  I've been playtesting it for a while, and it works nicely.  I
> > think it's ready for inclusion (please review it as such).
> 
> Two minor problems:
> 
> - When you launch a server "new game", the civclient at first fails to 
> connect.  Naturally, this is because the server is just starting up.  I 
> don't know if there's a good way around this.

I don't really think there is.

For the server to tell the client when it's ready through the network
would require the client to use server-like network features and ensure
general ugliness.  For it to do so over the pipe would mean ugly,
bug-prone and CPU-intesive busy-waiting.  For the client to just keep
trying makes more sense.
> 

> - After playing for a little bit on one game I managed to get the 
> control buttons (on the right side of the chat area) to go away.  This 
> one may take some time to track down.
> 

It can be induced by suspending civclient with ctrl-z and restoring it
with either fg or bg.  This seems like a gtk bug, but maybe I'm doing
something wrong.  It strikes me as an unusual thing to do in a graphical
game, so it's not that serious, and it'll probably take a real gtk wizard
to track down.

> - When you choose an empty savegame name, nothing seems to happen.  This 
> is a bit confusing.
> 

This is fixed -- any failure on loading a game brings back the main menu.

> The interface looks very good; I think its time to seriously consider 
> applying this.  But you need to fix the diff problem, of course.  (Why 
> does nobody use "cvs diff" around here?)
> 

How and why does one use "cvs diff"?

I've cleaned up the patch by hand more thoroughly this time, so hopefully
patching problems have gone away.

> 
> Now, as to the code: it would be good to have a gui programmer look at 
> this, but here's my review.
> 
> - A lot of the spacing doesn't follow the style guide.  The #includes 
> also aren't grouped correctly in some cases.  Blah, blah, blah...
> 

I haven't really followed the style thread (I've always found that
formatting of that type has almost no effect on readability), but my
impression was that the guide wasn't completed.  If it is, I suppose I'll
go through and bring this into compliance.

> - game_difficulty and game_aifill in clinet.c shouldn't be made extern 
> within connectdlg.c; this is way ugly IMO.  Either put them into 
> clinet.h or remove them (somehow) from connectdlg.c.
> 

I'll admit that I'd like a nicer way to pass data from afar to
try_to_connect, but this isn't all that bad.  The placement makes sense if
you consider that eventually other clients (hopefully) will use them as
well.

> - The sprintf in clinet.c should probably be an snprintf (or 
> mysnprintf).  This may be a matter of style, but it's much easier for 
> someone looking at the code to determine it is correct if you use the 
> snprintf form.  Otherwise I have to count the characters to make sure 
> there's no buffer overflow.
> 

Changed.

> - The code to send /easy, /normal, or /hard in clinet.c could be written 
> more elegantly.  Writing it as a switch statement would be better, but 
> better still IMO would be something of the form
> 
>    *difficulty = (game_difficulty >= 7) ? "/hard" :
>                   (game_difficulty >= 5) ? "/normal" :
>                    "/easy";
>    mystrlcpy(...);
>    send_packet_generic_message(...);
> 

Changed.  (well, I used ifs instead of ?:s for readability, but otherwise
as you suggested).

> - You have a couple of other patches mixed in here.  For instance the 
> find_city_near_tile fix is included.
> 

Oops, I'd forgotten that.  I've got those out now.

> - I notice you still send data over the pipes to civserver.  I still 
> think this is bad.  For one thing, there is no feedback on your actions; 
> for instance saving a game just silently succeeds/fails.
> 
> If the data went entirely over the chatline instead, would it still be 
> necessary to have pipes for the civserver's stdin, stdout, and stderr? 
> It should just be a matter of making the server happy, right?  How do 
> (daemon) servers run on civserver.freeciv.org handle this?
> 
> The only thing that looks like it might need to use the pipe is the 
> "cmdlevel hack first" line.  This could be controlled from the command 
> line using an rc file (though this would be kindof ugly too).
> 

Since the server controls are based on being on owning the server, I don't
really see the need to use the network (except where it's
easier).  If/when we get real server controls in the client, the start and
score buttons will become redundant, but there will still be a need for
extra controls for the server owner.

> - is_server_running() should IMO be declared with an explicit void 
> parameter, e.g.
>    int is_server_running(void)
> otherwise compilers may think the parameter list just isn't included. 
> This also applies to other functions.
> 

Done.

> -  What's up with this?
> +  /*  write(2,"|",1);*/
> 
> +  /*  write(2,"%",1);*/
> 
> +    /*    write(2,"&",1);*/
> 

Debugging flags I forgot to take out completely.  They're gone now.

> - In add_server_ctrl_buttons() the malloc is unnecessary.  If you do use 
> malloc it should be fc_malloc.  Again there's the sprintf thing.
> 

Changed.

> ditto in get_player_name.
> 

It's needed there.  Pieces of buf get passed indirectly but without
copying into the callback functions.  It won't do for that to get
de-allocated.

> - In start_server(), the three if() statements at the top can be unified.
> 

Done

> - In start_server, shouldn't we write an error message if the exec 
> fails?  Or, alternately, how does civclient find out about the failed exec?
> 

Done

> - In start_server, your list of execs to find the civserver seems 
> adequate, but a better solution may be possible.  But in execvp, the 
> first parameter of the argument list is supposed to be the executable - 
> which would require changing argv[0] for each execvp.  Ugh.
> 

They're pretty thorough, and now that failure gives an error message,
they're probably adequate.  If you've got a better idea, I'd be happy to
change them, though.

> - In start_server, the if (argv[3]) check is quite a hack.  We may wish 
> to add other parameters later, etc.  Why not make this information (if 
> we're loading a game) a parameter to the function?
> 

Changed

> - I see now that if we load a savefile, the cmdlevel hack stuff isn't 
> set up.  This could make things more complicated if we try to use the 
> chatline for every server command (a player could connect to a game and 
> not have enough permissions).  No doubt there's a way around this...
> 
> - This is a nit and a style issue to boot, but I don't really like the 
> loop used in min_free_port.  What about
> 
> for (port=5555; ; port++) {
>    ...
>    if (/* port is valid */)
>      break;
> }
> 

Personally, I find for statements with one empty part to be ugly.  I don't
really see an advantage to switching it.

> - In start_server_for_new_game, rather than use malloc (which will be 
> called each time a new game is started from the client, i.e. memory 
> leak), why not just declare the buffer static, i.e.
> 
> void start_server_for_new_game()
> {
>    static char port_arg[6];
>    /* ... */
>    snprintf(port_arg, sizeof(port_arg), "%d", server_port);
>    argv[2] = port_arg;
>    /* ... */
> }
> 
> Same in start_server_load_game().  Or perhaps these can be unified into 
> one function - a NULL filename meaning no loaded game?  That would make 
> sense IMO.
> 

Done

> - In filesel_callback2, the free(data) needs a comment of some sort 
> (explaining where data is allocated and/or why we're freeing it here). 
> Ditto for new_game_ok_callback and new_game_cancel_callback.  In 
> filesel_callback there's an unmatched malloc (probably linking to one of 
> these, eh?) that could use a comment as well.  Any time the malloc and 
> free are separated for local variables some sort of explanation is in 
> order IMO.
> 

I've documented this a bit.  I think it's clear now.

> - In filesel_callback, the 255-length buffer is unsafe.  Not all os's 
> will obey this limit.  What about using strdup (or mystrdup or whatever) 
> instead?
> 

Done

> - Typo: "What do you wish to to?" in popup_main_menu().
> 

Fixed

> - "Quit client only" isn't immediately obvious.  A code comment might be 
> helpful; eventually we'll need user documentation.
> 
> - What will happen in a GUI that doesn't implement the functions you add 
> to connectdlg_g?  It doesn't look like it would be good.  I'll try out 
> xaw and see.
> 
> 
> Hmmm, that's all.  It looks to me like the functionality is taken care 
> of, and all that's needed is refinement/cleanup.
> 
> Great patch.
> 
> jason
> 
> 
> 
>