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

[Anton V. Tarasov]

Mikhail,

Ok, after all it sounds reasonable. I'm fine with the last webrev.

Thanks,
Anton.

On 31.10.2012 17:13, Mikhail Cherkasov wrote:
> 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.
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>