Hi Jonathan,
Thanks! I marked the bug as Fix Available.
Regards, Pavel
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
