[Freeciv-Dev] Re: (PR#3727) Rectangular selection with right-click-and-drag

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

a-l@xxxxxxx wrote:
> Parent: (PR#3529) Map Control patch
> 
> On Sun, 2 Mar 2003 01:41:50 -0800
> "Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx> wrote:

>>be much easier to do if you'd split the patch up.  Doing that would also
>>make it easier to provide support for other GUIs (which can be done on a
> 
> 
> I also ported this to GTK2, and made a pure standalone patch (attached):
> 
> Essentially only the mouse features from #3529, minus highlighting of
> unit tiles, which was for mass orders.
> 
> - Rectangular selection with right click and drag.
> - Cities become highlighted with a sprite on the map,
>   and become exclusively selected in the City List window.
> - Shift + Left click on owned city or visible unit copies to clipboard.
> - Shift + Right click does paste city production.
> - Shift + Control Right click buys production.
> - Wakeup sentries moved to Control + Middle click.
> - Adjust workers moved to Shift + Control Left click.
> 
>       Function Wakeup sentries had it's own gtk signal connect on mouse
>       button press. All combinations of Shift/Control/plain mouse button
>       presses are now in one function, one gtk signal connected.
> 
> So, this is an intuitive light version of the the most frequently used
> features in the City List window, done directly on-map.
> 
> And then there is the sprite itself. Attatched here for browsing, but you
> still need to apply #3532 to make it work. gfx.tar.gz and gfx-Feb-25.diff.
> autogen.sh required.
> 
> Of course, if you agree this patch and its sprite belongs in cvs, I can gimp
> it into one of the existing .png files, or create a new .png with it's own
> .spec, which can be expanded later. But I wouldn't create one new .png for
> every new little sprite, it's a bit awkward.

I haven't fully figured out this code, but I do have some comments.

> +/**************************************************************************
>  ...
>  **************************************************************************/
>  gint move_mapcanvas(GtkWidget *widget, GdkEventButton *event)
>  {
>    update_line(event->x, event->y);
> +  if (!hover_state) {
> +    update_rect_at_mouse_pos();
> +  }
>    return TRUE;
>  }

Is hover_state really the correct check here?

> diff -ruN -Xdiff_ignore_makefile cvs-Mar-12/client/gui-mui/cityrep.c 
> rectangle/client/gui-mui/cityrep.c
> --- cvs-Mar-12/client/gui-mui/cityrep.c       Tue Feb 18 15:52:19 2003
> +++ rectangle/client/gui-mui/cityrep.c        Thu Mar 13 15:02:50 2003
> @@ -61,6 +61,22 @@
>  static Object *cityrep_change_button;
>  static Object *cityrep_configure_objects[NUM_CREPORT_COLS];
>  
> +/****************************************************************
> +...
> +*****************************************************************/
> +void cities_on_map_selection(void)
> +{
> +  /* PORTME */
> +}

Function header comments are needed here.  They should be the same 
throughout all GUIs.

> +/* On-map Area Selection */
> +void cities_on_map_selection(void);
> +void city_on_map_select(struct city *pcity, bool on_off);

These function names mean very little to me.  But perhaps just adding a 
function header comment will help here.

> +/* Mapcanvas clipboard, defaults to Settler (0) */
> +static int clipboard = 0;

Settler is not always 0.  I suggest introducing -1 as a special-case for 
no clipboard.  Alternately you need to check if 0 is a valid production 
item (you need to check this anyway, but perhaps you already do).

> +/* Selection Rectangle */
> +static int first_x, first_y, old_x, old_y;

As (file-wide) global variables these should have more explicit names.

> +/* This changes the behaviour of left mouse button */

Changes how?

> +**************************************************************************/
> +void draw_rectangle(int src_x, int src_y, int dest_x, int dest_y,
> +                    bool draw)
> +{
> +  int x1, y1, x2, y2;
> +  int dist_x, dist_y;
> +  int i, test_x;
> +
> +  dist_x = real_map_distance(src_x, src_y, dest_x, src_y);
> +  dist_y = real_map_distance(src_x, src_y, src_x, dest_y);
> +
> +  test_x = src_x + dist_x;
> +  normalize_map_pos(&test_x, &src_y);

Any time you call normalize_map_pos you need to check the result.  But 
I'm not quite sure what the correct behavior should be in this case if 
it returns FALSE.  Is this impossible (if so, you should assert(0))?  Or 
is some other handling needed?

> +  if (test_x == dest_x)  {        /* is dest to the right of src? */
> +    x1 = src_x;     x2 = dest_x;
> +  } else  {
> +    x1 = dest_x;    x2 = src_x;
> +  }
> +  if (dest_y >= src_y) {          /* vertical is simple */
> +    y1 = src_y;     y2 = dest_y;
> +  } else  {
> +    y1 = dest_y;    y2 = src_y;
> +  }

This code assumes the current topology, although doing differently may 
turn out to be very difficult.  What if the map wraps in the Y 
direction?  What if it is a isometric map (i.e., what civ2/smac/civ3 use)?

> +    /*  Now we make tiles within the rectangle, that contain owned
> +     *  cities, highlighted on the map and selected in the City List
> +     *  window.
> +     *
> +     *      ptile->selected == 1    owned city
> +     */
> +
> +    dist_x = real_map_distance(x1, y1, x2, y1);
> +    dist_y = real_map_distance(x1, y1, x1, y2);
> +
> +    /* Maybe use new macro rectangle_iterate here */
> +    for (yy = 0; yy <= dist_y; yy++) {
> +      for (xx = 0; xx <= dist_x; xx++) {
> +        x = x1 + xx;
> +        y = y1 + yy;
> +        normalize_map_pos(&x, &y);
> +
> +        ptile = map_get_tile(x, y);
> +        pcity = map_get_city(x, y);
> +
> +        if (pcity && pcity->owner == game.player_idx) {
> +          ptile->selected = 1;  /* highlight */
> +          refresh_tile_mapcanvas(x, y, TRUE);
> +          tiles_highlighted_cities = TRUE;
> +        }
> +      }
> +    }

This particular code looks like it will immediately segfault if you drag 
your rectangle off into "unreal" positions.  This is impossible now, but 
please don't assume it.  Something like

   if (!normalize_map_pos(&x, &y)) continue;

is all that is needed.

> +    connection_do_buffer(&aconnection);
> +    city_list_iterate(game.player_ptr->cities, pcity) {
> +      if (map_get_tile(pcity->x, pcity->y)->selected == 1)
> +        send_production_packet(pcity);
> +    } city_list_iterate_end;
> +    connection_do_unbuffer(&aconnection);

I'm not sure what the best way to handle this loop is:

- Loop over all cities in the civ.  This is O(c) where c is the number 
of cities.

- Loop over all tiles in the rectangle.  This is O(a) where a is the 
(tile) area of the rectangle.

Both may end up with degraded performance in certain cases.  As a side 
note, this question comes up in other places also...

> +static void send_production_packet(struct city *pcity)

This name is misleading; it should have "clipboard" in it somewhere. 
Some other function names are similar.

> +    whole_map_iterate(x, y) {
> +      ptile = map_get_tile(x, y);
> +      if (ptile->selected) {
> +        ptile->selected = 0;
> +        refresh_tile_mapcanvas(x, y, TRUE);
> +      }
> +    } whole_map_iterate_end;

This function is very inefficient.

- Is whole_map_iterate necessary at all?  Can't we just limit the 
iteration to those tiles covered by the rectangle?

- Calling refresh_tile_mapcanvas here is quite inefficent and results in 
many unnecessary draws (up to 7x as many as needed) in iso-view. 
update_map_canvas() would be better (but again you have to consider the 
range of the rectangle).

- You need not pass write_to_screen==TRUE here (with the new flush code 
it should pass FALSE instead).

>  /***************************************************************
> diff -ruN -Xdiff_ignore_makefile cvs-Mar-12/common/map.h 
> rectangle/common/map.h
> --- cvs-Mar-12/common/map.h   Thu Mar 13 14:27:49 2003
> +++ rectangle/common/map.h    Thu Mar 13 14:36:44 2003
> @@ -62,6 +62,8 @@
>    struct city *worked;      /* city working tile, or NULL if none */
>    unsigned short continent;
>    signed char move_cost[8]; /* don't know if this helps! */
> +  int selected;             /* Area Selection. 1=city. Client only.
> +                               Leave as int. */
>  };

I suppose it is most efficient to add a new field to the map structure, 
that is only used by the client.  But why is it an 'int' not a 'bool'?

-----

It should eventually be possible for the rectangle to turn out 
rectangular in iso-view as well.  To make this change easier, I think 
update_rectangle should take the *canvas* coordinates of the mouse, not 
the tile coordinates.  In fact, I think if you do everything in canvas 
coordinates all of the wrapping issues will end up being much easier.

-----

Documentation is needed for all of these new features.  I'm not sure 
what to do about the inconsistency they introduce between the different 
GUI interfaces.  But then, I'm not sure how existing interface 
documentation is done.  Vasco?

-----

As I said, these are just a few thoughts not a full critique.  My 
impression, though, is that the overall design is pretty good but a fair 
bit of the implementation needs a much deeper look.  You may not 
understand the different coordinate systems well enough to provide an 
implementation that can correctly handle different topologies with both 
iso and non-iso view; hopefully I can help here.

jason