Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: 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
URL: 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
URL: 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 controlLMB 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
[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: 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
URL: http://bugs.freeciv.org/Ticket/Display.html?id=39602
William Allen Simpson wrote:
URL: 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 controlLMB 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
Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: http://bugs.freeciv.org/Ticket/Display.html?id=39602 Pepeto _ wrote: URL: 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
URL: 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
URL: 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
[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: 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
URL: 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
Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: 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
URL: http://bugs.freeciv.org/Ticket/Display.html?id=39602 On 12/3/07, Jason Dorje Short [EMAIL PROTECTED] wrote: URL: 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
URL: http://bugs.freeciv.org/Ticket/Display.html?id=39602 William Allen Simpson wrote: URL: 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
[Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: 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
URL: 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
Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
URL: 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 control 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 control 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
URL: 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
