<AWT Dev> Bug 853079: focus problems with openjdk 1.7.0 under gnome3 when selcted keyboard is not the first in keyboard list

[Anthony Petrov]

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
>>>
> 
>