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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Mike Kaufman wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=977 >
> 
> Here's the connect dialog patch ported over to the new delta system.

Looking at the code...

I don't understand the design at all, but I haven't followed this 
discussion closely.  But I do see:

- Your translation code is wrong.

skill_level_names and skill_level_names_translated should be just one 
array, skill_level_names.  This array should use N_().  N_() doesn't 
actually translate the names (this is impossible in a static array), it 
just marks them for later translation.  So later you access either 
_(skill_level_names[i]) or skill_level_names[i] depending on whether you 
want it translated or not (respectively).

Also, /* TRANS */ comments are for translators.  They should go 
immediately before a translated string and are put into the translator's 
potfile by xgettext (the program that scans for translatable strings).

- Why do the packets you introduce require explicit (non-delta) 
handling?  Raimar?

- /* FIXME: may not be long enough? use MAX_PATH? */

You should use PATH_MAX or (better) dynamically assign arrays that store 
a path.  Note that PATH_MAX doesn't exist on all systems; if it doesn't 
exist then the path length is unbounded.  (I'm pretty sure it's 
PATH_MAX, defined in limits.h, not MAX_PATH.)

- This patch introduces a lot of design that needs documentation, 
probably in doc/HACKING.  Better code comments will help too: not just 
saying what a function does but how it fits into the overall design.

jason