Request to backport 6736649 and 6797139

[Dr Andrew John Hughes]

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.

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

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.

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

> Thanks,
> Omair

Thanks,
-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint = F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8