Bugzilla – Bug 12817
Right-clicking on unexpanded stacks no longer selects top piece
Last modified: 2020-05-20 22:21:38 CEST
From as far back as at least 3.3.0-svn9294, right-clicking on an unexpanded stack no longer selects the top piece. As changes between 3.2.17 and 3.3.0-svn9294 are mostly for Java 9 compatibility and don't contain changes to UI code, I strongly suspect this problem is caused by a change to Java between Java 8 and Java 14.
I think the place to start looking is line 110 of KeyBufferer.java
Surely isMetaDown() hasn't changed?
I added this to the top of mousePressed():
System.out.println("e == " + e);
I clicked with the left mouse button:
e == java.awt.event.MouseEvent[MOUSE_PRESSED,(1223,501),absolute(1979,767),button=1,modifiers=Button1,extModifiers=Button1,clickCount=1] on VASSAL.build.module.Map$View[,-2,-2,3400x2200,layout=java.awt.FlowLayout,alignmentX=0.0,alignmentY=0.0,border=javax.swing.plaf.synth.SynthBorder@13fda5d4,flags=297,maximumSize=,minimumSize=,preferredSize=]
That seems ok.
I clicked with the right mouse button:
e == java.awt.event.MouseEvent[MOUSE_PRESSED,(1225,505),absolute(1981,771),button=3,modifiers=Meta+Button3,extModifiers=Button3,clickCount=1] on VASSAL.build.module.Map$View[,-2,-2,3400x2200,layout=java.awt.FlowLayout,alignmentX=0.0,alignmentY=0.0,border=javax.swing.plaf.synth.SynthBorder@13fda5d4,flags=297,maximumSize=,minimumSize=,preferredSize=]
Wait, what? I just clicked. I didn't click while holding down any keys, but it's showing Meta as a modifier. Hmm.
Also, when I middle-click:
e == java.awt.event.MouseEvent[MOUSE_PRESSED,(1096,273),absolute(1588,541),button=2,modifiers=Alt+Button2,extModifiers=Button2,clickCount=1] on VASSAL.build.module.Map$View[,-266,0,3400x2200,layout=java.awt.FlowLayout,alignmentX=0.0,alignmentY=0.0,border=javax.swing.plaf.synth.SynthBorder@13fda5d4,flags=297,maximumSize=,minimumSize=,preferredSize=]
I wasn't holding down Alt when I did this. Not sure what's going on here.
It sort of looks like modifiers is no longer correct? InputEvent.getModifiers() was deprecated in Java 9... but isMetaDown() wasn't, so I'd expect that to still work.
For legacy-Mac-one-button-mouse reasons, isMetaDown() has historically been linked to right button mouse clicks (and is used that way in various parts of Vassal code). I could easily imagine that similar thinking may have led to isAltDown() being associated (gah!) with the middle mouse.
With isMetaDown() apparently working right it seems like it wouldn't fall through into the "else" and select all ... unless it's getting sent through here "an extra time" or something? I don't have my dev environment set up for Java 12/13/14 yet, but a breakpoint set inside the else here might be educational.
OMG. They _did_ change isMetaDown(). This is what I get when I add print some diagnostic information from 3.2.17 and run it with Java 8:
e == java.awt.event.MouseEvent[MOUSE_PRESSED,(1090,588),absolute(1764,791),button=3,modifiers=Meta+Button3,extModifiers=Button3,clickCount=1] on VASSAL.build.module.Map$View[,-2,-2,3400x2200,layout=java.awt.FlowLayout,alignmentX=0.0,alignmentY=0.0,border=javax.swing.plaf.synth.SynthBorder@29ebf684,flags=297,maximumSize=,minimumSize=,preferredSize=]
e.isMetaDown() == true
With Java 14, e.isMetaDown() is false when I right-click.
Holy Flipping Crud, Batman!
Well that means MORE places throughout the code need a hasty update.
I know there's a different (and probably less deprecated) way of detecting right-clicks with I think it's BUTTON_MASK_3 or however you spell it. But anyway time for a global-search for isMetaDown because it's a number of places in this same role.
We'll also need to go hunting for isAltDown(), as this will have also screwed that up.
I think you're clear on isAltDown() -- there are only two references in the whole codebase (that I could find), and both related to use of the actual Alt key (for "Immobilized" Does-Not-Stack units that only select when Alt key down). I don't think Vassal has any special middle-mouse-button stuff, and if it does it doesn't seem to use the legacy way of detecting it.
Whereas I've got 21 matches in 17 files for isMetaDown().
... meaning to say I think isAltDown() will still work for detecting the actual Alt key, which is the only application of the method that Vassal is using.
Whereas all the isMetaDown()'s are going to have to be turned, presumably, into something more like...
((ev.getModifiers() & InputEvent.BUTTON3_MASK) == InputEvent.BUTTON3_MASK)
> ((ev.getModifiers() & InputEvent.BUTTON3_MASK) == InputEvent.BUTTON3_MASK)
getModifiers() is deprecated; looks like getModifiersEx() is the recommended replacement. (InputEvent.BUTTON3_MASK is also deprecated...)
e.getButton() == 3
do the job in the cases where we need to check for the right button? Or is that going to get us into trouble with single-button mice? (Does anyone still use those?)
I guess so - wow, all the Java that everybody "knows" for mouse is now deprecated!
My only concern is "what about multiple buttons at once" but a valid position to take on that is "who cares?" :)
By the way, this is starting to somewhat color the discussion of "I wonder what version of Java we should require Linux to run..." Not definitive, but certainly coloring it.
> My only concern is "what about multiple buttons at once" but a valid position to take on that is "who cares?" :)
Because MouseEvent is a state change, I think it can't be for more than one button---if you press two, one of them got pressed first and the other second---and getButton() will return the one for which the event is a change event.
Now, you can have other buttons specified in whatever getModifiersEx() returns, so you could see multiple buttons there, but I suspect that there's no place in VASSAL where we check for multiple buttons, in which case we don't need to start now. I'll keep my eyes open for that as I work through the changes.
(In reply to Brian Reynolds from comment #13)
> By the way, this is starting to somewhat color the discussion of "I wonder
> what version of Java we should require Linux to run..." Not definitive, but
> certainly coloring it.
All of these deprecations are from Java 9. (I have yet to run into something deprecated since Java 8 which _wasn't_ deprecated at Java 9.) Since Java 11 is a long-term support release and virtually everyone on Linux has 11 or later, that's why I set 11 as the minimum.
One last thing if you wouldn't mind unlocking bug 12554 (Undo Bug), I have been doing some debugging on it. Haven't quite found it yet but I have ruled out several things that it is not. It's actually executing the apparently proper set of MovePiece commands (to move the thing back both places), and in the right order, and yet the second one doesn't quite seem to "take". Anyway, happy to put some updates there (or if I should just put that stuff in the Developers forum that's fine too -- whichever is preferred).
Done. (I hadn't realized that one was locked.)
In going through the various uses of isMetaDown(), I've discovered that we've literally never handled popup menus correctly---the way you're supposed to do it, back so far that there's not even a "Since" version in the javadoc---is to check isPopupTrigger(). Argh.
I just want to point out the javadoc for MouseEvent.isPopupTrigger():
"Popup menus are triggered differently on different systems. Therefore, isPopupTrigger should be checked in both mousePressed and mouseReleased for proper cross-platform functionality."
What complete bollocks. Thank you for giving me a cross-platform GUI toolkit which still makes me do all the work myself. Surely this is the sort of thing which a cross-platform GUI toolkit should handle if it's good for anything at all?!
I'm going to pour myself a whiskey now.
EXCELLENT IDEA, SIR! Whiskey it is, here too. Immediately.
Only 11 isMetaDown() now...
The remaining instances of isMetaDown():
* These functions have ones which aren't associated with raising a popop menu, so shouldn't be replaced by isPopupTrigger():
I guess isMetaDown() should be replaced by getButton() == 3?
* There's one weird case:
This is using (event.isMetaDown() || event.isShiftDown()) for opening a dialog. That's sort of like raising a popup menu? So maybe this also should be changed to isPopupTrigger() || event.isShiftDown()?
I would try getButton()==3 for all of them because it's the closest to the method which was working before, which is to say brute force.
("if brute force isn't working, it means you aren't using enough brute force...") :)
I think this is fixed as of trunk@svn9399.
If you have time, have a quick swing through 12554 and see if my latest report "rings any bells". Mostly I'm trying to decide if it's worth "going deeper" or of this is more of an "avert your eyes" situation.
Also I responded to your questions on the Chatter mod in the forum thread.