Request to backport 6736649 and 6797139
Dr Andrew John Hughes
ahughes at redhat.com
Tue Jan 18 15:56:03 PST 2011
On 16:26 Tue 18 Jan , Omair Majid wrote:
> 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).
>
Ok, so I guess I spotted it quicker than them ;-)
Please backport this one as well.
> >>> 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
--
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
More information about the distro-pkg-dev
mailing list