[Freeciv-Dev] Re: Freeciv commit: ali: Rectangular area selection of cities with righ...

[Raimar Falke <i-freeciv-lists@xxxxxxxxxxxxx>]

On Fri, Nov 14, 2003 at 04:40:40AM -0800, Raimar Falke wrote:
> Date: Fri Nov 14 12:26:56 2003-Fri Nov 14 12:26:58 2003
> Author: ali
> Subject: ali:  Rectangular area selection of cities with righ...
> Files:
>   client/control.c:1.117
>   client/gui-gtk-2.0/cityrep.c:1.51
>   client/gui-gtk-2.0/gui_main.c:1.62
>   client/gui-gtk-2.0/mapctrl.c:1.33
>   client/gui-gtk-2.0/mapctrl.h:1.8
>   client/gui-gtk-2.0/mapview.c:1.81
>   client/gui-gtk/cityrep.c:1.81
>   client/gui-gtk/gui_main.c:1.140
>   client/gui-gtk/mapctrl.c:1.91
>   client/gui-gtk/mapctrl.h:1.15
>   client/gui-gtk/mapview.c:1.184
>   client/gui-mui/cityrep.c:1.28
>   client/gui-mui/mapctrl.c:1.16
>   client/gui-mui/mapview.c:1.63
>   client/gui-sdl/cityrep.c:1.12
>   client/gui-sdl/mapctrl.c:1.35
>   client/gui-sdl/mapview.c:1.64
>   client/gui-stub/cityrep.c:1.5
>   client/gui-stub/mapctrl.c:1.8
>   client/gui-stub/mapview.c:1.35
>   client/gui-win32/cityrep.c:1.27
>   client/gui-win32/mapctrl.c:1.30
>   client/gui-win32/mapview.c:1.83
>   client/gui-xaw/cityrep.c:1.52
>   client/gui-xaw/mapctrl.c:1.78
>   client/gui-xaw/mapview.c:1.150
>   client/include/cityrep_g.h:1.5
>   client/include/mapctrl_g.h:1.11
>   client/include/mapview_g.h:1.41
>   client/mapctrl_common.c:1.19
>   client/mapctrl_common.h:1.12
>   client/mapview_common.c:1.69
>   client/packhand.c:1.337
>   common/map.c:1.150
>   common/map.h:1.163
>   data/helpdata.txt:1.124
> Message:
>   Rectangular area selection of cities with right-click- and-drag.
>   Patch by me.
> Lines: 1776

I didn't review this patch before but during updating the delta patch
I found some problems:

> +/**************************************************************************
> +...
> +**************************************************************************/
> +static void clipboard_send_production_packet(struct city *pcity)
> +{
> +  struct packet_city_request packet;
> +  int cid = clipboard;

The correct type is cid and not int.

> +  if (clipboard_is_unit)  {
> +    cid += B_LAST;
> +  }

This is strongly discouraged. It violates the abstraction of the cid
data type.

It should be written as

  cid cid = cid_encode(clipboard_is_unit, clipboard);

> +  if ((pcity->currently_building == clipboard
> +    && pcity->is_building_unit == clipboard_is_unit)
> +      || !city_can_build_impr_or_unit(pcity, cid))  {
> +    return;
> +  }

This may also be rewritten as

  if (cid == cid_encode_from_city(pcity)
      || !city_can_build_impr_or_unit(pcity, cid)) {
    return;
  }

A general question is if we shouldn't (in the client) introduce cid
everywhere.

>  struct tile {
>    enum tile_terrain_type terrain;
>    enum tile_special_type special;
> @@ -60,6 +65,7 @@
>    unsigned short continent;
>    signed char move_cost[8]; /* don't know if this helps! */
>    struct player *owner;     /* Player owning this tile, or NULL. */
> +  enum tile_hilite hilite;  /* Area Selection. Client only. */
>  };

A seperate struct should be introduced to mark it clearly that this
field is client-only. See struct connection as an example.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Only one human captain has ever survived battle with the Minbari
  fleet. He is behind me. You are in front of me. If you value your 
  lives, be somewhere else."
    -- Ambassador Delenn, "Severed Dreams," Babylon 5