Roman, Mario,
I agree with your reasoning regarding "prefer to
not leave bugs intact". :) I appreciate that you've tested with a
multi-button mouse, this confirms that the fix is safe. I believe that
we don't have any specific tests for this case, so your manual testing
should be enough. Thanks for this!
I re-read the fix more precisely, and it actually looks good to me. Just
a few suggestions:
src/solaris/classes/sun/awt/X11/XlibUtil.java
403 if (button <= 0 || button > 5) {
Should we use XConstants.MAX_BUTTON_MASK instead of a hard-coded value here?
406 return 1 << 7 + button;
I suggest to rewrite this as "1 << (7 + button)" for clarity.
src/solaris/classes/sun/awt/X11/XConstants.java
Perhaps it makes sense to rename ALL_BUTTON_MASKS to ALL_BUTTONS_MASK
since it's only one mask for all the buttons.
--
best regards,
Anthony
On 11/12/2012 6:44 PM, Roman Kennke wrote:
Am Montag, den 12.11.2012, 18:29 +0400 schrieb Anthony Petrov:
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?
This should not be the case. We only removed handling of additional bits
in the event-state mask that *cannot be mouse buttons*. Buttons > 5 are
still reported and handled using button number, just not in the bitmask.
Did you
run any tests? And specifically, have you tried an advanced mouse
(gaming mice, I believe, always have plenty of buttons)?
Yes, we tried a many-button-mouse, however we do not have a specific
many-button test. If you have any specific test in mind that we should
run, please point us to it.
Did they still
work fine after you fix, and are able to send events for all their
buttons to Java?
Well, yeah, as I said, we still get events for everything. What is fixed
is only the way we handle the button-mask. It is simply not possible to
handle more than 5 buttons using this mask, because we would look at
something that is not buttons. (Just look at the input event masks in
XConstants. Bits 8-12 are button masks, the rest are used for different
purposes.)
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.
Yeah this is a little strange. We decided to make
XlibUtil.getButtonMask() 1-based so that incoming button numbers can
easily be converted to button masks (X11 and Java number buttons
starting from 1). This is what happens in this place (correct me if I am
wrong). In some other places we have 0-based loops, which is why we have
to add 1 there. Maybe it would be preferable to make those loops 1-based
so that the code at least looks the same. ?
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?
We believe the mouse button logic is currently broken (see above
discussion on why this is the case). We will likely run into similar
bugs in the future if we leave half of it 'intact'. I would prefer to
not leave bugs intact ;-)
Best regards,
Roman
--
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
