Create account / Log in
Bug 12817 - Right-clicking on unexpanded stacks no longer selects top piece
Right-clicking on unexpanded stacks no longer selects top piece
Status: RESOLVED FIXED
Product: VASSAL
Classification: Applications
Component: Player
3.3.0-beta3
All All
: high blocker
: 3.3.0
Assigned To: Joel Uckelman
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-17 01:25 CEST by Joel Uckelman
Modified: 2020-05-20 22:21 CEST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joel Uckelman 2020-05-17 01:25:33 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.
Comment 1 Brian Reynolds 2020-05-17 02:18:26 CEST
I think the place to start looking is line 110 of KeyBufferer.java

Surely isMetaDown() hasn't changed?
Comment 2 Joel Uckelman 2020-05-17 16:55:14 CEST
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.
Comment 3 Joel Uckelman 2020-05-17 16:56:32 CEST
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.
Comment 4 Joel Uckelman 2020-05-17 17:10:46 CEST
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.
Comment 5 Brian Reynolds 2020-05-17 17:39:50 CEST
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.
Comment 6 Joel Uckelman 2020-05-17 17:40:30 CEST
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.
Comment 7 Brian Reynolds 2020-05-17 17:52:46 CEST
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.
Comment 8 Joel Uckelman 2020-05-17 19:03:42 CEST
We'll also need to go hunting for isAltDown(), as this will have also screwed that up.
Comment 9 Brian Reynolds 2020-05-17 19:15:40 CEST
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().
Comment 10 Brian Reynolds 2020-05-17 19:18:43 CEST
... 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)
Comment 11 Joel Uckelman 2020-05-17 22:29:55 CEST
> ((ev.getModifiers() & InputEvent.BUTTON3_MASK) == InputEvent.BUTTON3_MASK)

getModifiers() is deprecated; looks like getModifiersEx() is the recommended replacement. (InputEvent.BUTTON3_MASK is also deprecated...)

Won't

  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?)
Comment 12 Brian Reynolds 2020-05-17 22:35:07 CEST
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?" :)
Comment 13 Brian Reynolds 2020-05-17 22:39:38 CEST
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.
Comment 14 Joel Uckelman 2020-05-17 22:44:38 CEST
> 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.
Comment 15 Joel Uckelman 2020-05-17 22:47:32 CEST
(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.
Comment 16 Brian Reynolds 2020-05-17 23:40:51 CEST
Sounds solid.

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).
Comment 17 Joel Uckelman 2020-05-17 23:44:20 CEST
Done. (I hadn't realized that one was locked.)
Comment 18 Joel Uckelman 2020-05-17 23:46:34 CEST
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.
Comment 19 Joel Uckelman 2020-05-18 00:32:21 CEST
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.
Comment 20 Brian Reynolds 2020-05-18 00:33:44 CEST
EXCELLENT IDEA, SIR! Whiskey it is, here too. Immediately.
Comment 21 Joel Uckelman 2020-05-18 23:32:54 CEST
Only 11 isMetaDown() now...
Comment 22 Joel Uckelman 2020-05-19 12:20:53 CEST
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():

  Map.mouseDragged()
  PieceMover.canHandleEvent()
  RegionGrid.Config.View.mouseDragged()
  PolygonEditor.ModifyPolygon.mousePressed()

I guess isMetaDown() should be replaced by getButton() == 3?

* There's one weird case:

  PropertySheet.TickLabel.mouseClicked()

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()?

Thoughts?
Comment 23 Brian Reynolds 2020-05-19 14:40:53 CEST
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...") :)
Comment 24 Joel Uckelman 2020-05-20 01:53:51 CEST
I think this is fixed as of trunk@svn9399.
Comment 25 Brian Reynolds 2020-05-20 02:21:51 CEST
Yay!

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.