<AWT Dev> [8] [PATCH] Review request for 8001633: Wrong alt processing during switching between windows.

[Mikhail Cherkasov]

Anton,
In this case alt_event will be processed twice, first time by AWT 
window, second time by AltProcessor.  But AltProcessor can not move 
focus to AWT menu, because it still requires JRootPane to obtain JMenu.
Any way we should not try to make altProcessor work with AWT windows.
I think AltProcessor should do nothing if winAncestor is null.
Question is how it should be done?
We can keep variant from the last patch or add explicit check like this:
if ( winAncestor == null ) {
return;
}

On 10/31/2012 11:11 AM, Anton V. Tarasov wrote:
> On 31.10.2012 0:36, Mikhail Cherkasov wrote:
>> Anton,
>> Applet viewer processes all alt_events correct with/without this fix 
>> because applet viewer
>> is from AWT world. So NPE problem appears due mixing  awt windows and 
>> swing windows.
>> But this is a special case, and I hope no one will use both AWT and 
>> Swing windows in one application.
>
> I'm afraid this architecture may still be valid for our Swing/AWT reg 
> tests, at least. And so, we're better to handle it.
> In some way like this:
>
> if (tk instanceof SunToolkit) {
>     Window ancestor = (winAncestor == null ?
>         SunToolkit.getContainingWindow(ev.getComponent())  : 
> winAncestor);
>
>     // do the rest with ancestor
> }
>
> What do you think?
>
> Thanks,
> Anton.
>
>>
>> So I attached new version, with reversed the check order.
>>
>> On 10/30/2012 4:57 PM, Anton V. Tarasov wrote:
>>> On 30.10.2012 16:45, Mikhail Cherkasov wrote:
>>>> Anton, when we switch to Applet Viewer and press the 'alt' key
>>>>  AltProcessor.postProcessKeyEvent executes the following 
>>>> lines(173-175):
>>>>
>>>> root = SwingUtilities.getRootPane(ev.getComponent());
>>>> winAncestor = (root == null ? null :
>>>> SwingUtilities.getWindowAncestor(root));
>>>>
>>>> SwingUtilities.getRootPane(ev.getComponent()); returns null for 
>>>> Applet Viewer.
>>>> So when AltProcessor tries to process 'alt' release event the 
>>>> following code throws NPE:
>>>>
>>>> ev.getWhen() <= ((SunToolkit) 
>>>> tk).getWindowDeactivationTime(winAncestor); (~145 line)
>>>>
>>>> If new check goes first, the check with NPE will not be executed, 
>>>> but it's no explicit way to fix this.
>>>
>>> Well, I think I've missed the latest findings related to the NPE (I 
>>> thought it was caused by null AppContext).
>>> In this case I'm ok with the simple rearrangement. However, another 
>>> concern arises here. According to the suggested
>>> code all alt_released events will be skipped inside an AppletViewer. 
>>> Am I right?
>>>
>>> Thanks,
>>> Anton.
>>>
>>>>
>>>> May be better to add explicit check at the beginning of 
>>>> AltProcessor.altPressed method:
>>>> if (winAncestor == null ) {
>>>>   return; // nothing to do here because we have no window.
>>>> } ?
>>>>
>>>> On 10/30/2012 3:35 PM, Anton V. Tarasov wrote:
>>>>> Mikhail,
>>>>>
>>>>> If SunToolkit.getContainingWindow just forces AppContext 
>>>>> initialization (?), than I'd prefer to fix it in some direct way,
>>>>> or if that is impossible or hard to do, make appropriate comments.
>>>>>
>>>>> Thanks,
>>>>> Anton.
>>>>>
>>>>> On 30.10.2012 15:28, Mikhail Cherkasov wrote:
>>>>>> Leonid, you're right, reversing fix NPE.
>>>>>> Please find new patch and webrev in attachments.
>>>>>>
>>>>>> On 10/29/2012 11:40 PM, Leonid Romanov wrote:
>>>>>>> Hi,
>>>>>>> I've found that reversing the check order in 
>>>>>>> WindowsRootPaneUI.java like
>>>>>>> this
>>>>>>>
>>>>>>> skip = SunToolkit.getContainingWindow(originalSource) != 
>>>>>>> winAncestor
>>>>>>>     || ev.getWhen() <= ((SunToolkit)
>>>>>>> tk).getWindowDeactivationTime(winAncestor);
>>>>>>>
>>>>>>> also fixes the alt-tab exception regression. Could you check 
>>>>>>> please that
>>>>>>> this change indeed fixes the exception bug and if it does, 
>>>>>>> incorporate it
>>>>>>> into your patch.
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: awt-dev-bounces at openjdk.java.net [mailto:awt-dev-
>>>>>>>> bounces at openjdk.java.net] On Behalf Of Mikhail Cherkasov
>>>>>>>> Sent: Monday, October 29, 2012 8:18 PM
>>>>>>>> To: Anton V. Tarasov; awt-dev at openjdk.java.net
>>>>>>>> Subject: Re: <AWT Dev> [8] [PATCH] Review request for 8001633: 
>>>>>>>> Wrong alt
>>>>>>>> processing during switching between windows.
>>>>>>>>
>>>>>>>> Please review the second version:
>>>>>>>> http://cr.openjdk.java.net/~alexp/8001633/webrev.01/
>>>>>>>> <http://cr.openjdk.java.net/%7Ealexp/8001633/webrev.01/>
>>>>>>>> All remarks was corrected.
>>>>>>>>
>>>>>>>> On 10/29/2012 4:32 PM, Anton V. Tarasov wrote:
>>>>>>>>> Hi Mikhail,
>>>>>>>>>
>>>>>>>>> * KeyEvent.java
>>>>>>>>>
>>>>>>>>> - No need to initialize 'originalSource' in constructors which 
>>>>>>>>> call to
>>>>>>>>> this(...) where you already initialized it.
>>>>>>>>>
>>>>>>>>> -950      * we need to able to obtain original source.
>>>>>>>>>
>>>>>>>>> "be" is omitted ("we need to be able")
>>>>>>>>>
>>>>>>>>> * WindowsRootPaneUI.java
>>>>>>>>>
>>>>>>>>> I think there's no need to put another 'skip' setting into a 
>>>>>>>>> separate
>>>>>>>>> if-block (skip will be equal 'false' in majority of cases).
>>>>>>>>> Why don't you write it simply as follows?
>>>>>>>>>
>>>>>>>>> Component originalSource =
>>>>>>>>> AWTAccessor.getKeyEventAccessor().getOriginalSource(ev);
>>>>>>>>>
>>>>>>>>> skip = (ev.getWhen()<=
>>>>>>>>> ((SunToolkit)tk).getWindowDeactivationTime(winAncestor)) ||
>>>>>>>>> SwingUtilities.getWindowAncestor(originalSource) != winAncestor);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Anton.
>>>>>>>>>
>>>>>>>>> On 29.10.2012 14:23, Mikhail Cherkasov wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Could you please review a fix for 8001633
>>>>>>>>>> <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8001633>:
>>>>>>>> Wrong
>>>>>>>>>> alt processing during switching between windows.
>>>>>>>>>> Bug:http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8001633
>>>>>>>>>> <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8001633>
>>>>>>>>>> Webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~bagiras/8/7082294.1/
>>>>>>>>>>
>>>>>>>>>> To prevent wrong focus traversing to menu was added 
>>>>>>>>>> additional check
>>>>>>>>>> to AltProcessor.
>>>>>>>>>> It checks that original source of 'alt' event belongs to
>>>>>>>>>> AltProcessor.winAncestor
>>>>>>>>>> or its component.
>>>>>>>>>>
>>>>>>>>>> Patch is attached.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Mikhail.
>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>