<Swing Dev> RfR JDK-8134116, Add more comprehensive fix and regression test for JDK-8133897

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Thu Nov 12 11:20:11 UTC 2015


   The fix looks good to me.

   Thanks,
   Alexandr.

On 11/11/2015 7:19 PM, Pete Brunet wrote:
> Thanks Alexandr, I made that change and added a test to ensure
> getAccessibleName returns the proper page when the page's component is
> null.  Please see the following webrev.
>
> http://cr.openjdk.java.net/~ptbrunet/JDK-8134116/webrev.05/
>
> Pete
>
> On 11/11/15 2:25 AM, Alexander Scherbatiy wrote:
>> On 11/10/2015 11:52 PM, Pete Brunet wrote:
>>> Thanks Sergey, Please see
>>>
>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8134116/webrev.04/
>>>
>>> parent.indexOfTab(title) is replaced with
>>> parent.indexOfTabComponent(tabComponent).
>>>
>>> The regression test runs OK.
>>    It looks like the check to null component in the getTitleMethod() is
>> not necessary.
>>    Even if a page contains a null component the
>> parent.indexOfComponent(null) returns the correct index.
>>    So we always asks for a component (it may be null) which definitely
>> is containded in the pages list.
>>    Here is a small example:
>>    ---------------
>>      tabbedPane.addTab("Label 1", new JLabel("Label 1"));
>>      tabbedPane.addTab("Null", null);
>>      tabbedPane.addTab("Label 2", new JLabel("Label 2"));
>>      System.out.println("index of null component: " +
>> tabbedPane.indexOfComponent(null)); // returns index 1
>>    ---------------
>>
>>    Thanks,
>>    Alexandr.
>>
>>> Pete
>>>
>>> On 11/10/15 6:12 AM, Sergey Bylokhov wrote:
>>>> Hi, Pete.
>>>> On 31.10.15 6:28, Pete Brunet wrote:
>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8134116/webrev.03/
>>>> My suggestion was to remove all usage of parent.indexOfTab(title) in
>>>> the code and replace it by parent.indexOfTabComponent(comp).
>>>> For example:
>>>>       getTabBounds(parent, parent.indexOfTab(getTitle()));
>>>> Can return incorrect bounds if a few pages will have the same title.
>>>>
>>>> Another problem in the fix is that it iterates over components twice:
>>>> in the getTitle()->(parent.indexOfComponent(component)), and in the
>>>> parent.indexOfTab(title).
>>>>
>>>> Please do not use such comments in the code "// For
>>>> JDK-8133897/JDK-8134116", this information can be obtained from the
>>>> mercurial history.
>>>>
>>>>
>>>>
>>>>>>>        I guess it will be better to don't use the title (especially
>>>>>>> parent.indexOfTab(title)) at all in the our code, except the
>>>>>>> situation
>>>>>>> when we should access the title(like in getAccessibleName()). All
>>>>>>> other cases should be rewritten to use
>>>>>>> parent.indexOfTabComponent(comp). For example your  "private String
>>>>>>> getTitle() {}" can be implemented like this:
>>>>>>>
>>>>>>>       return getTitleAt(parent.indexOfTabComponent(comp));
>>>>>>>
>>>>>>> On 20.10.15 18:45, Pete Brunet wrote:
>>>>>>>> Please review this patch:
>>>>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8134116/webrev.02/
>>>>>>>>
>>>>>>>> The issue raised/fixed in 8133897 and now resolved in a better
>>>>>>>> fashion
>>>>>>>> in this patch is caused by an override of the functionality of
>>>>>>>> JTabbedPane such that its Page inner class title field is not kept
>>>>>>>> up to
>>>>>>>> date by the overriding code. When the Page title field is empty
>>>>>>>> getTitleAt is now called so that the overridden getTitleAt will
>>>>>>>> provide
>>>>>>>> the title.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8134116
>>>>>>>>
>>>>>>>> Pete
>>>>>>>>




More information about the swing-dev mailing list