Re: [Freeciv-Dev] cmdline patch (updated)

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

Reinier Post wrote:

> An update of the patch to allow client side server commands is attached.
> Also available here:
> 
>   http://cursus02.win.tue.nl:4321/tmp/freeciv/may29+cmdline.diff-Nurd.gz
> 
> Explanation (not included) in
> 
>   http://cursus02.win.tue.nl:4321/tmp/freeciv/README.cmdline

Um, this README looks a little bit out of date, eg:

> Before the game starts, command levels cannot be assigned to individual
> players; this is something to rectify in a later version.

This is now fixed in the current patch, right?

The main problem I had trying the patch was that reloading a save 
game (created either before or after the patch), all players are 
listed as "unprotected" when using "cmdlevel".  Its not clear 
(certainly not without looking at the source) what that means, 
and its also not clear what the server operator can/should do 
to change this, to be able to use cmdlevel on such players.
I'm guessing maybe you just need to call new_user inside one 
of the load functions.

I don't like the new "show" format.  The main changes seem to be:

- Added column to show actual default values.  This may be useful 
sometimes, but given that this output is already squeezed for
room horizontally, and the defaults are not _that_ useful (explain 
works to see individual defaults if one is interested), I think we 
can do without this.

- Mandatory leading space before small numerical values.  This just
wastes horizontal space.  Presumably the reason is to keep things 
lined up, but this doesn't work anyway, due to long names and 
potentially large values.

- Changed "#*" to "cd".  A matter of taste, but I actually prefer to 
old format.   And having to space "cd" loses one more space ;-)


Other comments from the patch:

+cmdlevel L all  - sets command access level to L for all players
+cmdlevel L new  - does the same for new/unprotected players only
+cmdlevel L P    - sets command access level to L for player P

What if there is a player named "all" or "new"? :-)

+Abbreviations are allowed, as long as they're unique.
+>

But this isn't strictly correct right?  Because eg 's', and 'h'
still work (which I like!) but with a warning.  But I'm not
sure quite how/where ambiguity is allowed/handled.

+  /save /etc/passwd

I agree with Nicolas this should not be an example.  Put in a
separate warning about this if you like, but have a sensible
example in the examples section.


+Freeciv is confused about the meaning of 'player'.  As a participant in
+Freeciv games, you'll notice that the participants are called 'players'.
+At the same time, players seem to be identified with civilizations.
+On the other hand, civilizations seem to be identified by 'race':
+every player chooses a race at the start of the game.

Also, 'player' can be associated with 'Ruler', which one may
want to consider as someone distinct from the a human player.
(The 'Ruler' being an entity inside the game, whose role may be
player by a human, or AI, or potentially multiple humans (at
different times, or hopefully in the future with multiple 
connections per player).)

+There are three player-related entities:

++ user
+  identifies 'someone out there', usually a human user running a civclient

For completeness, another player-related entity (at least in the 
server) is a 'connection', which in some ways corresponds to your
description of a 'user', since it represents 'someone out there'.  
I guess the main difference between a 'connection' and a 'user' is 
that one may want to consider a user to persist between different 
connections or even different games.

Another difference between players (civilizations) and users is
that ideally one could have multiple users per civilization.

+Races aren't actually used for annything that matters; for them,
+so the question isn't very interesting.

Right, races mainly have "superficially" differences.  (Flags, 
default ruler name, city names.)  The main important thing they 
_could_ be used for (but are not much at the moment (at all?)) 
is determining AI characteristics.  Potentially in scenarios/modpacks 
one may want them to have more real differences (not implemented).

+Users are created when they first connect to a server, rememberered by
+the running server and in savefiles.  When a user first connects, the

Just a note: written as above, its not clear that this is just 
the plan at the moment, and has not yet been implemented.  Better
language would be to say, eg:  "Users should be created ..." etc


+  "Server Command Output    ",          /* E_SERVER_COMMAND_FEEDBACK */

Actually, it took me a while to think if this, but I don't think 
it ever really makes sense to change the Message Options for the 
server command feedback: you certainly don't want popups, and the 
Messages window is really line-based, and mainly to support
goto-location and popup-city etc.  So it would make more sense
to me to just use E_NOEVENT for the server command feedback,
and skip this new even type.


+       char *line_by_line = mystrdup(op->extra_help);
+       if (!line_by_line) {
+          freelog(LOG_DEBUG,"warning: an unimportant mystrdup() failed");

mystrdup() never returns NULL, since it uses fc_malloc, whose
main point is to automatically do the check for NULL for you.


       for (i=0;i<game.nplayers;i++) {
-       if (game.players[i].conn || game.players[i].ai.control) plrs++ ;
+        if (game.players[i].conn || game.players[i].ai.control) plrs++ ;
+      }
+
+      if (plrs<game.min_players) {
+        cmd_reply(cmd,caller, C_FAIL,
+          "Not enough human players, game will not start.");

Maybe this is how it should be (I'm not sure), but this 
changed message is factually incorrect, because of the 
"|| ai.control" part when counting plrs.

+  default:
+    freelog(LOG_NORMAL,"bug in civserver: impossible command recognized; 
bye!");
+    assert(0);

Should be LOG_FATAL, surely.

Regards,
-- David