[Freeciv-Dev] Re: (PR#977) Extended connect dialog

["Daniel L Speyer" <dspeyer@xxxxxxxxxxx>]

Mike Kaufman wrote:

>I've merged all the connect dialog tickets into the original. Please use
>PR#977 for all future correspondence on this.
>
>Now to the comments:
>
>connectdlg_common.c:
> o check coding style. (spaces, comments doing line wraps...)
> o "Owner" as name. use user_username() instead.
>  
>
I know there's a good reason I went to Owner, though I can't remember 
what it is at the moment.  If nothing else, it makes it clear who's 
running a multi-player game. but that's not the only reason.

>don't use /rename anymore (remove the rename code). Instead use /take
>  
>
Has this code stabilized enough to be worth syncing with?  Last I 
looked, there was a new way to do it every week and I just stuck with 
what worked.  Is there a problem with rename?

>there is a bug in the load savegame code. picking an AI player picks the
>wrong player.
>  
>
Can you reproduce it?

>on PAGE_START, each radio highlights on mouseover. This looks ugly. Think
>you could do something about it?
>  
>
That's a theme issue.  IMHO, it looks nice.  If you don't like it, tell 
GTK to stop.

>+++ freeciv-cvs-Aug-13/server/plrhand.c 2003-08-13 22:46:05.000000000 +0000
>@@ -17,6 +17,7 @@
>
> #include <assert.h>
> #include <stdarg.h>
>+#include <stdio.h>
>  
>
That's a leftover from debugging, I'll get rid of it.

> #include "diptreaty.h"
> #include "events.h"
>@@ -1100,7 +1101,7 @@
>   if (pplayer) {
>     dest = (struct conn_list*)&pplayer->connections;
>   } else {
>-    dest = &game.game_connections;
>+    dest = &game.est_connections;
>   }
>
>   va_start(args, format);
>@@ -1119,7 +1120,7 @@
>   if (pplayer) {
>     dest = (struct conn_list*)&pplayer->connections;
>   } else {
>-    dest = &game.game_connections;
>+    dest = &game.est_connections;
>   }
>
>Explain the reason you do this. In my opinion this is wrong.
>  
>
This is needed to make sure everyone gets everything while a game is 
being loaded.  Some important messages were being dropped, though I 
don't remember which.

>+#define CHALLENGE_ROOT ".freeciv-tmp"
>and sprintf(fn, ".civserverlog%d", server_port);
>
>These are not good names. On windows and especially on VMS.
>  
>
We run on VMS?!

On windows, they just look a little unixy, but no harm done.  If we want 
to hide them on windows, we'll have to do that specially (not me though, 
I don't have windows, and I stay away from code I can't test).

>+++ freeciv-cvs-Aug-13/common/mem.c     2003-08-13 22:45:50.000000000 +0000
>@@ -115,7 +115,11 @@
> char *real_mystrdup(const char *str,
>                    const char *called_as, int line, const char *file)
> {
>-  char *dest = (char *)fc_real_malloc(strlen(str)+1, called_as, line,
>   file);
>+  char *dest;
>+
>+  if (!str) return NULL;
>+
>+  dest = (char*)fc_real_malloc(strlen(str)+1, called_as, line, file);
>   /* no need to check whether dest is non-NULL! */
>   strcpy(dest, str);
>   return dest;
>
>This can be omitted from the patch. This is my old cruft.
>  
>
Will do.

>+  strcpy(pack.message, "/ score\n");
>+  strcpy(pack.message, "/ quit\n");
>
>remove the writespace.
>  
>
Will do

>userNB as a variable name... change it.
>  
>
You mean uberNB?  Do you think really_big_notebook is better?

>report.c:1152: warning: no previous prototype for `page_conn_flagged'
>stdinhand.c: In function `report_server_options':
>stdinhand.c:2668: warning: implicit declaration of function
>`page_conn_flagged'
>
>I got this, do you?
>  
>
Never noticed it, but it does need a prototype, so I guess I missed 
that.  Will fix.

>connectdlg_common.c:59:bool hoping_for_human_opponents = FALSE;
>
>what's this? cruft?
>  
>
Yeah, I think that should come out.

>
>
>metaserver: I'm very opposed to this. As I've said before, if you want to
>run a serious server (by which I mean advertising on metaserver, etc), then 
>you should NOT be running the server from the client. This sort of thing
>belongs in a GUI frontend to the server proper. I'm not quite sure that if
>one doesn't know enough to run a server proper, he ought to be advertising
>on metaserver... Please remove it.
>  
>
Just because someone prefers a GUI doesn't make them an ignoramous, and 
even if they are, they should still be able to marginally endanger their 
own data by advertising a server!  This isn't a disk partitioning tool, 
it's a game -- it should be easy to play.

>rulesetdirs: I don't like this at all. In my opinion this entirely breaks
>the distinction between client and server. Remove it. It doesn't make sense
>anyway (rulesets could be anywhere). In a future patch, you may implement 
>this as a server only function.
>  
>
The function is called from the server.  If you think it would go better 
in some random server file, I suppose it could move, butt all the 
related functions are in common.  Also, rulesets can't be "anywhere", 
they can only be in searched directories.  This is a good reason to 
leave the code together -- so that if the search algorythm is changed, 
it will give the correct list.

>Save: should act just like /save: the server should pick the savegame name
>even if we used Save As... before. Save As... should have the previous savegame
>name though...
>  
>
"Save" and "Save as" are well established in the GUI world and anyone 
with any GUI experience knows what they mean.  Violating that would be 
like using "+" for multiplication.

>server options: I've been convinced that having a gui for this is ok, but this
>implementation makes the front page look way too busy. This should be in a
>separate dialog (combined with Game/Server Options). Put a button only on
>the newgame page.
>  
>
I'll agree it should be prettier, but hiding everything isn't the right 
way either.  I tried it.  I think it makes sense to get this patch done 
and then tweak the UI.

Also, I am convinced that it's better to err on the side of exposing to 
much.  Games don't intimidate people, but the vast majority of our 
players don't know about the vast majority of our features (check any 
freeciv.org poll).  Making things easy to play with is how people will 
learn.

>Quit: I was, to say the least, quite surprised to find multiple stale
>civservers running on by machine after testing. It seems that Quitting out
>of the client doesn't kill the civserver. It should. The only time that the
>server shouldn't be killed is by using "Disconnect from Game". Also remove
>the popup in favor of a append_output_window()
>  
>
Can you work out how this happened?  I thought all those bugs were fixed.

As for popup vs append_output_window, I'm undecided.  What do other 
people think?

>authentication solution: I'll admit that your solution was clever, however, 
>I'm uncomfortable with the idea of a separate [tiny] popup for each query.
>I don't know yet what to recommend. I'll think about it.
>  
>
Meanwhile, how do you use authentication?  I never tested that code 
becaus eI couldn't figure it out.

--Daniel

>-mike
>
>
>  
>