Re: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
This looks fine to me. Please add this info to JBS when you get a
chance, and also link the new bug to this bug.
The fix looks fine to me, although I was puzzled by the following:
-return
-System.getProperty("java.home","") + File.separator +
-"lib" + File.separator + "fonts" + File.separator;
+return System.getProperty("java.home","") + File.separator +
+"lib" + File.separator + "fonts";
Your fix preserves existing behavior, given that the return value from
the now-eliminated call to the JDKFontLookup class did not have the
trailing File.separator, but I think that might be a bug given how the
returned string from this method is used elsewhere in this file. Since
this is preexisting, you might choose to address that as well in the
follow-up bug (JDK-8198752). If so, please add a comment to that bug.
+1 from me (you need a +1 from Phil as well)
-- Kevin
Ajit Ghaisas wrote:
Hi Phil,
All my webrev does is to replace the way of obtaining name of JDK
font directory – and as rightly pointed by you below (first two lines
of your last reply) – it is what is needed to get rid of dependency on
sun.font.lookup.
I have tested it by running an OpenJDK build that doesn't contain a
lib/fonts directory using Ensemble8 sample.
I got your point about Lucida Sans fonts being pointless with
openJDK & the font finding fallback code is being little dubious.
I would like to address that separately. I have filed bug
JDK-8198752 <https://bugs.openjdk.java.net/browse/JDK-8198752> to
handle that.
Regards,
Ajit
*From:* Phil Race
*Sent:* Thursday, February 15, 2018 10:44 PM
*To:* Ajit Ghaisas
*Cc:* Kevin Rushforth; openjfx-dev@openjdk.java.net
*Subject:* Re: [11] Review request : JDK-8195806 : Eliminate
dependency on sun.font.lookup in javafx.graphics
This part is probably as good as you can do
+return System.getProperty("java.home","") + File.separator +
+"lib" + File.separator + "fonts";
but if we want to support running against OpenJDK which does not have
Lucida Sans
then we need to look at the caller of getJDKFontDir()
So going forward all of this ..
private static final String jreDefaultFont = "Lucida Sans Regular";
private static final String jreDefaultFontLC = "lucida sans regular";
private static final String jreDefaultFontFile = "LucidaSansRegular.ttf";
.. is probably pointless.
And if we can't find it then the first layer of fall back code is a
bit dubious
// Normally use the JRE default font as the last fallback.
// If we can't find even that, use any platform font;
for (String font : fontToFileMap.keySet()) {
String file = findFile(font); // gets full path
fontResource = createFontResource(jreDefaultFontLC, file);
if (fontResource != null) {
break;
}
It did not really matter in the past .. it was just to prevent NPE in an unlikely scenario ..
But if it is to be the first class way of finding this font it probably should
be using
"System" instead ? But then you need to make sure we don't have a circularity
in the case
that we are using this in initialiasing System.
-phil.
On 02/14/2018 09:39 PM, Ajit Ghaisas wrote:
Hi Phil,
Thanks for having a look at the changes.
I have little difficulty in understanding your question.
Are you referring to the sun.font.SunFontManager initialization?
I am asking as it is not evident from the code changes that I have done
as part of this webrev.
Request your guidance in this regard.
Regards,
Ajit
-Original Message-
From: Philip Race
Sent: Wednesday, February 14, 2018 12:52 PM
To: Ajit Ghaisas
Cc: Kevin Rushforth; openjfx-dev@openjdk.java.net
<mailto:openjfx-dev@openjdk.java.net>
Subject: Re: [11] Review request : JDK-8195806 : Eliminate dependency on
sun.font.lookup in javafx.graphics
Well that removes the dependency but how are you fixing the problem of how
else to find the font ?
There needs to be alternate code or you need to go back to see what would
happen if some code tried to locate that font and how it would fail.
-phil.
On 2/13/18, 11:11 PM, Ajit Ghaisas wrote:
Hi Kevin, Phil,
Request you to review following fix :
Issue : https://bugs.openjdk.java.net/browse/JDK-8195806
Fix :
http://cr.openjdk.java.net/~aghaisas/fx/8195806/webrev.0/
<http://cr.openjdk.java.net/%7Eaghaisas/fx/8195806/webrev.0/>
Regards,
Ajit
RE: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
Hi Phil,
All my webrev does is to replace the way of obtaining name of JDK font
directory – and as rightly pointed by you below (first two lines of your last
reply) – it is what is needed to get rid of dependency on sun.font.lookup.
I have tested it by running an OpenJDK build that doesn't contain a
lib/fonts directory using Ensemble8 sample.
I got your point about Lucida Sans fonts being pointless with openJDK & the
font finding fallback code is being little dubious.
I would like to address that separately. I have filed bug HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8198752"JDK-8198752 to handle that.
Regards,
Ajit
From: Phil Race
Sent: Thursday, February 15, 2018 10:44 PM
To: Ajit Ghaisas
Cc: Kevin Rushforth; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195806 : Eliminate dependency on
sun.font.lookup in javafx.graphics
This part is probably as good as you can do
+ return System.getProperty("java.home","") + File.separator +
+ "lib" + File.separator + "fonts";
but if we want to support running against OpenJDK which does not have Lucida
Sans
then we need to look at the caller of getJDKFontDir()
So going forward all of this ..
private static final String jreDefaultFont = "Lucida Sans Regular";
private static final String jreDefaultFontLC = "lucida sans regular";
private static final String jreDefaultFontFile = "LucidaSansRegular.ttf";
.. is probably pointless.
And if we can't find it then the first layer of fall back code is a bit dubious
// Normally use the JRE default font as the last fallback.
// If we can't find even that, use any platform font;
for (String font : fontToFileMap.keySet()) {
String file = findFile(font); // gets full path
fontResource = createFontResource(jreDefaultFontLC, file);
if (fontResource != null) {
break;
}
It did not really matter in the past .. it was just to prevent NPE in an
unlikely scenario ..
But if it is to be the first class way of finding this font it probably should
be using
"System" instead ? But then you need to make sure we don't have a circularity
in the case
that we are using this in initialiasing System.
-phil.
On 02/14/2018 09:39 PM, Ajit Ghaisas wrote:
Hi Phil,
Thanks for having a look at the changes.
I have little difficulty in understanding your question.
Are you referring to the sun.font.SunFontManager initialization?
I am asking as it is not evident from the code changes that I have done as
part of this webrev.
Request your guidance in this regard.
Regards,
Ajit
-Original Message-
From: Philip Race
Sent: Wednesday, February 14, 2018 12:52 PM
To: Ajit Ghaisas
Cc: Kevin Rushforth; HYPERLINK
"mailto:openjfx-dev@openjdk.java.net"openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195806 : Eliminate dependency on
sun.font.lookup in javafx.graphics
Well that removes the dependency but how are you fixing the problem of how else
to find the font ?
There needs to be alternate code or you need to go back to see what would
happen if some code tried to locate that font and how it would fail.
-phil.
On 2/13/18, 11:11 PM, Ajit Ghaisas wrote:
Hi Kevin, Phil,
Request you to review following fix :
Issue : https://bugs.openjdk.java.net/browse/JDK-8195806
Fix :
http://cr.openjdk.java.net/~aghaisas/fx/8195806/webrev.0/
Regards,
Ajit
Re: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
This part is probably as good as you can do
+ return System.getProperty("java.home","") + File.separator +
+ "lib" + File.separator + "fonts";
but if we want to support running against OpenJDK which does not have
Lucida Sans
then we need to look at the caller of getJDKFontDir()
So going forward all of this ..
private static final String jreDefaultFont = "Lucida Sans Regular";
private static final String jreDefaultFontLC = "lucida sans regular";
private static final String jreDefaultFontFile = "LucidaSansRegular.ttf";
.. is probably pointless.
And if we can't find it then the first layer of fall back code is a bit
dubious
// Normally use the JRE default font as the last fallback.
// If we can't find even that, use any platform font;
for (String font : fontToFileMap.keySet()) {
String file = findFile(font); // gets full path
fontResource = createFontResource(jreDefaultFontLC, file);
if (fontResource != null) {
break;
}
It did not really matter in the past .. it was just to prevent NPE in an
unlikely scenario ..
But if it is to be the first class way of finding this font it probably should
be using
"System" instead ? But then you need to make sure we don't have a circularity
in the case
that we are using this in initialiasing System.
-phil.
On 02/14/2018 09:39 PM, Ajit Ghaisas wrote:
Hi Phil,
Thanks for having a look at the changes.
I have little difficulty in understanding your question.
Are you referring to the sun.font.SunFontManager initialization?
I am asking as it is not evident from the code changes that I have done as
part of this webrev.
Request your guidance in this regard.
Regards,
Ajit
-Original Message-
From: Philip Race
Sent: Wednesday, February 14, 2018 12:52 PM
To: Ajit Ghaisas
Cc: Kevin Rushforth; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195806 : Eliminate dependency on
sun.font.lookup in javafx.graphics
Well that removes the dependency but how are you fixing the problem of how else
to find the font ?
There needs to be alternate code or you need to go back to see what would
happen if some code tried to locate that font and how it would fail.
-phil.
On 2/13/18, 11:11 PM, Ajit Ghaisas wrote:
Hi Kevin, Phil,
Request you to review following fix :
Issue : https://bugs.openjdk.java.net/browse/JDK-8195806
Fix :
http://cr.openjdk.java.net/~aghaisas/fx/8195806/webrev.0/
Regards,
Ajit
RE: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
Hi Phil, Thanks for having a look at the changes. I have little difficulty in understanding your question. Are you referring to the sun.font.SunFontManager initialization? I am asking as it is not evident from the code changes that I have done as part of this webrev. Request your guidance in this regard. Regards, Ajit -Original Message- From: Philip Race Sent: Wednesday, February 14, 2018 12:52 PM To: Ajit Ghaisas Cc: Kevin Rushforth; openjfx-dev@openjdk.java.net Subject: Re: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics Well that removes the dependency but how are you fixing the problem of how else to find the font ? There needs to be alternate code or you need to go back to see what would happen if some code tried to locate that font and how it would fail. -phil. On 2/13/18, 11:11 PM, Ajit Ghaisas wrote: > Hi Kevin, Phil, > >Request you to review following fix : > >Issue : https://bugs.openjdk.java.net/browse/JDK-8195806 > >Fix : > http://cr.openjdk.java.net/~aghaisas/fx/8195806/webrev.0/ > > Regards, > Ajit
Re: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
Well that removes the dependency but how are you fixing the problem of how else to find the font ? There needs to be alternate code or you need to go back to see what would happen if some code tried to locate that font and how it would fail. -phil. On 2/13/18, 11:11 PM, Ajit Ghaisas wrote: Hi Kevin, Phil, Request you to review following fix : Issue : https://bugs.openjdk.java.net/browse/JDK-8195806 Fix : http://cr.openjdk.java.net/~aghaisas/fx/8195806/webrev.0/ Regards, Ajit
