<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40184 >

> [EMAIL PROTECTED] - Wed Jun 11 16:51:03 2008]:
> 
>  I only read sources, not yet tested.
> 
> 
>  -  Please don't add new empty function headers. New code should have
> function headers already when committed.

What do you mean by "empty function header"? If you mean the

/*****************************
  ...
******************************/

Then I only fill that in if the function name is not descriptive
enough or there are some special conditions or side-effects that
programmers need to be aware of (e.g. "you must free the return
value yourself").

Do you think I should describe the functions more in the header?

>  - You add function client_player(), but don't always use it in new
> code instead of client.conn.playing

This I will fix. It is because I added the client_* functions
after already writing some code using client.conn.playing and
forgot to update the old code. :(
 
> 
>  What really happens to these files:
> 
> --- a/client/gui-gtk-2.0/Makefile.am
> +++ b/client/gui-gtk-2.0/Makefile.am
> @@ -42,8 +42,10 @@ libguiclient_a_SOURCES = \
>         dialogs.h       \
>         diplodlg.c      \
>         diplodlg.h      \
> -       editdlg.c       \
> -       editdlg.h       \
> +      editgui.c       \
> +      editgui.h       \
> +      editprop.c      \
> +      editprop.h      \
> 
>  Are editdlg.[ch] renamed, split or completely replaced?

They are completely replaced. Similarly for client/include
/editgui_g.h. The point was that the code in editgui.[ch]
is more that just an "edit dialog" so the name editdlg did
not really apply anymore (cf. function editgui_tileset_changed
and editgui_refresh which affect more than one widget).

I added editprop.[ch] because editgui.[ch] was getting very
long. It will contain the properties dialog (see the wikia
editor page for an overview) and its helper functions.

>  - There's some new direct S_FORTRESS and S_AIRBASE references. That
> goes against work for generic military bases. See common/base.[ch] to
> see if better implementation is already possible.

That I did not know about. I'll look into using that code.

Most of the time I had problems trying to get a "generic" sprite
for various objects (like roads or fortresses) so the added
code in client/tilespec.c might not be the best possible approach.

>  - Add FIXME about the fact that following code is not generic enough
> (gen-movement)
> +  coastal = is_sailing_unittype(punittype);
> +  homecity = find_closest_owned_city(pplayer, ptile, coastal, NULL);

This is a direct conversion of the old can_unit_exist_at_tile
to work on unit_type's (needed for example to check if a unit
type can be created at a certain tile).

I'll add the FIXME here, but generalizing this code really
belongs in another ticket.
 
>  - Why city creation now fails without telling reason to client?
> (modifications to handle_edit_city_create() /
> handle_edit_create_city() )

I thought a long time about how and when to do error messages
in server edithand.c. I decided the server should for the most
part remain silent for bad input or "illegal" operations. This
is because the user (who is editing) is assumed to know the
rules (e.g. where cities may be placed) and becase operations
can be batched (which would result in a lot of duplicate
messages).

Do you think the server should notify the user more in
such circumstances?

>  This is huge patch. Let's make only agreed fixes against this patch
> before commit (It's impossible for others to track what other changes
> appear if you do other development).

It's not hard for me to stash local changes and variously merge
between branches, but I will address now the issues you have raised
before extending the functionality further.


----------------------------------------------------------------------
何も欲しくない人がこの世界のどこかにいるか

_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to