[Freeciv-Dev] Re: Improved game starting (version 8)

[Daniel L Speyer <dspeyer@xxxxxxxxxxx>]

On Sun, 27 Jan 2002, Raimar Falke wrote:

> On Sun, Jan 20, 2002 at 12:51:54AM -0500, Daniel L Speyer wrote:
> > Here is yet another version of the improved game starting patch.  The
> > server control buttons now send there commands over the network, and there
> > is now a checkbox to start the game immediately.  I have also moved the
> > extern statement from connectdlg.c to connectdlg.h.
> 
> gcc -DHAVE_CONFIG_H -I. -I. -I../.. -I. -I./.. -I./../include -I../../common 
> -I../../intl -I/usr/lib/glib/include -I/usr/X11R6/include 
> -I/usr/X11R6/include  -Wall -Wpointer-arith -Wcast-align -Wmissing-prototypes 
> -Wmissing-declarations -Werror  -g -O2 -Wall -c connectdlg.c
> cc1: warnings being treated as errors
> connectdlg.c:501: warning: no previous prototype for `remove_server_controls'
> connectdlg.c:513: warning: no previous prototype for `savegame_callback'
> connectdlg.c:523: warning: no previous prototype for `save_game'
> connectdlg.c:542: warning: no previous prototype for `add_server_ctrl_buttons'
> connectdlg.c:570: warning: no previous prototype for `chosen_name_callback'
> connectdlg.c:611: warning: no previous prototype for `get_player_name'
> connectdlg.c:743: warning: no previous prototype for `min_free_port'
> 

OK, I'll fix these.

> Try to build it for the xaw client:
> 
> gcc  -g -O2 -Wall  -o civclient  attribute.o citydlg_common.o cityrepdata.o 
> civclient.o climisc.o clinet.o control.o goto.o helpdata.o mapview_common.o 
> packhand.o options.o tilespec.o gui-xaw/libguiclient.a 
> ../common/libcivcommon.a ../intl/libintl.a  gui-xaw/libguiclient.a 
> ../common/libcivcommon.a  -L/usr/X11R6/lib -lXaw -lXpm -lXmu -lXt  -lSM -lICE 
> -lXext -lX11  -lz
> civclient.o: In function `set_client_state':
> client/civclient.c:551: undefined reference to `popup_main_menu'
> 

Hmm, I guess popup_main_menu needs to have the same name as the connect
dialog in xaw.  I'll work on that.

> Overall I think that the people will just like it very much.
> 
> Issues:
>  - if I load a file which isn't a savegame there is no feedback

Hmm, I dread the thought of parsing its pipe output -- maybe I can give
context-sensitive errors on server death, though.

>  - if I delete the "select player to be" dialog there is no other open
>  window

Yeah, I guess I should catch that and treat it as a cancel.

>  - if I select file "foobar" as savefile name it will create a "foobar.gz"

Should it be foobar.sav.gz?  Does gtk have a way to help with this?

>  - there is no error if the game couldn't be saved. Just remove the
>  write permissions *evil grin*.

The server only sends a warning through the pipe.  I just went through
removing all pipe usage from those buttons.  Maybe the server should
command a popup for failed saves?

>  - in the "new game" dialog: the ok button should have the same size
>  as the cancel button

Fine

>  - I would like the ability to dump the server communication. Best
>  would be a file. Enabled by command line option or a DUMP_SERVER_IO
>  pre-processor switch.

Difficult.  How about a gui command to display all server pipe output not
yet claimed?  Would that be good enough?

>  - please cleanup the code: indent (or add the spaces by hand), add
>  {}, seperate variable declaration from method body, use fc_malloc
>  instead of malloc,...)

Right.

>  - sizeof(char) is by definition 1

Is it?  That seems like a bad idea.  Eventually libc might go
Unicode.  More credibly, GTK could do something like qt, and we'll want to
mass-replace char with gchar -- then we'll want these sizeofs

>  - are you serious about _the thing_ in get_player_name? Use a struct
>  which has MAX_NUM_PLAYERS slots.

Yes, I am serious.

I'd forgotten that MAX_NUM_PLAYERS already existed, so I guess there isn't
quite as much need for dynamicism as I'd thought.  Even so, I can't
immediately see how anything else will work.  Remember, every callback
needs to get everything, so that it can de-allocate it.  However, each
callback needs to get something different, so that it knows what it
is.  This pretty much requires a circular structure of some sort.  I
suppose you could have something like:

Array <--------------\----\
 /-\                 |    |
 | |---->struct{name,|}   |
 +-+                      |
 | |--------->struct{name,|}
 +-+
  .
  .
  .
 +-+
 | |---->buf
 +-+
  .
  .
  .


and then pass the relevant struct, but I'm not sure how much nicer that
really is.  The struct name will have to clutter the namespace, if nothing
else.

>  - I shudder if I see (I'm broken down from the AI)
> +  gtk_widget_destroy(*( ((GtkWidget**)data) +5));
>    or 
> +  GtkWidget **pass=(GtkWidget**)malloc(7*sizeof(GtkWidget*));
> 
>    What is this 5 and what this 7?
> 

I'll make those more readable as part of cleanup

>  - what about fdopen so that we can use fputs or fprintf instead of
>  this ugly write?
> 

I've haven't used it to avoid cluttering the namespace, but it might be a
little nicer.  I'll see how much nicer it'd be.

--Daniel Speyer
"May the /src be with you, always"


>       Raimar
> 
> -- 
>  email: rf13@xxxxxxxxxxxxxxxxx
>  "Understanding is a three-edged sword; 
>   your side, their side, and the truth."
>     -- a well-known Vorlon
> 
> 
>