Request to backport 6736649 and 6797139

[Omair Majid]

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.

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

> 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

Thanks,
Omair