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

[Frank Ding]

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?

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