ASLPieceMover: on drag gesture use correct component

[bsnyder788]

No description provided.

[uckelman]

Would you explain why you believe this change to be correct?

[derimmer]

@uckelman the code in ASLPieceMover.java is very similar to the code in Vassal's PieceMover class. As far as I can tell, drawWin is always going to be passed from either method to SwingUtilities.convertPointToScreen() as null. And it will therefore cause an NPE in convertPointToScreen. Am I misunderstanding something? If the call from PieceMover does cause an NPE, is it being handled somewhere? If so, we are probably missing that in ASLPIeceMover. Thanks.

[uckelman]

As far as I can tell, drawWin is always going to be passed from either method to SwingUtilities.convertPointToScreen() as null.

That's not right. dragWin will not be null, and setDrawWinToOwnerOf(dragWin); sets drawWin from it. If you have NPE stack traces from this, I'd like to see them.

This also seems not to address the change.

[bsnyder788]

java.lang.NullPointerException: Cannot invoke "java.awt.Component.getX()" because "c" is null
at java.desktop/javax.swing.SwingUtilities.convertPointToScreen(SwingUtilities.java:445)
at VASL.build.module.map.ASLPieceMover$DragHandlerNoImage.dragGestureRecognized(ASLPieceMover.java:1605)
at VASSAL.build.module.Map.lambda$addTo$4(Map.java:974)
at java.desktop/java.awt.dnd.DragGestureRecognizer.fireDragGestureRecognized(DragGestureRecognizer.java:363)
at java.desktop/sun.awt.X11.XMouseDragGestureRecognizer.mouseDragged(XMouseDragGestureRecognizer.java:217)
at java.desktop/java.awt.AWTEventMulticaster.mouseDragged(AWTEventMulticaster.java:328)
at java.desktop/java.awt.Component.processMouseMotionEvent(Component.java:6674)
at java.desktop/javax.swing.JComponent.processMouseMotionEvent(JComponent.java:3407)
at java.desktop/java.awt.Component.processEvent(Component.java:6395)
at java.desktop/java.awt.Container.processEvent(Container.java:2266)
at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:5001)
at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2324)
at java.desktop/java.awt.Component.dispatchEvent(Component.java:4833)
at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4948)
at java.desktop/java.awt.LightweightDispatcher.processMouseEvent(Container.java:4592)
at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Container.java:4516)
at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2310)
at java.desktop/java.awt.Window.dispatchEventImpl(Window.java:2780)
at java.desktop/java.awt.Component.dispatchEvent(Component.java:4833)
at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:775)
at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:720)
at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:714)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:97)
at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:747)
at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:744)
at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

[bsnyder788]

As far as I can tell, drawWin is always going to be passed from either method to SwingUtilities.convertPointToScreen() as null.

That's not right. dragWin will not be null, and setDrawWinToOwnerOf(dragWin); sets drawWin from it. If you have NPE stack traces from this, I'd like to see them.

This also seems not to address the change.

Where do you see where setDrawWinToOwnerOf(dragWin) setting drawWin. That method looks to be empty (and deprecated to me). Unless there is something else overriding that. https://github.com/vasl-developers/vasl/blob/develop/src/VASL/build/module/map/ASLPieceMover.java#L928-L936

[bsnyder788]

I could see a case for using setDrawWin(dragWin); instead of setDrawWinToOwnerOf(dragWin);, and then continuing to pass drawWin into the line I modified in this PR

[uckelman]

Where do you see where setDrawWinToOwnerOf(dragWin) setting drawWin. That method looks to be empty (and deprecated to me).

It's in the version of AbstractDragHandler in Vassal. I hadn't realized the one here differs. You're getting an NPE because you're missing the code in setDrawWinToOwnerOf which would prevent it.

https://github.com/vassalengine/vassal/blob/master/vassal-app/src/main/java/VASSAL/build/module/map/PieceMover.java

The fact that ASLPieceMover copies so much code from PieceMover causes problems like this. (And possibly also the alignment problems people have been reporting.) It might be worth reworking ASLPieceMover to override only the parts you actually need in VASL, instead of copying wholesale from an out-of-date version of PieceMover.

[derimmer]

The fact that ASLPieceMover copies so much code from PieceMover causes problems like this. (And possibly also the alignment problems people have been reporting.) It might be worth reworking ASLPieceMover to override only the parts you actually need in VASL, instead of copying wholesale from an out-of-date version of PieceMover.

Understood. When we were making changes to ASLPieceMover as part of the creation of a zoom function that differentially zooms counters and boards, the person coding was having issues overriding parts of PieceMover, especially Abstract DragHandler. So we ended up copying lots of PieceMover as you say. Agree that it would make some sense to scale that back.

[derimmer]

<<It's in the version of AbstractDragHandler in Vassal. I hadn't realized the one here differs. You're getting an NPE because you're missing the code in setDrawWinToOwnerOf which would prevent it.>>

Opening PieceMover from VASSAL3.7.12 in my dev environment, I can find 4 references to "setDrawWinToOwnerOf".

1. an empty (abstract) method in the class AbstractDragHandler
2. a concrete implementation of the method in the class DragHandlerNoImage
3. a call to the method in the method DragHandlerNoImage.dragGestureRecognized()
4. a call to the method in the method DragHandlerNoImage.dragEnter().

Looking at ASLPieceMover in VASL6.6.9-beta2.vmod, I can find (1), (3), and (4) but NOT (2) the concrete implementation in class DragHandlerNoImage.

It seems to me that without the concrete implementation of the method, any calls to it will result in a null response.

One issue we have is that, using my dev environment on my PC, I cannot trap a call to setDrawWinToOwnerOf. Neither of the two calling methods in DragHandlerNoImage seem to be triggered. However, two Linux users have reported NPE bugs coming specifically from the dragGestureRecognized() method. It is not clear to me why that method is being called on their devices and not on mine. I have replicated the steps they use that consistently generate the NPE and I cannot do it.

I am going to create new version of VASL-6.6.9 that will contain the missing concrete implementation in ASLPieceMover and see if that solves the issue for our two Linux users.

Then I will return to the larger question of how to avoid replacing AbstractDragHandler in ASLPieceMover.

[derimmer]

I am going to create new version of VASL-6.6.9 that will contain the missing concrete implementation in ASLPieceMover and see if that solves the issue for our two Linux users.

This was pushed yesterday as commit #1772 . It will be in the upcoming VASL6.6.9-beta4.vmod release.