[Freeciv-Dev] Re: (PR#8491) Re: Freeciv commit: kauf: A new connect dialog for the client. This allo...

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8491 >

On Mon, Apr 12, 2004 at 06:41:13AM -0700, Mike Kaufman wrote:
> > > +enum skill_levels { 
> > > +  NOVICE, 
> > > +  EASY, 
> > > +  NORMAL, 
> > > +  HARD, 
> > > +  EXPERIMENTAL,
> > > +  NUM_SKILL_LEVELS
> > > +};
> > > +
> > > +extern const char *skill_level_names[NUM_SKILL_LEVELS];
> > 
> > Shouldn't/can't these be shared with the server.
>  
> possibly. devise a method and tell me.

Move the enum and the name array to common. Change stdinhand.c to use
them.

> > > +void really_close_connection_dialog(void);
> > 
> > The naming is very poor. The documentation don't make it any clearer
> > what this function should do.
> 
> umm. did you read the function header?

Sure. gui-stub/connectdlg.c reads:

/**************************************************************************
 really close and destroy the dialog.
**************************************************************************/
void really_close_connection_dialog(void)
{
  /* PORTME */
}

/**************************************************************************
 close and destroy the dialog.
**************************************************************************/
void close_connection_dialog()
{
  /* PORTME */
}

Very informative.

> Maybe we should add a "no matter what." ?

And what could force close_connection_dialog to not work?

> > > +/******************************************************************
> > > + Fill the settable_options array with an option.
> > > + If we've filled the last option, popup the dialog.
> > > +*******************************************************************/
> > > +void handle_options_settable(struct packet_options_settable *packet)
> > > +{
> > > +  int i = packet->id;
> > 
> > > +  assert(i >= 0);
> > 
> > Also check against the upper bound.
> 
> what upper bound? 65535?

nids.

> > > +  switch (packet->type) {
> > > +  case 0:
> > 
> > Use enums.
> 
> we had this discussion already. the answer is no.

Your answer was:

  as for the complaint about enums, this won't work because the common
  code doesn't have access to the enums in stdinhand.c and it's not
  going to just for this purpose.

I still disagree. Others please.

> > > Index: common/dataio.h
> > > ===================================================================
> > > RCS file: /home/freeciv/CVS/freeciv/common/dataio.h,v
> > > retrieving revision 1.4
> > > retrieving revision 1.5
> > > diff -u -r1.4 -r1.5
> > > --- common/dataio.h       28 Nov 2003 17:37:21 -0000      1.4
> > > +++ common/dataio.h       10 Apr 2004 03:47:49 -0000      1.5
> > > @@ -59,7 +59,8 @@
> > >  
> > >  void dio_get_sint8(struct data_in *din, int *dest);
> > >  void dio_get_sint16(struct data_in *din, int *dest);
> > > -void dio_get_sint32(struct data_in *din, int *dest);
> > > +#define dio_get_sint32(d,v) dio_get_uint32(d,v)
> > 
> > Please explain. Please explain good. Really good.
> 
> it's 'explain well'... 
> it's simple really. If you'd actually written a function instead of just
> the function prototype, I'd have used that.

Ok that backfired. Still not a reason to write the function.

> > > +PACKET_SINGLE_WANT_HACK_REQ=108;cs,handle-per-conn,no-handle,dsend
> > 
> > > + UINT32 challenge;
> > 
> > Since an UINT32 don't provide _this_ much entropy we should have used
> > a more general approach: a string. This way if we add the entropy we
> > don't have to change the protocol.
> 
> coulda. still can if you want to change the protocol now.

Hmmm.

> > > +PACKET_SINGLE_PLAYERLIST_REPLY=111;sc
> > > +  UINT8 nplayers;
> > > +  STRING load_filename[MAX_LEN_PACKET];
> > > +  STRING name[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 
> > > +  STRING username[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 
> > 
> > > +  STRING nation_name[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 
> > 
> > > +  STRING nation_flag[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 
> > 
> > Why can't the nation_flag not be extracet from the nation name?
> 
> no. the rulesets haven't been transferred over yet. this really is a 'bug'
> that needs to be fixed.

Ahh. I agree. Another client may want extra information about the
nation.

> > > +PACKET_OPTIONS_SETTABLE_CONTROL=112;sc,handle-via-packet
> > 
> > > +  UINT16 nids;
> > 
> > Shouldn't this be named "noptions"?
> 
> maybe. you wanted it to be nids though IIRC. at least you disliked nopts.

You used "nopts" in PACKET_OPTIONS_SETTABLE. I suggested "id" and you
used this. In PACKET_OPTIONS_SETTABLE_CONTROL however it an amount and
should be named "noptions".

> > IMHO this should look like:
> > 
> >   SINT32 int_value;
> >   SINT32 int_value_default;
> >   SINT32 int_value_min;
> >   SINT32 int_value_max;
> > 
> >   BOOL bool_value;
> >   BOOL bool_value_default;
> > 
> >   STRING string_value[MAX_LEN_PACKET];
> >   STRING string_value_default[MAX_LEN_PACKET];
> 
> I know you do. no.

Others?

> > > +#define MAX_LEN_PATH 4095
> > 
> > See C99 and FILENAME_MAX.
> 
> I'm too lazy.

Hardly an excuse.

> > > +++ server/srv_main.h     10 Apr 2004 03:47:50 -0000      1.18
> > > @@ -34,7 +34,7 @@
> > >    /* filenames */
> > >    char *log_filename;
> > >    char *gamelog_filename;
> > 
> > > -  char *load_filename;
> > > +  char load_filename[512]; /* FIXME: may not be long enough? use 
> > > MAX_PATH? */
> > 
> > Use MAX_LEN_PATH.
> 
> hence the FIXME. The only problem being that this is probably bigger than
> MAX_LEN_PACKET...

And?

> you want to write a patch to fix these issues, I'll certainly commit it.

It is your patch. I'm merely given a late review.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "The Internet is really just a series of bottlenecks 
  joined by high speed networks."
    -- Sam Wilson