[Freeciv-Dev] Re: GUI PATCH: refresh_tile_mapcanvas cleanup

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"Ross W. Wetmore" wrote:
> 
> You may have introduced a few bugs, see explanation below :-).

I think if anything I fixed an existing buglet (see explanation below).

> At 12:00 PM 01/10/10 -0400, Jason Dorje Short wrote:
> >The function refresh_tile_mapcanvas() is absolutely identical in every
> >GUI.
> [...]
> >In summary, all of the GUI authors will need to sign off on such a patch
> >for it to go through.
> >
> >jason? rc
> >Index: client/climisc.c
> >===================================================================
> >RCS file: /home/freeciv/CVS/freeciv/client/climisc.c,v
> >retrieving revision 1.62
> >diff -u -r1.62 climisc.c
> >--- client/climisc.c   2001/10/04 19:36:54     1.62
> >+++ client/climisc.c   2001/10/10 15:46:22
> >@@ -828,3 +828,19 @@
> >   }
> >   return cids_used;
> > }
> >+
> >+/**************************************************************************
> >+ GUI Independent (with new access functions).  However, this should go
> >+ in client/mapview.c instead of client/climisc.c.
> >+**************************************************************************/
> >+void refresh_tile_mapcanvas(int x, int y, int write_to_screen)
> >+{
> >+  assert(is_real_tile(x, y));
> >+  if (!normalize_map_pos(&x, &y))
> >+    return;
> >+
> >+  if (tile_visible_mapcanvas(x, y)) {
> >+    update_map_canvas(x,y, 1, 1, write_to_screen);
> >+  }
> >+  overview_update_tile(x, y);
> >+}
> 
> I don't think this is a correct interpretation of the original code.
> 
> If you remove the assert(is_real_tile() and if condition around
> normalize_map_pos() then I am much happier with it.
> 
> But I don't understand the GUI, so my belief that the code checking
> in tile_visible_mapcanvas() and overview_update_tile() that expects
> to draw unreal tiles as black tiles in the GUI window may be in
> error.
> 
> But I would get someone to confirm this before you really change the
> existing code flow by stopping the GUI from ever drawing unreal tiles
> where it expects (and maybe needs) to.

Look again.  My proposed change is not identical to current code, but
I'm pretty positive it's the intended behavior.  The current code will
never draw an unreal or non-normal tile; if passed an unreal position it
will convert it to a real, normal tile (using the equivalent of
nearest_real_pos).  This is surely not intended; the only reason
map_adjust_[xy] is called is to wrap a non-normal real map position.  So
the current code, if it is bogusly called with an unreal position, will
simply redraw some random nearby real tile.

> [...]
> >Index: client/gui-gtk/mapview.c
> >===================================================================
> >RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v
> >retrieving revision 1.103
> >diff -u -r1.103 mapview.c
> >--- client/gui-gtk/mapview.c   2001/10/05 09:47:41     1.103
> >+++ client/gui-gtk/mapview.c   2001/10/10 15:46:23
> >@@ -640,21 +640,6 @@
> > /**************************************************************************
> > ...
> > **************************************************************************/
> >-void refresh_tile_mapcanvas(int x, int y, int write_to_screen)
> >-{
> >-  x = map_adjust_x(x);
> >-  y = map_adjust_y(y);
> >-
> >-  if (tile_visible_mapcanvas(x, y)) {
> >-    update_map_canvas(x, y, 1, 1, write_to_screen);
> >-  }
> >-  overview_update_tile(x, y);
> >-}

jason