[Freeciv-Dev] (PR#2520) New metaserver

["Vasco Alexandre da Silva Costa via RT" <rt@xxxxxxxxxxxxxx>]

[jdorje - Tue Dec 10 19:56:11 2002]:

> +  case 'R': return _("Running");
> +  case 'E': return _("Endgame");
> +  default:  return _("Undefined");
> +  }
> 
> s/Undefined/Unknown/ is better IMO.

Ok.

> +  for (i=0; i<nservers; i++) {
> +      char *host, *version, *state, *comment;
> +      int port, nplayers;
> +      struct server *pserver = (struct server*)fc_malloc(sizeof(struct
> server));
> 
> Trivial style issues: spacing and wrapping.  There are more elsewhere.

Ugh. I thought the limit was 80 chars. Now I notice it is 77. Why?
Oh well.

> +static bool is_url_safe(char ch)
> 
> This function is a misnomer.  It doesn't check a URL, but a single
> character (of a URL?).

Do you consider is_char_url_safe() satisfactory?

> +const char *my_url_encode(const char *txt)
> 
> What about fc_url_encode?  Isn't my_ only used for non-portable system
> functions?

Ok.

> +  default:
> +    state = 'U';
> +    break;
> +  }
> 
> Having a 'default' case prevents the compiler from detecting missing
> enum values.  Is it possible to avoid this?

I do not think that is worthwhile. That would imply breaking the
protocol compatibility every time an extra case is added.

> +  my_snprintf(s, n,
> +    "host=&port=%d&state=%c&",
> +    srvarg.port, state);
> 
> Question: does the style guide say anything about indentation in these
> latter cases?  The indent options it describes would indent it
> differently, but AFAIK nothing was decided about this.
> 
> 
> +           if (plr->ai.skill_level <= 3) {
> +                ai = 'E';
> +              } else if (plr->ai.skill_level <= 5) {
> +                ai = 'N';
> +              } else {
> +                ai = 'H';
> +              }
> 
> IMO it would be better to send the numerical difficulty of the AI, and
> have it interpreted into hard/normal/easy at the other end.  That way
> when a new difficulty "novice" or "impossible" is added it needs only
> small changes (and no worry of conflicting letters).

Ummm... Ok.

> +enum metainfo_type {
> +  META_INFO    = 0,
> +  META_VERSION = 1,
> +  META_COMMENT = 2,
> +  META_GOODBYE = 4
> +};
> 
> What's up with the numbers here?  Is this a flag-based enum or an
> enumerated list of values?  Perhaps a comment to explain it?

It is a list of bitfield flags. Except for META_INFO which is an empty
bitfield.

> It seems confusing that META_INFO is hard-coded as the default "no-op"
> metaserver action (so in some places we use META_INFO but in others we
> use META_VERSION | META_COMMENT, with no mention of META_INFO even
> though it's seemingly implied).

Do you prefer '0' to using META_INFO? I guess that could make more sense.

> As for the overall design of the communication: it seems nice.  But I
> suspect you knew that already.
> 
> Unfortunately, this patch will break every client except gui-gtk-2.0. 
> Does this mean support will be needed for all other GUIs, or is there a
> workaround to keep the interface backwards-compatible?

A gateway from the old metaserver to the new metaserver was considered
but discarded. Support will be added for the other GUIs.