Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
On 3/10/19 4:07 AM, Philip Race wrote: > +1 (approved) > > -phil. > Pushed to client [1]. Thanks! -- [1] - http://hg.openjdk.java.net/jdk/client/rev/0804f29e8be7
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
+1 (approved) -phil. On 3/1/19, 8:58 PM, Martin Balao wrote: On 2/27/19 12:20 PM, Philip Race wrote: The webrev still appears to be against jdk/jdk not jdk/client I had suggested excluding small point sizes since I don't think the failures I saw can possibly be limited to Solaris ... it just hasn't been reproduced elsewhere yet. Apologies for the misunderstanding. Webrev.04: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.04/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.04.zip New: * Re-enabled Solaris * Changed font size to 10px * Unnecessary null check removed * Rebased against jdk/client Thanks, Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
On 2/27/19 12:20 PM, Philip Race wrote: > The webrev still appears to be against jdk/jdk not jdk/client > > I had suggested excluding small point sizes since I don't think > the failures I saw can possibly be limited to Solaris ... it just > hasn't been reproduced elsewhere yet. Apologies for the misunderstanding. Webrev.04: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.04/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.04.zip New: * Re-enabled Solaris * Changed font size to 10px * Unnecessary null check removed * Rebased against jdk/client Thanks, Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
The webrev still appears to be against jdk/jdk not jdk/client I had suggested excluding small point sizes since I don't think the failures I saw can possibly be limited to Solaris ... it just hasn't been reproduced elsewhere yet. -phil. On 2/25/19, 9:06 PM, Martin Balao wrote: Hi, Here we have Webrev.03: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.03/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.03.zip Changes: * Test moved to test/jdk/java/awt/FontMetrics * Test imports are now explicit * getAllFonts NPE handled * Solaris excluded from test * Serb added as a reviewer (commit message) Are we good to go? Thanks, Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi, Martin.> * getAllFonts NPE handled The current version is fine, but note that this null-check is not necessary. Previously it was possible because the loadFonts() method might skip initialization of the instance field "fonts"(which was null by default) as a result NPE could occurred when you try to iterate over "fonts" -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi, Here we have Webrev.03: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.03/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.03.zip Changes: * Test moved to test/jdk/java/awt/FontMetrics * Test imports are now explicit * getAllFonts NPE handled * Solaris excluded from test * Serb added as a reviewer (commit message) Are we good to go? Thanks, Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
On 2/22/19 11:10 AM, Martin Balao wrote:
Hi Phil, Sergey,
Thanks for your feedback.
I'll propose a new Webrev moving the test location as suggested by
Sergey, handling NPE when there are no fonts -test should pass IMO- and
making explicit imports.
Do you want me to exclude Solaris from the test?
I suppose we could do it that way.
Mainly we don't want to push a test that we know fails ..
I was proposing to just exclude point sizes < 3 .. I think there must be
some rounding issues
as we approach the limit of 1 pixel ...
FWIW I checked that it is freetype 2.6.3 installed on that Solaris 11.3
SPARC system which
doesn't seem that different than what I see on RH systems but there are
also a ton of
config options and I don't know how it was built.
-phil.
Kind regards,
Martin.-
On 2/21/19 10:48 PM, Philip Race wrote:
Oh two other things.
1) The change should be pushed to jdk/client not jdk/jdk
2) If you are making changes anyway maybe enumerate the imports rather
than wild-carding..
-phil.
On 2/21/19, 5:47 PM, Philip Race wrote:
I submitted a test job to see what it did on our test platforms :
Solaris, WIndows, Mac as well as Linux
and I got lot of failures on Solaris .. 38 of them .. a sample below
but not just for obscure fonts,
it included Arial (in various styles), Arial Black, Arial Narrow ...
They are all at char width 1 or 2 .. since Arial is also available on
Mac + Windows and didn't
fail there I wonder if it is due to the freetype installed on Solaris ?
Not sure what to do ... exclude the smallest sizes for now ?
-phil.
Testing java.awt.Font[family=AR PL ShanHeiSun Uni,name=AR PL ShanHeiSun
Uni,style=bolditalic,size=1] in FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing java.awt.Font[family=AlArabiya,name=AlArabiya,style=bolditalic,size=1]
in FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing java.awt.Font[family=AlBattar,name=AlBattar,style=bolditalic,size=1] in
FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing java.awt.Font[family=AlHor,name=AlHor,style=bolditalic,size=1] in
FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing
java.awt.Font[family=AlManzomah,name=AlManzomah,style=bolditalic,size=1] in
FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
On 2/21/19, 2:19 PM, Sergey Bylokhov wrote:
Hi, Martin.
What exceptions do you expect below? I think any exception will means
a bug in jdk, no?
try {
GraphicsEnvironment e =
GraphicsEnvironment.getLocalGraphicsEnvironment();
fonts = e.getAllFonts();
} catch (Exception e) {}
Even if you skip an Exception, you will get NPE later at line "103
for (Font f : fonts) {"
BTW probably the test can be moved to the "java/awt/FontMetrics/"
folder.
On 21/02/2019 13:27, Martin Balao wrote:
Hi Phil,
On 2/19/19 10:34 PM, Philip Race wrote:
One more thing about the test, I am not sure why you need to use
sun.font.FontDesignMetrics directly ?
Isn't it enough to create a BufferedImage and get an appropriate
FRC and FM by setting the properties on the graphics for that ?
Yes, that's better, there's no need to use an internal API here.
Webrev.02:
*
http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02/
*
http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02.zip
Are we good to go with Webrev.02?
Thanks,
Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi Phil, Sergey,
Thanks for your feedback.
I'll propose a new Webrev moving the test location as suggested by
Sergey, handling NPE when there are no fonts -test should pass IMO- and
making explicit imports.
Do you want me to exclude Solaris from the test?
Kind regards,
Martin.-
On 2/21/19 10:48 PM, Philip Race wrote:
> Oh two other things.
>
> 1) The change should be pushed to jdk/client not jdk/jdk
> 2) If you are making changes anyway maybe enumerate the imports rather
> than wild-carding..
>
> -phil.
>
> On 2/21/19, 5:47 PM, Philip Race wrote:
>> I submitted a test job to see what it did on our test platforms :
>> Solaris, WIndows, Mac as well as Linux
>> and I got lot of failures on Solaris .. 38 of them .. a sample below
>> but not just for obscure fonts,
>> it included Arial (in various styles), Arial Black, Arial Narrow ...
>>
>> They are all at char width 1 or 2 .. since Arial is also available on
>> Mac + Windows and didn't
>> fail there I wonder if it is due to the freetype installed on Solaris ?
>>
>> Not sure what to do ... exclude the smallest sizes for now ?
>>
>> -phil.
>>
>> Testing java.awt.Font[family=AR PL ShanHeiSun Uni,name=AR PL ShanHeiSun
>> Uni,style=bolditalic,size=1] in FT_LOAD_TARGET_MONO
>> getMaxAdvance: 2
>> Max char width: 1
>> PASSED
>> .
>> Testing
>> java.awt.Font[family=AlArabiya,name=AlArabiya,style=bolditalic,size=1] in
>> FT_LOAD_TARGET_MONO
>> getMaxAdvance: 2
>> Max char width: 1
>> PASSED
>> .
>> Testing java.awt.Font[family=AlBattar,name=AlBattar,style=bolditalic,size=1]
>> in FT_LOAD_TARGET_MONO
>> getMaxAdvance: 2
>> Max char width: 1
>> PASSED
>> .
>> Testing java.awt.Font[family=AlHor,name=AlHor,style=bolditalic,size=1] in
>> FT_LOAD_TARGET_MONO
>> getMaxAdvance: 2
>> Max char width: 1
>> PASSED
>> .
>> Testing
>> java.awt.Font[family=AlManzomah,name=AlManzomah,style=bolditalic,size=1] in
>> FT_LOAD_TARGET_MONO
>> getMaxAdvance: 2
>> Max char width: 1
>> PASSED
>>
>>
>>
>>
>>
>> On 2/21/19, 2:19 PM, Sergey Bylokhov wrote:
>>> Hi, Martin.
>>>
>>> What exceptions do you expect below? I think any exception will means
>>> a bug in jdk, no?
>>>
>>> try {
>>> GraphicsEnvironment e =
>>> GraphicsEnvironment.getLocalGraphicsEnvironment();
>>> fonts = e.getAllFonts();
>>> } catch (Exception e) {}
>>>
>>> Even if you skip an Exception, you will get NPE later at line "103
>>> for (Font f : fonts) {"
>>>
>>> BTW probably the test can be moved to the "java/awt/FontMetrics/"
>>> folder.
>>>
>>> On 21/02/2019 13:27, Martin Balao wrote:
Hi Phil,
On 2/19/19 10:34 PM, Philip Race wrote:
> One more thing about the test, I am not sure why you need to use
> sun.font.FontDesignMetrics directly ?
>
> Isn't it enough to create a BufferedImage and get an appropriate
> FRC and FM by setting the properties on the graphics for that ?
>
Yes, that's better, there's no need to use an internal API here.
Webrev.02:
*
http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02/
*
http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02.zip
Are we good to go with Webrev.02?
Thanks,
Martin.-
>>>
>>>
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Oh two other things.
1) The change should be pushed to jdk/client not jdk/jdk
2) If you are making changes anyway maybe enumerate the imports rather
than wild-carding..
-phil.
On 2/21/19, 5:47 PM, Philip Race wrote:
I submitted a test job to see what it did on our test platforms :
Solaris, WIndows, Mac as well as Linux
and I got lot of failures on Solaris .. 38 of them .. a sample below
but not just for obscure fonts,
it included Arial (in various styles), Arial Black, Arial Narrow ...
They are all at char width 1 or 2 .. since Arial is also available on
Mac + Windows and didn't
fail there I wonder if it is due to the freetype installed on Solaris ?
Not sure what to do ... exclude the smallest sizes for now ?
-phil.
Testing java.awt.Font[family=AR PL ShanHeiSun Uni,name=AR PL ShanHeiSun
Uni,style=bolditalic,size=1] in FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing java.awt.Font[family=AlArabiya,name=AlArabiya,style=bolditalic,size=1]
in FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing java.awt.Font[family=AlBattar,name=AlBattar,style=bolditalic,size=1] in
FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing java.awt.Font[family=AlHor,name=AlHor,style=bolditalic,size=1] in
FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
.
Testing
java.awt.Font[family=AlManzomah,name=AlManzomah,style=bolditalic,size=1] in
FT_LOAD_TARGET_MONO
getMaxAdvance: 2
Max char width: 1
PASSED
On 2/21/19, 2:19 PM, Sergey Bylokhov wrote:
Hi, Martin.
What exceptions do you expect below? I think any exception will means
a bug in jdk, no?
try {
GraphicsEnvironment e =
GraphicsEnvironment.getLocalGraphicsEnvironment();
fonts = e.getAllFonts();
} catch (Exception e) {}
Even if you skip an Exception, you will get NPE later at line "103
for (Font f : fonts) {"
BTW probably the test can be moved to the "java/awt/FontMetrics/"
folder.
On 21/02/2019 13:27, Martin Balao wrote:
Hi Phil,
On 2/19/19 10:34 PM, Philip Race wrote:
One more thing about the test, I am not sure why you need to use
sun.font.FontDesignMetrics directly ?
Isn't it enough to create a BufferedImage and get an appropriate
FRC and FM by setting the properties on the graphics for that ?
Yes, that's better, there's no need to use an internal API here.
Webrev.02:
*
http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02/
*
http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02.zip
Are we good to go with Webrev.02?
Thanks,
Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi, Martin.
What exceptions do you expect below? I think any exception will means a bug in
jdk, no?
try {
GraphicsEnvironment e =
GraphicsEnvironment.getLocalGraphicsEnvironment();
fonts = e.getAllFonts();
} catch (Exception e) {}
Even if you skip an Exception, you will get NPE later at line "103 for (Font f :
fonts) {"
BTW probably the test can be moved to the "java/awt/FontMetrics/" folder.
On 21/02/2019 13:27, Martin Balao wrote:
Hi Phil,
On 2/19/19 10:34 PM, Philip Race wrote:
One more thing about the test, I am not sure why you need to use
sun.font.FontDesignMetrics directly ?
Isn't it enough to create a BufferedImage and get an appropriate
FRC and FM by setting the properties on the graphics for that ?
Yes, that's better, there's no need to use an internal API here.
Webrev.02:
* http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02/
* http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02.zip
Are we good to go with Webrev.02?
Thanks,
Martin.-
--
Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi Phil, On 2/19/19 10:34 PM, Philip Race wrote: > One more thing about the test, I am not sure why you need to use > sun.font.FontDesignMetrics directly ? > > Isn't it enough to create a BufferedImage and get an appropriate > FRC and FM by setting the properties on the graphics for that ? > Yes, that's better, there's no need to use an internal API here. Webrev.02: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.02.zip Are we good to go with Webrev.02? Thanks, Martin.-
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Sorry .. missed it. One more thing about the test, I am not sure why you need to use sun.font.FontDesignMetrics directly ? Isn't it enough to create a BufferedImage and get an appropriate FRC and FM by setting the properties on the graphics for that ? -phil. On 2/19/19, 10:48 AM, Martin Balao wrote: Hi Phil, Are we good to go with Webrev.01? Thanks, Martin.- On 2/14/19 3:47 PM, Martin Balao wrote: Hi Phil, Thanks for your feedback. I proposed the font in the test case because it's licensed under GNU Affero General Public License, Version 3 (AGPL) with an exemption [2]. My understanding was that we were able to ship it. Anyways, I removed the font and use system installed fonts. On newer Linux distributions, Z003 font should be available anyways. Comment added explaining that bold value comes from freetype 2.6 and is valid at least up to 2.9.1. I was not sure if applying the OBLIQUE_MODIFIER changes as part of this ticket. Done. Line lengths adjusted to not exceed 80 chars. freetypeScaler.c copyright date updated. Webrev 01: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.01/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.01.zip Are we good to go? Kind regards, Martin.- -- [1] - https://github.com/URWTypeFoundry/Core_35/blob/master/COPYING [2] - https://github.com/URWTypeFoundry/Core_35/blob/master/LICENSE
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi Phil, Thanks for your feedback. I proposed the font in the test case because it's licensed under GNU Affero General Public License, Version 3 (AGPL) with an exemption [2]. My understanding was that we were able to ship it. Anyways, I removed the font and use system installed fonts. On newer Linux distributions, Z003 font should be available anyways. Comment added explaining that bold value comes from freetype 2.6 and is valid at least up to 2.9.1. I was not sure if applying the OBLIQUE_MODIFIER changes as part of this ticket. Done. Line lengths adjusted to not exceed 80 chars. freetypeScaler.c copyright date updated. Webrev 01: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.01/ * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.01.zip Are we good to go? Kind regards, Martin.- -- [1] - https://github.com/URWTypeFoundry/Core_35/blob/master/COPYING [2] - https://github.com/URWTypeFoundry/Core_35/blob/master/LICENSE
Re: [OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Unfortunately we can't check in the font. That would require us to obtain a 3rd party code license and to update the font regularly as new versions are created. Way too onerous. Perhaps instead you can just run through installed fonts ? Can you also add a comment that explains that the values you are using match those in freetype 2.6 -> current (2.9.1) ... And consequently I think you should update the values used by OBLIQUE_MODIFIER at the same time to match current freetype. The current value is from the ancient 2.3 release. Include a comment there too .. Also can you check you are keeping line lengths including comment lines < 80 chars where possible ? -phil. On 2/12/19, 2:40 PM, Martin Balao wrote: Hi, I'd like to propose the following fix for "JDK-8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth" [1]: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.00/ Can I have a review? Thanks, Martin.- -- [1] - https://bugs.openjdk.java.net/browse/JDK-8218854
[OpenJDK 2D-Dev] RFR 8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth
Hi, I'd like to propose the following fix for "JDK-8218854: FontMetrics.getMaxAdvance may be less than the maximum FontMetrics.charWidth" [1]: * http://cr.openjdk.java.net/~mbalao/webrevs/8218854/8218854.webrev.00/ Can I have a review? Thanks, Martin.- -- [1] - https://bugs.openjdk.java.net/browse/JDK-8218854
