Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
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
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
Re: [Freeciv-Dev] (PR#39602) quickselect doesn't like the selection of many units
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
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
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
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
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
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
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
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
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
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
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
