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

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Daniel L Speyer wrote:

> 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.
Cool.


> > 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"?
From within a directory maintained by cvs, just run "cvs diff".  If you 
need help setting up CVS, see the "how to contribute" (IIRC) link off of 
www.freeciv.org.
Why: because it's very very easy.  One line will make all the patch you 
need; you don't need to worry about tracking a separate directory or 
copy of the file at all.
The only disadvantage is it uses the network and so may be slow for some.


> > - 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.
k&r style has been the style; recent threads have only refined this. 
The style guide was (I think) placed into CVS today.
Some people (Raimar?) have a script that they run over patches (?) to 
indent them.  This would probably be of use to (just about) everyone. 
It should be made public or even put into CVS.

> > - 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.
What I don't like is that they're declared extern in the connectdlg.c, 
not that they're placed in clinet.c.  Put extern definitions into 
clinet.h and #include that from gui-gtk/connectdlg.c.
Better yet (IMO), make a struct for them in clinet.h:

struct autoserver_data {
  int difficulty;
  int aifill;
  /* others added in future */
}

extern struct autoserver_settings autoserver;


> > - 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.
Well, IMO it's better since (1) it allows the user to see the feedback 
on the chatline and (2) it can be used on any server, not just an 
auto-started one.
The server controls are entirely separate from autostarting a game. 
What if you removed them from your patch and have them be covered by a 
separate patch?
Your patch would make sure commandlevel ctrl was alway set on an 
auto-started server.
The server controls patch would give more detailed thought to the UI for 
the controls.  It was suggested that they go into a menu; this seems 
very reasonable (especially as the number of controls grows).  However 
the "game" menu won't be so good once the number of controls grows; 
perhaps a new "server" menu would be better.  In any event this menu (or 
items in it) should be activated and de-activated based on our 
commandlevel on the server we're connected to (and possibly other 
checks), although this is a lower priority.

These patches should be completely independent of each other.


> > - 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.
Fine by me.


jason