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

["Jason Short via RT" <rt@xxxxxxxxxxxxxx>]

[vasc@xxxxxxxxxxxxxx - Sun Dec  8 23:43:45 2002]:

> 
> Hello,
> 
> Here is the first iteration of the patch that adds support for the new
> metaserver. You will find the web frontend for the metaserver here:
> 
> http://meta.freeciv.org/metaserver.phtml
> 
> The new metaserver was implemented by Paul Zastoupil.
> 
> 
> This patch currently only adds support to the GTK+ 2.0 client. I am
> sending this early on so I can get comments rolling sooner.

+  case 'R': return _("Running");
+  case 'E': return _("Endgame");
+  default:  return _("Undefined");
+  }

s/Undefined/Unknown/ is better IMO.

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


+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?).


+const char *my_url_encode(const char *txt)

What about fc_url_encode?  Isn't my_ only used for non-portable system
functions?


+  default:
+    state = 'U';
+    break;
+  }

Having a 'default' case prevents the compiler from detecting missing
enum values.  Is it possible to avoid this?


+  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).


+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 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).


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?

jason