Hi Mario, Roman,
Thanks for additional details. I'm not really an expert in the mouse
events handling area, but the fix looks scary. I recall we put a lot of
effort in supporting mice with many buttons in Java (by many I mean like
7, 10, and even more buttons). Looking at the former
XConstants.buttonsMask array and the new XlibUtil.getButtonMask(), it
/seems/ (please keep in mind, I lack deep expertise in this area) that
you effectively drop support for multi-buttons mice. Is this so? Did you
run any tests? And specifically, have you tried an advanced mouse
(gaming mice, I believe, always have plenty of buttons)? Did they still
work fine after you fix, and are able to send events for all their
buttons to Java?
A little nit, in XWindow.java:
- ((mouseButtonClickAllowed & XConstants.buttonsMask[lbutton])
!= 0) ) // No up-button in the drag-state
+ ((mouseButtonClickAllowed & XlibUtil.getButtonMask(lbutton))
!= 0) ) // No up-button in the drag-state
Shouldn't it read lbutton+1 in the second line? The same question for
line 750.
Overall, is it possible to somehow simplify the fix? Perhaps we only
want to fix the code that actually installs the window grab, and leave
all the mouse buttons logic intact?
--
best regards,
Anthony
On 11/12/12 17:58, Roman Kennke wrote:
Am Donnerstag, den 08.11.2012, 18:00 +0100 schrieb Mario Torre:
Hi all!
I would like to propose a fix for this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=853079
The root cause of the bug is an event being generated that sets the
grabWindowRef on the window confusing the internal events state.
Usually, when a mouse press is detected the grabWindowRef is set.
However, in some cases (like the reproducer for bug in the report, but
there are likely other conditions that have similar behaviour), the
mouse events carry some flags that the actual code mistakenly interpret
causing the grabWindowRef to not be unset.
The proposed patch address the code for the button event mapping:
http://cr.openjdk.java.net/~neugens/853079/webrev.01/
We have no easy way to automatically provide a test case for this bug
unfortunately.
I could reproduce the bug with OpenJDK 6, 7 and 8 as well as Oracle JDK,
so it's a long lasting bug. I'm not sure if it's worth to eventually
backport to 7 as well if approved.
Let me add some little explanations (I worked with Mario on this
bugfix). The underlying problem is that the bitmask that we get in every
event (event-state) has 5 bits for button state (for the first 5
buttons). All other bits are used for other stuff. However, we get
number-of-buttons from X11 reported as 10, and hence look at 10 buttons
in that bitmask, which is wrong. In our particular case, the
KeymapStateMask was set (bit #14) and this lead to button-release
handler assuming not all buttons have been released and hence screwing
up the grab-state. However, the problem is more general: if we consider
more than 5 buttons in that mask, we risk to run into problems like
that. The fix limits to 5 buttons when dealing with the event-state
mask. (And, this is very reasonable: the other buttons are usually not
real buttons, but stuff like mouse-wheel or touch-scroll thingies.)
Kind regards,
Roman
