RFR: 8277922: Unable to click JCheckBox in JTable through Java Access Bridge [v3]

[Anton Litvinov]

On Wed, 9 Mar 2022 23:00:37 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Anton Litvinov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   The third version of the fix for JDK-8277922
>
> src/java.desktop/share/classes/javax/swing/JTable.java line 8417:
> 
>> 8415:                 if (ac != null) {
>> 8416:                     return ac.getAccessibleAction();
>> 8417:                 }
> 
> This is the fix for possible NPE, right?

Yes, correct, this "if" condition is a fix for possible NPE, the method "javax.swing.JTable.AccessibleJTable.AccessibleJTableCell.getCurrentAccessibleContext()" may return "null" according to its documentation and to its code. I already took into account this possible NPE in the 1st and 2nd versions of the fix for this bug, and saw it amoral not to take it into account in the 3rd version of the fix, therefore I added this "if (ac != null) {" block in the 3rd fix version.

> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java line 48:
> 
>> 46: import javax.swing.table.TableCellRenderer;
>> 47: 
>> 48: public class BooleanRendererHasAccessibleActionTest {
> 
> Would `BooleanRendererHasNoAccessibleActionTest` be a better name? After all, you test for *no* accessible action.

For almost 10 years of fixing bugs in JDK, I have always given names to the regression tests to describe exactly the failing test scenario, rather then the expected and not failing behavior. So for me the test name "BooleanRendererHasNoAccessibleActionTest" would mean that the test should fail when BooleanRenderer does not have "AccessibleAction", and this is the opposite from what the bug and what I am fixing by the 3rd fix version. I am not going to change the test name.

> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java line 50:
> 
>> 48: public class BooleanRendererHasAccessibleActionTest {
>> 49:     private volatile JFrame frame;
>> 50:     private volatile JTable table;
> 
> Neither `frame` nor `table` are accessed from any other thread but EDT, I believe `volatile` isn't necessary in this case.

I am not going to remove these "volatile" modifiers. These two variables are initialized and assigned "null" values on the Main thread in "main" method and then other values are assigned to them as well as they are accessed from EDT. Hence we got two threads accessing these two references, therefore these references must be thread-safe from my view point. I do not want to start again our endless arguing on this subject, which we periodically had before, in this review request and about variables in the regression test, especially when you are suggesting to decrease thread-safety.

> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java line 142:
> 
>> 140: 
>> 141:     private void testAccessibleActionInCellRenderer(int row, int column,
>> 142:             boolean shouldBeNull) {
> 
> Since `shouldBeNull` is always `true`, I propose to drop it.

In my opinion this argument does not create any troubles for the test but makes real goals achieved by the method "testAccessibleActionInCellRenderer" more clear and makes this test method more generic. I do not want to remove it. If the goal is to optimize the code, then everything in this test can be rearranged, rewritten, I am not trying to achieve the goal to write absolutely minimal test case.

> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java line 143:
> 
>> 141:     private void testAccessibleActionInCellRenderer(int row, int column,
>> 142:             boolean shouldBeNull) {
>> 143:         System.out.println(String.format(
> 
> I suggest using `System.out.printf`, just add `"\n"` to the end of format string.

I am not against that somebody prefers "System.out.printf", let they use it, if they want, but I personally like to use "System.out.println" and "System.out.print" and have full right to use it.

> test/jdk/javax/accessibility/JTable/BooleanRendererHasAccessibleActionTest.java line 161:
> 
>> 159:         AccessibleAction cellRendererAa = cellRendererAc.getAccessibleAction();
>> 160:         if ((shouldBeNull && (cellRendererAa != null)) ||
>> 161:             (!shouldBeNull && (cellRendererAa == null))) {
> 
> If `shouldBeNull` is dropped from parameters, this condition can be simplified to
> Suggestion:
> 
>         if (cellRendererAa != null)) {
> 
> 
> And the error message below should say _'cellRendererAa' is not null_.

Alexey, I refuse to make the forth version of the fix just to remove this innocent "shouldBeNull" argument. Information in the current error message "Test failed. 'cellRendererAa' is not as should be" is enough to understand what is the reason of the failure. To understand what value was expected by the failing test scenario, it is needed just to read in "System.out" the output of the code

143         System.out.println(String.format(
144                 "testAccessibleActionInCellRenderer():" +
145                     " row='%d', column='%d', shouldBeNull='%b'",
146                 row, column, shouldBeNull));

-------------

PR: https://git.openjdk.java.net/jdk/pull/7416