Request to backport 6736649 and 6797139

[Omair Majid]

On 01/17/2011 03:00 PM, Dr Andrew John Hughes wrote:
> On 14:29 Mon 17 Jan     , Omair Majid wrote:
>> On 01/17/2011 01:55 PM, Dr Andrew John Hughes wrote:
>>> On 13:19 Mon 17 Jan     , Omair Majid wrote:
>>>> On 01/17/2011 12:00 PM, Dr Andrew John Hughes wrote:
>>>>> On 09:40 Mon 17 Jan     , Omair Majid wrote:
>>>>>> On 01/17/2011 09:10 AM, Dr Andrew John Hughes wrote:
>>>>>>> On 17:35 Fri 14 Jan     , Omair Majid wrote:
>>>>>>>> I would like to backport the following two changesets to openjdk6:
>>>>>>>>
>>>>>>>> changeset:   624:e78c2f17a606
>>>>>>>> user:        mlapshin
>>>>>>>> date:        Tue Aug 26 12:16:23 2008 +0400
>>>>>>>> summary:     6736649:
>>>>>>>> test/closed/javax/swing/JMenuItem/6458123/ManualBug6458123.java fails on
>>>>>>>> Linux
>>>>>>>>
>>>>>>>> changeset:   1637:281fbd82a971
>>>>>>>> user:        alexp
>>>>>>>> date:        Wed Sep 02 17:47:19 2009 +0400
>>>>>>>> summary:     6797139: JButton title is truncating for some strings
>>>>>>>> irrespective of preferred size.
>>>>>>>>
>>>>>>>> Together, these changesets fix a bug where some strings are incorrectly
>>>>>>>> being truncated.
>>>>>>>>
>>>>>>>
>>>>>>> I don't see either of these in IcedTea6.  Please add them there first before
>>>>>>> trying to upstream them.
>>>>>>>
>>>>>>
>>>>>> I was going to add them to IcedTea6 as soon as Joe approved them for
>>>>>> openjdk6. Can I assume you are okay with adding it to IcedTea6 without
>>>>>> waiting for Joe's comments?
>>>>>
>>>>> Post the patch and we can make that decision :-)
>>>>>
>>>>
>>>> I have attached the patch for icedtea6. Ok to commit?
>>>>
>>>> Thanks,
>>>> Omair
>>>
>>> The changesets seem to have a lot of whitespace changes, which make it
>>> hard to read, but as these come from the upstream changesets, there's
>>> not much to be done about that.  Is it correct that the first adds the
>>> calculation of the right width and then the second removes it?
>>>
>>>> +-            textRect.width += getLeftTextExtraWidth() + getRightTextExtraWidth();
>>>> ++            textRect.width += getLeftTextExtraWidth();
>>>
>>
>> Yes. Apparently there were some fixes that were performed before OpenJDK
>> was branched; the second patch rolls back some of those changes. From
>> what I can tell, this rollback patch seems quite hackish. But perhaps
>> this is just the complexity of the JDK code.
>>
>
> It's annoying that so much history has not been preserved correctly.
>

Agreed. The bug reports leave much to be desired as well :(

>>> I'm a little worried by the addition of an exception:
>>>
>>>> +      * Returns the left side bearing of the first character of string. The
>>>> +-     * left side bearing is calculated from the passed in FontMetrics.
>>>> ++     * left side bearing is calculated from the passed in
>>>> ++     * FontMetrics.  If the passed in String is less than one
>>>> ++     * character, this will throw a StringIndexOutOfBoundsException exception.
>>>> +      *
>>>> +      * @param c JComponent that will display the string
>>>> +      * @param fm FontMetrics used to measure the String width
>>>> +@@ -234,14 +245,11 @@
>>>> +      */
>>>> +     public static int getLeftSideBearing(JComponent c, FontMetrics fm,
>>>> +                                          String string) {
>>>> +-        if ((string == null) || (string.length() == 0)) {
>>>> +-            return 0;
>>>> +-        }
>>>> +         return getLeftSideBearing(c, fm, string.charAt(0));
>>>> +     }
>>>
>>> Is this visible further up the stack?
>>>
>>
>> Hm. I missed that. In general yes, it will be visible -
>> sun.swing.SwingUtilities2 is a public class. On the other hand, it is
>> supposed to be an internal class not part of the public API. From the
>> Javadocs:
>>
>> <b>WARNING:</b>  While this class is public, it should not be treated as
>> public API and its API may change in incompatable ways between dot dot
>> releases and even patch releases. You should not rely on this class even
>> existing.
>>
>> Perhaps worse than the exception, the patch removes a public method:
>> +- public static int getRightSideBearing(JComponent c, FontMetrics fm,
>> +-                                       String string) {
>>
>> I am starting to have second thoughts about backporting this.
>>
>> On the other hand, I can see (using reflection) that the proprietary JDK
>> 6 does not have getRightSideBearing methods either.
>>
>
> It doesn't matter about changing the API of classes outside those that form part
> of the public documentation (mainly java.* and javax.*.).  My concern was that
> this exception would travel up the stack and be thrown from a javax.swing method
> where it wasn't previously.  Can you check what calls this method and whether it
> can reach that level?
>

Pretty much any swing program using JMenus will hit this code and cause 
a StringIndexOutOfBoundsException :/

> I'm still for backporting as it fixes a bug users have encountered.  If necessary,
> we can just drop the changes which allow the exception to be thrown.
>

Looks like the openjdk7 folks ran into this same problem and fixed it:

changeset:   1738:9d78c3d9def2
user:        alexp
date:        Mon Sep 21 17:58:09 2009 +0400
summary:     6883341: SWAT: jdk7-b72 swat build(2009-09-17) threw 
exceptions when running Java2D demo by clicking Paint ta

This changeset gets rid of the StringIndexOutOfBoundsException (by 
effectively reverting the exception changes).

>>> Do the new tests pass?  They should be fixed to use the newer Oracle copyright notices.
>>>
>>
>> Sorry, but I forgot to run the tests. I will fix the copyrights, run the
>> tests and post the results. I can confirm that the patch does fix the
>> issue pointed out in the bug report at
>> https://bugzilla.redhat.com/show_bug.cgi?id=661610
>>
>
> This sounds a good reason to backport.
>
> I'll await the results.

I can confirm that the new tests pass. However, I also discovered that 
swing programs using JMenus crash, so I didnt attempt to run all the tests.

I will post an updated patch soon.

Thanks,
Omair