<Swing Dev> Accessibility inner classes in AccessibleJTable do not stick to AccessibleComponent.getLocationOnScreen api

[Jonathan Lu]

Hi Pavel,

Thanks for review, the change has been pushed to
http://hg.openjdk.java.net/jdk8/awt/jdk/rev/2fe9c1f0b16b

changeset:   5692:2fe9c1f0b16b
tag:         tip
user:        dingxmin
date:        Mon Aug 20 13:16:18 2012 +0800
files:       src/share/classes/javax/swing/JTable.java 
test/javax/swing/JTable/7188612/JTableAccessibleGetLocationOnScreen.java
description:
7188612: JTable's AccessibleJTable throws IllegalComponentStateException 
instead of null
Reviewed-by: rupashka

And to Frank, could you pls verify it?

Thanks
Jonathan

On 08/17/2012 07:13 PM, Pavel Porvatov wrote:
> Hi Frank,
>> Hi Pavel,
>>   Thanks for your review again.  Another review package incorporating 
>> your comments is @ 
>> http://cr.openjdk.java.net/~dingxmin/7188612/webrev.01
>>   Could you please take a look at it ?
> The fix looks good for me.
>
> Regards, Pavel
>>
>> Thanks,
>> Frank
>>
>> On 8/16/2012 11:02 PM, Pavel Porvatov wrote:
>>> Hi Frank,
>>>> Hi Pavel,
>>>>   Thanks for your comments from which I learned a lot.  A new 
>>>> review patch is created by incorporating your comments @
>>>>
>>>>   http://cr.openjdk.java.net/~dingxmin/7188612/webrev.00/
>>>>
>>>>   Could you please review it again?
>>> I have few comments about the last webrev:
>>> 1. There is no need to re-throw exceptions via exThrownInEDT. You 
>>> can just throw RuntimeException if the test fails. It's ok to throw 
>>> RuntimeException on the EDT, jtreg will fail in that case
>>> 2. Can we join two SwingUtilities.invokeAndWait into one?
>>> 3. Comments
>>> 96         // cast to interface AccessibleComponent to call 
>>> getLocationOnScreen()
>>> and
>>> 109         // cast to interface AccessibleComponent to call 
>>> getLocationOnScreen()
>>> look unnecessary
>>>
>>> Other code looks good for me
>>>
>>> Regards, Pavel
>>>>
>>>> Best regards,
>>>> Frank
>>>>
>>>> On 8/6/2012 9:54 PM, Pavel Porvatov wrote:
>>>>> Hi Frank,
>>>>>
>>>>> The fix looks good but I have several comments for the test:
>>>>> 1. The Swing components should be created and accessed only from 
>>>>> EDT thread. Therefore the assertGetLocation method should be on 
>>>>> EDT as well
>>>>>
>>>>> 2. I'd remove lines 59-61
>>>>>
>>>>> 3. The lines
>>>>>  79         // Display the window.
>>>>>  80         frame.setVisible(true);
>>>>>  81         // make the frame invisible to test 
>>>>> getLocationOnScreen() of
>>>>>  82         // JTable$AccessibleJTable$AccessibleJTableHeaderCell
>>>>>  83         // and JTable$AccessibleJTable$AccessibleJTableCell
>>>>>  84         frame.setVisible(false);
>>>>> don't guaranty that the frame becomes visible for awhile. You 
>>>>> should use SunToolkit.realsync for that (take a look at other 
>>>>> tests). Is it really necessary for the test to show and hide the 
>>>>> frame?
>>>>>
>>>>> 4. Why don't you cast getAccessibleContext into AccessibleTable in 
>>>>> one line, but wrote six lines for that:
>>>>>  88         AccessibleContext context = (AccessibleContext) table
>>>>>  89                 .getAccessibleContext();
>>>>>  90         // To get 
>>>>> JTable$AccessibleJTable$AccessibleJTableHeaderCell instance,
>>>>>  91         // we need to first
>>>>>  92         // convert the context var to type AccessibleTable
>>>>>  93         AccessibleTable accessibleTable = (AccessibleTable) 
>>>>> context;
>>>>>
>>>>> 5. Could you please remove useless comments like:
>>>>>  79         // Display the window.
>>>>>  80         frame.setVisible(true);
>>>>>
>>>>>  90         // To get 
>>>>> JTable$AccessibleJTable$AccessibleJTableHeaderCell instance,
>>>>>  91         // we need to first
>>>>>  92         // convert the context var to type AccessibleTable
>>>>>
>>>>>  94         // and then get 
>>>>> JTable$AccessibleJTable$AccessibleTableHeader
>>>>>
>>>>> etc.
>>>>>
>>>>> Only comments like
>>>>>  81         // make the frame invisible to test 
>>>>> getLocationOnScreen() of
>>>>>  82         // JTable$AccessibleJTable$AccessibleJTableHeaderCell
>>>>>  83         // and JTable$AccessibleJTable$AccessibleJTableCell
>>>>>
>>>>> 100         // getLocation() must be null according to its javadoc 
>>>>> and no exception
>>>>> 101         // is thrown
>>>>> looks reasonable for me in the provided test
>>>>>
>>>>> Regards, Pavel
>>>>>
>>>>>
>>>>>> Hi guys
>>>>>>   According to javadoc of 
>>>>>> AccessibleComponent.getLocationOnScreen, it returns null instead 
>>>>>> of throwing IllegalComponentStateException when the corresponding 
>>>>>> GUI object is not visible.
>>>>>>   However, two accessibility inner classes of JTable, 
>>>>>> JTable$AccessibleJTable$AccessibleJTableCell and 
>>>>>> Table$AccessibleJTable$AccessibleJTableHeaderCell implementing 
>>>>>> AccessibleComponent throw IllegalComponentStateException when the 
>>>>>> corresponding JTable is not visible.  This behavior conflicts 
>>>>>> with the spec.
>>>>>>   I have submitted a bug in sunbug system reporting the issue, 
>>>>>> and received two acknowledgement emails (bug id 7188613 and 
>>>>>> 7188612, don't know why there are two?).  I created a patch to 
>>>>>> fix the issue with a jtreg test case. Both the patch and the test 
>>>>>> are uploaded into the following space.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~youdwei/ojdk-491/webrev.02/
>>>>>>
>>>>>>   Would anyone help to review the patch?
>>>>>>
>>>>>> Best regards,
>>>>>> Frank
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>