[Freeciv-Dev] Re: (PR#4725) Rename map_get_known and map_get_known_and_seen to map_is_*

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

On Wed, 6 Aug 2003, Andy Smith wrote:

> On Tue, Aug 05, 2003 at 04:09:18AM -0700, Andy Smith wrote:
> > On Tue, Aug 05, 2003 at 03:35:52AM -0700, Gregory Berkolaiko wrote:
> > > 
> > > On Mon, 4 Aug 2003, Jason Short wrote:
> > > > Good points.
> > > > 
> > > > Yet the current naming system is still broken.  We have map_is_known and
> > > > map_get_known2.  "known" means two different things in these function
> > > > names.  We need to decide on a consistent meaning and come up with a new
> > > > term to describe the other.  Perhaps map_is_known and 
> > > > map_get_known_type?
> > > 
> > > Well, map_is_known and map_get_known would be better than before.
> > > Is map_is_known and map_get_known_type even better?  I am not sure, but I 
> > > would be willing to commit any of the above naming schemes.
> > 
> > OK, I will provide a patch for map_get_known2() -> map_get_known_type().
> 
> And this is attached.

Some big grudges about this patch:

1. It was agreed that one shouldn't do style changes outside the immediate 
area of a patch.

2. When one does such changes they should fully conform to the coding 
style, e.g.
-  if(game.player_idx==punit->owner && punit->activity!=ACTIVITY_GOTO && 
-     auto_center_on_unit && punit->activity!=ACTIVITY_SENTRY &&
-     !tile_visible_and_not_on_border_mapcanvas(pinfo->x, pinfo->y))
+  if (game.player_idx==punit->owner && punit->activity!=ACTIVITY_GOTO && 
+      auto_center_on_unit && punit->activity!=ACTIVITY_SENTRY &&
+      !tile_visible_and_not_on_border_mapcanvas(pinfo->x, pinfo->y)) {

should in foact be changed to 

  if (game.player_idx == punit->owner && punit->activity != ACTIVITY_GOTO 
      && auto_center_on_unit && punit->activity != ACTIVITY_SENTRY 
      && !tile_visible_and_not_on_border_mapcanvas(pinfo->x, pinfo->y)) {

or even better (but is a question of taste)

  if (game.player_idx == punit->owner && auto_center_on_unit
      && punit->activity != ACTIVITY_GOTO   
      && punit->activity != ACTIVITY_SENTRY
      && !tile_visible_and_not_on_border_mapcanvas(pinfo->x, pinfo->y)) {

3. One should read his own patches (and compile the too).  So that the 
obvious cruft like

--- freeciv-cvs/client/gui-gtk/citydlg.c        2003-08-06 17:19:16.000000000 
+0100
+++ freeciv-misnomer/client/gui-gtk/citydlg.c   2003-08-06 17:10:35.000000000 
+0100
@@ -1771,10 +1771,16 @@
 
   /* We have to draw the tiles in a particular order, so its best
      to avoid using any iterator macro. */
+<<<<<<< citydlg.c
+  for (city_x = 0; city_x < CITY_MAP_SIZE; city_x++) {
+    for (city_y = 0; city_y < CITY_MAP_SIZE; city_y++) {
+      int map_x, map_y;
+=======
   for (city_x = 0; city_x<CITY_MAP_SIZE; city_x++)
     for (city_y = 0; city_y<CITY_MAP_SIZE; city_y++) {
       int map_x, map_y, canvas_x, canvas_y;
 
+>>>>>>> 1.172

doesn't get in.

4. 87K is too large for the list, needs to be compressed.  On the other 
hand, compressed patches don't get read.  The conclusion: when making a 
patch, concentrate on one thing.

Please don't be discouraged by this criticism and keep contributing,
G.