Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-03 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Pepeto _ wrote:
> http://bugs.freeciv.org/Ticket/Display.html?id=39602 >
> 
>> [EMAIL PROTECTED] - Lun. Déc. 03 18:45:11 2007]:
>>
>> William Allen Simpson wrote:
>>
>>> (2) One of them has not worked for about two years, and certainly
> was not
>>> tested at the time it was committed.
>> I'm quite sure it worked when it was committed.
> 
> The code you committed was not clean. It worked for one selected unit.
> For more, it crashed. But I don't blame you. In a so big patch, This is
> 'human' to make at least one mistake. Multi unit selection feature
> couldn't be fixed in only one commit, that's normal...

The original patch was provided by a warclient developer and worked.  I 
broke it when battlegroups were introduced, clearly a rather foolish 
programming error.

-jason



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-03 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

William Allen Simpson wrote:
> http://bugs.freeciv.org/Ticket/Display.html?id=39602 >
> 
> Jason Dorje Short wrote:
>> William Allen Simpson wrote:
>>> (2) One of them has not worked for about two years, and certainly was not
>>> tested at the time it was committed.
>> I'm quite sure it worked when it was committed.
>>
> Here's the patch from your message of Wed, 16 Nov 2005 18:35:19 -0800,
> and I don't see any difference from what was there last week:
> 
> @@ -2324,10 +2623,9 @@
>   **/
>   void key_quickselect(enum quickselect_type qtype)
>   {
> -  struct unit *punit;
> +  unit_list_iterate(get_units_in_focus(), punit) {
> +struct unit *punit2 = quickselect(punit->tile, qtype);
> 
> -  if(punit_focus) {
> -punit = quickselect(punit_focus->tile, qtype);
> -set_unit_focus_and_select(punit);
> -  }
> +set_unit_focus_and_select(punit2);
> +  } unit_list_iterate_end;
>   }

Yes, the previously working behavior was broken in that commit, and 
nobody noticed until it was released since all the good multiplayer 
players use the stable branch.  We know this already.

> Have you tested LMB and RMB?  I don't have a useful test scenario.

They work but I'm not sure if it's as the behavior is intended. 
Obviously this code should be examined.

-jason



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


[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-03 Thread Pepeto _

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

> [EMAIL PROTECTED] - Lun. Déc. 03 18:45:11 2007]:
> 
> William Allen Simpson wrote:
> 
> > (2) One of them has not worked for about two years, and certainly
was not
> > tested at the time it was committed.
> 
> I'm quite sure it worked when it was committed.

The code you committed was not clean. It worked for one selected unit.
For more, it crashed. But I don't blame you. In a so big patch, This is
'human' to make at least one mistake. Multi unit selection feature
couldn't be fixed in only one commit, that's normal...


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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-03 Thread William Allen Simpson

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Jason Dorje Short wrote:
> William Allen Simpson wrote:
>> (2) One of them has not worked for about two years, and certainly was not
>> tested at the time it was committed.
> 
> I'm quite sure it worked when it was committed.
> 
Here's the patch from your message of Wed, 16 Nov 2005 18:35:19 -0800,
and I don't see any difference from what was there last week:

@@ -2324,10 +2623,9 @@
  **/
  void key_quickselect(enum quickselect_type qtype)
  {
-  struct unit *punit;
+  unit_list_iterate(get_units_in_focus(), punit) {
+struct unit *punit2 = quickselect(punit->tile, qtype);

-  if(punit_focus) {
-punit = quickselect(punit_focus->tile, qtype);
-set_unit_focus_and_select(punit);
-  }
+set_unit_focus_and_select(punit2);
+  } unit_list_iterate_end;
  }

It was a bad idea then, a bad idea today, and will be a bad idea tomorrow.

Let's stick to keeping working code working, and stop obsessing about code
that hasn't worked in years

Have you tested LMB and RMB?  I don't have a useful test scenario.



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-03 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

William Allen Simpson wrote:

> (1) You have two ways of doing the same thing.

Should be fixed.

> (2) One of them has not worked for about two years, and certainly was not
> tested at the time it was committed.

I'm quite sure it worked when it was committed.

> (3) It crashes.  We're not talking about failing an assert, or some other
> known problem, we're talking hard crashes!

Quite.  Fortunately it's a 2-line fix.

> (4) It is not easily used by laptops.

That IS a problem.  As discussed on irc and requested many times 
throughout the life of the progject, gtk menu keys really should be 
configurable.

> (5) It was not important enough to be implemented by all clients -- nor
> *any* other client!

At the time it was added gui-gtk was pretty much the only client.

-jason



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-03 Thread William Allen Simpson

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Daniel Markstedt wrote:
> I agree that this was done in too hasty a fashion. William, if you
> wanted this code gone so badly, you could at least have presented some
> kind of roadmap for reimplementation.
> 
There is no *need* for reimplementation.  There is *already* another
implementation using the mouse.

This is an example of one of the serious problems with this project:
careless and untested implementation.  I came to this project to help add
new features, but instead have spent nearly all my time trying to cleanup
old trash.

(1) You have two ways of doing the same thing.

(2) One of them has not worked for about two years, and certainly was not
tested at the time it was committed.

(3) It crashes.  We're not talking about failing an assert, or some other
known problem, we're talking hard crashes!

(4) It is not easily used by laptops.

(5) It was not important enough to be implemented by all clients -- nor
*any* other client!

(6) It was never documented.

There is absolutely no excuse for a "stable" branch having this kind of
crap in it at all.  It's not stable.  And there's no good reason for
doing it *again* in development

If you want to spend time on something, make sure the remaining method
always works and is well documented.

BTW, your poll is terribly inaccurate and incomplete.  It forgot to
mention the mouse quickselect that does the same things.  And anybody
that says they've used the feature (even seldom) is probably lying,
since it crashed immediately in 2.1, and I'm not sure it worked in
earlier versions.  They may have used it in a warclient, but that's
irrelevant to this project.



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

William Allen Simpson wrote:
> http://bugs.freeciv.org/Ticket/Display.html?id=39602 >
> 
> Jason Dorje Short wrote:
>> Also, it makes no technical sense to delete an existing keyboard command 
>>   that duplicates a mouse command.  We still have exactly the same 
>> problem we had before but without the keyboard shortcut for it.  Instead 
>> we should fix the commands so that they don't have two separate backend 
>> implementations, and fix them to work properly.
>>
> Presumably, the mouse command actually works (at least the code is much
> longer).  I didn't find a bug report for it.  So, no, we don't have "exactly
> the same problem".  Please re-read the code.

The crash problem having been easily solved, the remaining problem 
becomes what to do when multiple units are selected.

> And tell us where the mouse command is documented?!?!?

Excellent point.

I suspect a thorough review of all commands and their documentation 
would be in order.

-jason



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread Daniel Markstedt

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

On 12/3/07, Jason Dorje Short <[EMAIL PROTECTED]> wrote:
>
> http://bugs.freeciv.org/Ticket/Display.html?id=39602 >
>
> William Allen Simpson wrote:
>
> > Since this was broken (by you in PR#14365) circa Dec 2005, and has had only
> > one report (Aug 2007) in two (2) years, it is clearly not a commonly used
> > command.  It has never worked.  It was never documented.  It is not in
> > either xaw or sdl.  Going, going, gone
>
> Um, no.
>
>
> First of all, deleting features from the stable branch is not something
> that should be done lightly.  Nor should you, in general, delete things
> from any branch without the chance for discussion.
> [...]

I agree that this was done in too hasty a fashion. William, if you
wanted this code gone so badly, you could at least have presented some
kind of roadmap for reimplementation.

To hear the opinion of the gaming commuity, I started a poll at:
http://forum.freeciv.org/viewtopic.php?t=4218

 ~Daniel



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread William Allen Simpson

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Jason Dorje Short wrote:
> Also, it makes no technical sense to delete an existing keyboard command 
>   that duplicates a mouse command.  We still have exactly the same 
> problem we had before but without the keyboard shortcut for it.  Instead 
> we should fix the commands so that they don't have two separate backend 
> implementations, and fix them to work properly.
> 
Presumably, the mouse command actually works (at least the code is much
longer).  I didn't find a bug report for it.  So, no, we don't have "exactly
the same problem".  Please re-read the code.

And tell us where the mouse command is documented?!?!?



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

William Allen Simpson wrote:

> Since this was broken (by you in PR#14365) circa Dec 2005, and has had only
> one report (Aug 2007) in two (2) years, it is clearly not a commonly used
> command.  It has never worked.  It was never documented.  It is not in
> either xaw or sdl.  Going, going, gone

Um, no.


First of all, deleting features from the stable branch is not something 
that should be done lightly.  Nor should you, in general, delete things 
from any branch without the chance for discussion.

Secondly, there was discussion, which you ignored.


If we were to continue the discussion, it would go something like this: 
the bug was reported shortly after 2.1 was released and has since been a 
continual complaint on the forums (I'm told); therefore it clearly IS a 
command that is used.

Also, it makes no technical sense to delete an existing keyboard command 
  that duplicates a mouse command.  We still have exactly the same 
problem we had before but without the keyboard shortcut for it.  Instead 
we should fix the commands so that they don't have two separate backend 
implementations, and fix them to work properly.


Please rethink your unreasonable position.

-jason



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


[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread Pepeto _

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

> [jdorje - Dim. Déc. 02 01:59:36 2007]:
> 
> Your last patch looks like it does what it says it does.  And I agree
> the current code is broken (modifying a unit list inside the list
> iterator is a no-no, that's what iterate_safe is for, but even with
> iterate_safe the behavior is broken as it just selected different units
> repeatedly).
> 
> What I question is why you'd want the behavior to be that way when there
> are multiple units selected.  Say I have 5 units on 5 different tiles
> selected...what use would be a quickselect that gives me 5 different
> land units, one per tile?  I thought the purpose of quickselect was to
> give just ONE unit for quick action in multiplayer games.
> 
> In that context I would think the key_quickselect action should just
> pick one unit on one tile to bring into focus.  The question then
> becomes (when the battlegroup spans multiple tiles), which tile to
> choose.  I'm no expect on multiplayer freeciv but I'd think the tile
> nearest the center of the screen would be the most likely to be the one
> the player wants.
> 
> -jason
> 
> 

I thought about it a long time. I think the last issue I found was the
more practical/nearer than the current way... 


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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread William Allen Simpson

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Committed S2_1 revision 14112.
Committed S2_2 revision 14113.
Committed trunk revision 14114.



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-02 Thread William Allen Simpson

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Jason Dorje Short wrote:
> No need to read the code, just ask anyone who plays multiplayer.
> 
The code itself is canonical.


> The quickselect is intended to pick a quick unit from the current stack 
> to do a quick action.
> 
Wrong.  The code comments explicitly say "Focus unit is excluded."  The
code matches the comments [client/control.c line 2028]:

 if (game.player_ptr != unit_owner(punit) || unit_is_in_focus(punit)) {
   continue;
 }

Perhaps that careless misunderstanding is how the code was so badly done
(but doesn't explain how it was egregiously never tested before commit).


> The / and * keys duplicate the behavior of the RMB and LMB.  This should 
> be documented and those key commands added to the menu.  And while we're 
> at it we should pick keys that aren't so random.
> 
We should eliminate duplicate functions.  They are confusing and cause
extra work to document and maintain.

Since this was broken (by you in PR#14365) circa Dec 2005, and has had only
one report (Aug 2007) in two (2) years, it is clearly not a commonly used
command.  It has never worked.  It was never documented.  It is not in
either xaw or sdl.  Going, going, gone



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-01 Thread Jason Dorje Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

William Allen Simpson wrote:

> Reading the code to try to divine the original intent, it makes no sense to
> have a special purpose key to filter land or sea units from an existing
> selected group.  The only time that could possibly be used would be in a
> port, and only when both ships and land units are present.
> 
> This especially make no sense with battlegroups that can be on more than one
> tile.  More than one port?  Battlegroups with both land and sea units?

No need to read the code, just ask anyone who plays multiplayer.

The quickselect is intended to pick a quick unit from the current stack 
to do a quick action.  Since the most important action is around ports 
which almost always contain both land and sea units, the case you 
dismiss is actually one of the more common ones.

The / and * keys duplicate the behavior of the RMB and LMB.  This should 
be documented and those key commands added to the menu.  And while we're 
at it we should pick keys that aren't so random.

-jason



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


Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-01 Thread William Allen Simpson

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Yet another of the myriad bugs introduced by PR#14365.  I'm so sick and
tired of cleaning up this battlegroup mess!

These keys are undocumented intercepts (just as ESC) that appear only in the
gui-gtk2 client.  They are not in any menu.  Apparently, their only real
purpose is to crash.  Only warclient folks would even notice them (probably
by furiously trying every key).

   http://www.youtube.com/watch?v=TTwgNhX4BSo

Looking at the proposed patches, they are inconsistent.  None performs a
semantically useful function.  They fix the symptom without addressing the
underlying problem.

Reading the code to try to divine the original intent, it makes no sense to
have a special purpose key to filter land or sea units from an existing
selected group.  The only time that could possibly be used would be in a
port, and only when both ships and land units are present.

This especially make no sense with battlegroups that can be on more than one
tile.  More than one port?  Battlegroups with both land and sea units?

(Before PR#14365, it would try to find a land or sea unit that wasn't
currently selected.  That's duplicated by  LMB/RMB already in
mapview for both gtk2 and xaw.  But not documented!)

It's a waste of time to maintain such undocumented and infrequently useful
keys  I'm deleting the code.

Index: client/control.c
===
--- client/control.c(revision 14109)
+++ client/control.c(working copy)
@@ -1990,8 +1990,8 @@
 
 /**
  Quickselecting a unit is normally done with  left, right click,
- or keypad / * for the current tile. Bypassing the stack popup is quite
- convenient, and can be tactically important in furious multiplayer games.
+ for the current tile. Bypassing the stack popup is quite convenient, 
+ and can be tactically important in furious multiplayer games.
 **/
 static struct unit *quickselect(struct tile *ptile,
 enum quickselect_type qtype)
@@ -2770,15 +2770,3 @@
 {
   request_toggle_fog_of_war();
 }
-
-/**
-...
-**/
-void key_quickselect(enum quickselect_type qtype)
-{
-  unit_list_iterate(get_units_in_focus(), punit) {
-struct unit *punit2 = quickselect(punit->tile, qtype);
-
-set_unit_focus_and_select(punit2);
-  } unit_list_iterate_end;
-}
Index: client/gui-gtk-2.0/gui_main.c
===
--- client/gui-gtk-2.0/gui_main.c   (revision 14109)
+++ client/gui-gtk-2.0/gui_main.c   (working copy)
@@ -595,14 +595,6 @@
 key_city_workers(w, ev);
 break;
 
-  case GDK_KP_Divide:
-key_quickselect(SELECT_SEA);
-break;
-
-  case GDK_KP_Multiply:
-key_quickselect(SELECT_LAND);
-break;
-
   default:
 return FALSE;
   }
Index: client/control.h
===
--- client/control.h(revision 14109)
+++ client/control.h(working copy)
@@ -169,7 +169,6 @@
 void key_city_outlines_toggle(void);
 void key_map_grid_toggle(void);
 void key_map_borders_toggle(void);
-void key_quickselect(enum quickselect_type qtype);
 void key_recall_previous_focus_unit(void);
 void key_unit_move(enum direction8 gui_dir);
 void key_unit_airbase(void);
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev


[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-01 Thread Jason Short

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

Your last patch looks like it does what it says it does.  And I agree
the current code is broken (modifying a unit list inside the list
iterator is a no-no, that's what iterate_safe is for, but even with
iterate_safe the behavior is broken as it just selected different units
repeatedly).

What I question is why you'd want the behavior to be that way when there
are multiple units selected.  Say I have 5 units on 5 different tiles
selected...what use would be a quickselect that gives me 5 different
land units, one per tile?  I thought the purpose of quickselect was to
give just ONE unit for quick action in multiplayer games.

In that context I would think the key_quickselect action should just
pick one unit on one tile to bring into focus.  The question then
becomes (when the battlegroup spans multiple tiles), which tile to
choose.  I'm no expect on multiplayer freeciv but I'd think the tile
nearest the center of the screen would be the most likely to be the one
the player wants.

-jason


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


[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-12-01 Thread Daniel Markstedt

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

> [pepeto - Tue Aug 21 21:40:57 2007]:
> 
> I got a crash. I selected many units with 'y', and then I typed on a
> quickselect key '*' or '/':
> 

Can reproduce like this on 2.1.1 win32-gtk2: select at least three of the same 
kind of unit and press '*' - client crash!

 ~Daniel

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


[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-10-15 Thread Pepeto _

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

New patch: It selects new units on the tiles of the current units in
focus. It selects only 1 unit by tile, to avoid unit selection duplication .

Index: client/control.c
===
--- client/control.c(révision 13752)
+++ client/control.c(copie de travail)
@@ -2726,13 +2726,41 @@
 }
 
 /**
-...
+  Selects new units on the same tiles than the previous selected ones.
+  Allows only one unit by tile.
 **/
 void key_quickselect(enum quickselect_type qtype)
 {
+  struct unit_list *punits = unit_list_new();
+  struct unit *punit2;
+  bool tiles[MAP_INDEX_SIZE], first;
+
+  memset(tiles, 0, sizeof(tiles));
+  /* Collect the units */
   unit_list_iterate(get_units_in_focus(), punit) {
-struct unit *punit2 = quickselect(punit->tile, qtype);
+if (tiles[punit->tile->index]) {
+  /* One unit by tile only */
+  continue;
+}
 
-set_unit_focus_and_select(punit2);
+punit2 = quickselect(punit->tile, qtype);
+if (punit2) {
+  unit_list_append(punits, punit2);
+}
+tiles[punit->tile->index] = TRUE;
   } unit_list_iterate_end;
+
+  /* Set them in the focus */
+  first = TRUE;
+  unit_list_iterate(punits, punit) {
+if (first) {
+  set_unit_focus_and_select(punit);
+  first = FALSE;
+} else {
+  add_unit_focus(punit);
+}
+  } unit_list_iterate_end;
+
+  unit_list_unlink_all(punits);
+  unit_list_free(punits);
 }

Index: client/control.c
===
--- client/control.c	(révision 13752)
+++ client/control.c	(copie de travail)
@@ -2726,13 +2726,41 @@
 }
 
 /**
-...
+  Selects new units on the same tiles than the previous selected ones.
+  Allows only one unit by tile.
 **/
 void key_quickselect(enum quickselect_type qtype)
 {
+  struct unit_list *punits = unit_list_new();
+  struct unit *punit2;
+  bool tiles[MAP_INDEX_SIZE], first;
+
+  memset(tiles, 0, sizeof(tiles));
+  /* Collect the units */
   unit_list_iterate(get_units_in_focus(), punit) {
-struct unit *punit2 = quickselect(punit->tile, qtype);
+if (tiles[punit->tile->index]) {
+  /* One unit by tile only */
+  continue;
+}
 
-set_unit_focus_and_select(punit2);
+punit2 = quickselect(punit->tile, qtype);
+if (punit2) {
+  unit_list_append(punits, punit2);
+}
+tiles[punit->tile->index] = TRUE;
   } unit_list_iterate_end;
+
+  /* Set them in the focus */
+  first = TRUE;
+  unit_list_iterate(punits, punit) {
+if (first) {
+  set_unit_focus_and_select(punit);
+  first = FALSE;
+} else {
+  add_unit_focus(punit);
+}
+  } unit_list_iterate_end;
+
+  unit_list_unlink_all(punits);
+  unit_list_free(punits);
 }
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev


[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units

2007-08-21 Thread Pepeto _

http://bugs.freeciv.org/Ticket/Display.html?id=39602 >

I got a crash. I selected many units with 'y', and then I typed on a
quickselect key '*' or '/':

#0  quickselect (ptile=0x101, qtype=SELECT_LAND) at
../utility/speclist.h:110
listsize = 
panytransporter = 
panymovesea = 
panysea = 
panymoveland = 
panyland = 
panymoveunit = 
panyunit = 
__PRETTY_FUNCTION__ = "quickselect"
#1  0x0041a79e in key_quickselect (qtype=SELECT_LAND) at
control.c:2704
punit2 = 
myiter = (struct genlist_link *) 0x1
#2  0x004b86ae in keyboard_handler (w=0x9010a0, ev=0x1ee2b10, 
data=) at gui_main.c:587
No locals.
#3  0x2afa41d1f68d in _gtk_marshal_BOOLEAN__BOXED ()
   from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#4  0x2afa444187da in g_closure_invoke () from
/usr/lib/libgobject-2.0.so.0
No symbol table info available.
#5  0x2afa44428408 in ?? () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#6  0x2afa44429617 in g_signal_emit_valist ()
   from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#7  0x2afa44429a13 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0
No symbol table info available.
#8  0x2afa41e1d13e in ?? () from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#9  0x2afa41d18d45 in gtk_propagate_event ()
   from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#10 0x2afa41d19c91 in gtk_main_do_event ()
   from /usr/lib/libgtk-x11-2.0.so.0
No symbol table info available.
#11 0x2afa421bf45c in ?? () from /usr/lib/libgdk-x11-2.0.so.0
No symbol table info available.
#12 0x2afa44a85a14 in g_main_context_dispatch ()
   from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#13 0x2afa44a8885d in ?? () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#14 0x2afa44a88b6a in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
No symbol table info available.
#15 0x2afa41d1a023 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#16 0x004ba44a in ui_main (argc=1, argv=0x7fff690ed898)
at gui_main.c:1424
home = 
sig = 
style = 
#17 0x00413f35 in main (argc=, 
argv=0x7fff690ed898) at civclient.c:357
i = 1
loglevel = 2
ui_options = 
ui_separator = 
option = 
user_tileset = false

/**
...
**/
void key_quickselect(enum quickselect_type qtype)
{
  unit_list_iterate(get_units_in_focus(), punit) {
struct unit *punit2 = quickselect(punit->tile, qtype);

set_unit_focus_and_select(punit2);
  } unit_list_iterate_end;
}

This function is all wrong. If you set a unit in focus whereas you are
iterating the same list, there will be in this function some free links.
set_unit_focus_and_select() will call unit_list_unlink_all(), then the
links which are always in the key_quickselect() function will be wrong.

Anyway I don't think it's a good idea to allow quickselect with many
selected units. I propose 2 patches...
--- client/control.c.old	2007-08-21 23:59:24.0 +0200
+++ client/control.c	2007-08-21 23:59:31.0 +0200
@@ -2709,9 +2709,13 @@
 **/
 void key_quickselect(enum quickselect_type qtype)
 {
-  unit_list_iterate(get_units_in_focus(), punit) {
-struct unit *punit2 = quickselect(punit->tile, qtype);
+  struct unit_list *punits = get_units_in_focus();
+  struct unit *punit;
 
-set_unit_focus_and_select(punit2);
-  } unit_list_iterate_end;
+  if (unit_list_size(punits) == 0) {
+return;
+  }
+
+  punit = unit_list_get(punits, 0);
+  set_unit_focus_and_select(quickselect(punit->tile, qtype));
 }
--- client/control.c.old	2007-08-21 23:59:24.0 +0200
+++ client/control.c	2007-08-22 00:04:00.0 +0200
@@ -2709,9 +2709,15 @@
 **/
 void key_quickselect(enum quickselect_type qtype)
 {
-  unit_list_iterate(get_units_in_focus(), punit) {
-struct unit *punit2 = quickselect(punit->tile, qtype);
+  struct unit_list *punits = get_units_in_focus();
+  struct unit *punit;
+  int size = unit_list_size(punits);
 
-set_unit_focus_and_select(punit2);
-  } unit_list_iterate_end;
+  if (size == 1) {
+punit = unit_list_get(punits, 0);
+set_unit_focus_and_select(quickselect(punit->tile, qtype));
+  } else if (size > 1) {
+append_output_window(_("You can use 'quick select' "
+   "when many units are selected."));
+  }
 }
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev