[9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Prahalad kumar Narayanan
The change looks good. +1.

Minor observation: The copyright should be updated to 2017 in the CFont.java file.
If you use any script to update the same, then ignore this observation.

Thanks
Have a good day

Prahalad N.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]]
Sent: Friday, February 10, 2017 3:37 AM
To: [hidden email]
Subject: 2d-dev Digest, Vol 117, Issue 3

Send 2d-dev mailing list submissions to
        [hidden email]

To subscribe or unsubscribe via the World Wide Web, visit
        http://mail.openjdk.java.net/mailman/listinfo/2d-dev
or, via email, send a message with subject or body 'help' to
        [hidden email]

You can reach the person managing the list at
        [hidden email]

When replying, please edit your Subject line so it is more specific than "Re: Contents of 2d-dev digest..."


Today's Topics:

   1.  [9] RFR JDK-8147002:[macosx] Arabic character cannot be
      rendered on MacOS X (Prasanta Sadhukhan)
   2.  RFR: 8172967: [macosx] Exception while working with layout
      for text containing unmappable character (Philip Race)
   3. Re:  Review Request for JDK-7107905: ColorModel subclasses
      are missing hashCode() or equals() or both methods (Jim Graham)
   4. Re:  Review Request for JDK-7107905: ColorModel subclasses
      are missing hashCode() or equals() or both methods (Phil Race)


----------------------------------------------------------------------

Message: 1
Date: Thu, 9 Feb 2017 14:38:11 +0530
From: Prasanta Sadhukhan <[hidden email]>
To: 2d-dev <[hidden email]>, Philip Race
        <[hidden email]>
Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
        character cannot be rendered on MacOS X
Message-ID: <[hidden email]>
Content-Type: text/plain; charset="utf-8"; Format="flowed"

Hi All,

Please review a fix for an issue which causes arabic character "alef" to be not rendered in osx for menlo font in italic style.

Bug: https://bugs.openjdk.java.net/browse/JDK-8147002

The issue was actually a regression caused by the fix to JDK-7162125:
[macosx] A font has different behaviour for ligatures depending on its creation mode in which we have added cascaded font list to find the real fonts that CFont uses, so that there is no need to use "negative" glyph code for finding the fallback fonts using the "subsititution"/"fallback" mechanism used by osx code.

However, the above logic of using cascaded font list in CFont does not take into account of using JRE provided fonts like all those Lucida* ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef' character) is intended to be rendered in Menlo font in italic style, osx will not be able to find the glyph in Menlo-Italic font and neither in all the cascaded system fonts provided by CoreText, so it results in empty box.

Before 7162125 fix, the fallback code in
CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
JRSFontCreateFallbackFontForCharacters()
was adding jre/lib/fonts to the fallback list which was causing the glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was rendered.

So, the proposed fix is to add jre provided font "Lucida Sans Regular"
to the cascaded list so that we get the "alef" glyph.
The reason for choosing "Lucida Sans Regular" over "Lucida Bright Regular" is, because it is the largest font file in jre and has all the glyph codepoints that no other font in the jre has, so we will not lose out on any codepoints and will help us in not getting missing glyph.

webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/

Regards
Prasanta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/4ad48b17/attachment-0001.html>

------------------------------

Message: 2
Date: Thu, 09 Feb 2017 11:30:26 -0800
From: Philip Race <[hidden email]>
To: 2d-dev <[hidden email]>
Subject: [OpenJDK 2D-Dev] RFR: 8172967: [macosx] Exception while
        working with layout for text containing unmappable character
Message-ID: <[hidden email]>
Content-Type: text/plain; charset=UTF-8; format=flowed


http://cr.openjdk.java.net/~prr/8172967/
https://bugs.openjdk.java.net/browse/JDK-8172967

Full evaluation in the bug report.
Short summary: avoid AIOB and NPE when Mac glyph mapper returns a negated unicode which is misinterpreted as having composite font slot 255

-phil.


------------------------------

Message: 3
Date: Thu, 9 Feb 2017 13:59:37 -0800
From: Jim Graham <[hidden email]>
To: Jayathirth D V <[hidden email]>, Philip Race
        <[hidden email]>, [hidden email]
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
        ColorModel subclasses are missing hashCode() or equals() or both
        methods
Message-ID: <[hidden email]>
Content-Type: text/plain; charset=windows-1252; format=flowed

 From my end this looks good.  +1 except for 2 outstanding review issues:

- Would like to hear back final comments from Joe Darcy on the new doc changes/CCC request
- Phil pointed out that there is an unneeded import in some of the files.  I agree that we should make a final webrev to delete them, but I don't need to approve it if that is the only change...

                        ...jim

On 2/8/17 11:56 PM, Jayathirth D V wrote:

> Hello All,
>
> There was a closed test which was failing because of identity-as-equals approach for ColorModel equals() method.
> I have modified it and added in the webrev. Along with this we are now using colorspace.hashCode() in hashCode() functions instead of Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in IndexColorModel equals() method because Arrays.copyOf() takes lot of time.
>
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>
> Ran jtreg test and JCK there are no additional test case failures because of the above change. Only 4 JCK tests are failing as it was happening previously.
>
> Just copy pasted my observation regarding JCK failures so that we can trace it easily:
>
> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
> cases: 4; passed: 3; failed: 1; first test case failure:
> ColorModel2001
>
> This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In the test case they have bits per component array of length 4 like {8, 8, 8, 8}. But in the test case wherever they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check that we have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>
> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases:
> 3; passed: 2; failed: 1; first test case failure: ColorModel0004
>
> Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>
> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
> cases: 2; passed: 1; failed: 1; first test case failure:
> ColorModel2006
>
> Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we don't have hashCode check in ColorModel and it will be different between 2 Objects.
>
> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttes
> tCase1: Failed. test cases: 2; passed: 1; failed: 1; first test case
> failure: testCase1
>
> Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the first JCK test is failing. We omit alpha bit if hasAlpha is false but JCK test tries to call getComponentSize() with index 3 which throws ArrayIndexOutOfBoundsException.
>
> Thanks,
> Jay
> -----Original Message-----
> From: Jayathirth D V
> Sent: Wednesday, February 08, 2017 3:41 PM
> To: Jim Graham; Philip Race; [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
>
> Hello All,
>
> I have updated the webrev to include the following changes.
>
> 1) Have identity as equals check in equals() method of ColorModel but elaborate the specification of equals() and hashCode() in ColorModel on what properties to     check in subclasses of ColorModel.
> 2) Made changes to test case to have single helper method wherever we have same equals/hashCode() check.
> 3) Updated IndexColorModel equals() method to use Arrays.equals() for rgb[] data.
> 4) Add comment on why we are not using validBits to calculate hashCode() in IndexColorModel hashCode() method.
>
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, February 02, 2017 2:51 AM
> To: Phil Race; Jayathirth D V; [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
>
> I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix.
> I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...
>
> ...jim
>
> On 1/31/17 2:34 PM, Jim Graham wrote:
>> For an application to run into this same issue they'd have to expect
>> getCompSizes() to return data for components that don't exist.  It's
>> unlikely they would use that data if they really understand the
>> objects.  While that would be odd, I guess I can see someone might be
>> constructing all of their CM's from an array of 4 components
>> regardless of the number of actual components and we'd be happily
>> remembering the useless extra components and returning an array of 4
>> from getCompSizes().  As I said, they shouldn't really be reading and
>> interpreting those extra components for any image processing, but I can imagine that they might do something like create a variant CM by calling the CompSizes() and copying them into a new array to construct a new CM with modifications.  They might just robotically always copy 4 values without really checking how many are valid.  That's a stretch, and their code is weak.  I can conceive of how this might happen, but I really have no idea how likely it is...
>>
>>             ...jim
>>
>> On 1/30/17 3:56 PM, Phil Race wrote:
>>> Sounds like we should at least try to get the tests updated so they only test what the spec. says.
>>> Although it does indicate that there is at least a chance that
>>> application code might also fail due to similar assumptions.
>>> Does #1 not fail with the previous iteration of this change too ?
>>>
>>> -phil.
>>>
>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>> Hmmm.  Sounds like the test cases were written based on bugs in the
>>>> implementation.  I'm not sure what the best tactic is here for the
>>>> short term for getting this in, but many of these changes should eventually be considered bugs in the tests.  Is it acceptable to break API tests like this at the last minute even if the tests are at fault?  Phil?
>>>>
>>>> Notes on specific instances below...
>>>>
>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>> Hi Phil,
>>>>>
>>>>>     1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>> ColorModel2001
>>>>>
>>>>>     This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In
>>>>> the test case they have bits per component array     of length 4 like {8, 8, 8, 8}. But in the test case wherever
>>>>> they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check     that we
>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>
>>>> This is a bug in the test then, especially if the size of our array matches the return value of getNumComponents.
>>>>
>>>>>     2)api/java_awt/Image/ColorModel/index.html#Equals: Failed.
>>>>> test
>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>> failure: ColorModel0004
>>>>>
>>>>>     Here they check for equality between 2 ColorModel objects
>>>>> having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>>>
>>>> How do they accomplish this when the CM class is abstract?  Do they
>>>> create a relatively empty subclass and instantiate that?
>>>>
>>>> The documentation for the equals() method does not document the
>>>> conditions under which it returns true, it uses a vague concept of "equals this ColorModel".  I don't see how they could test anything given that documentation.
>>>>
>>>>>     3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>> failure: ColorModel2006
>>>>>
>>>>>     Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we
>>>>> don't have hashCode check in ColorModel and it     will be different between 2 Objects.
>>>>
>>>> Same as above, there are no promises documented.
>>>>
>>>>>
>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTes
>>>>> t
>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>> failed: 1; first test case failure: testCase1
>>>>>
>>>>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the
>>>>> first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK test tries to call getComponentSize()
>>>>> with index 3 which throws ArrayIndexOutOfBoundsException.
>>>>
>>>> Same assessment as #1 above...
>>>>
>>>> Again, while these are my recommendations about the correctness of
>>>> these tests, the question remains whether we want to introduce a
>>>> change at this point in the release cycle that will essentially invalidate a number of tests that have been working for several releases already.  I'll leave that tactic issue to Phil...
>>>>
>>>>                 ...jim
>>>>
>>>


------------------------------

Message: 4
Date: Thu, 9 Feb 2017 14:05:24 -0800
From: Phil Race <[hidden email]>
To: Jim Graham <[hidden email]>, Jayathirth D V
        <[hidden email]>, [hidden email]
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
        ColorModel subclasses are missing hashCode() or equals() or both
        methods
Message-ID: <[hidden email]>
Content-Type: text/plain; charset="windows-1252"; Format="flowed"

Oh .. my reply was to an off-list email. I did not notice that.
So I should repeat that here :

On 2/9/17 12:38 PM, Phil Race wrote:
> 32 import java.util.Objects;
>
> This is now un-used, isn't it ? Yet all 3 subclasses still have this
> import.
>
> I don't need to "approve" a new webrev containing that but it would be
> good to publish one.
>
> +1

-phil.


On 02/09/2017 01:59 PM, Jim Graham wrote:

> From my end this looks good.  +1 except for 2 outstanding review issues:
>
> - Would like to hear back final comments from Joe Darcy on the new doc
> changes/CCC request
> - Phil pointed out that there is an unneeded import in some of the
> files.  I agree that we should make a final webrev to delete them, but
> I don't need to approve it if that is the only change...
>
>             ...jim
>
> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>> Hello All,
>>
>> There was a closed test which was failing because of
>> identity-as-equals approach for ColorModel equals() method.
>> I have modified it and added in the webrev. Along with this we are
>> now using colorspace.hashCode() in hashCode() functions instead of
>> Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in
>> IndexColorModel equals() method because Arrays.copyOf() takes lot of
>> time.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>
>> Ran jtreg test and JCK there are no additional test case failures
>> because of the above change. Only 4 JCK tests are failing as it was
>> happening previously.
>>
>> Just copy pasted my observation regarding JCK failures so that we can
>> trace it easily:
>>
>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
>> cases: 4; passed: 3; failed: 1; first test case failure:
>> ColorModel2001
>>
>>     This test fails because getComponentSize() returned an array with
>> length 3 but it expects the length to be 4. In the test case they
>> have bits per component array     of length 4 like {8, 8, 8, 8}. But
>> in the test case wherever they are passing "has Alpha" as "false" we
>> omit the alpha component bit. This is because of tighter check    
>> that we have in ColorModel class as "nBits = Arrays.copyOf(bits,
>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>
>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>> cases: 3; passed: 2; failed: 1; first test case failure:
>> ColorModel0004
>>
>>     Here they check for equality between 2 ColorModel objects having
>> same values, but it fails because now we are using identity-as-equals
>> check in ColorModel.
>>
>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
>> cases: 2; passed: 1; failed: 1; first test case failure:
>> ColorModel2006
>>
>>     Here they check for hashCode equality between 2 ColorModel
>> objects having same values, but it fails since we don't have hashCode
>> check in ColorModel and it     will be different between 2 Objects.
>>
>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1:
>> Failed. test cases: 2; passed: 1; failed: 1; first test case failure:
>> testCase1
>>
>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>> also happening because of same reason as why the first JCK test is
>> failing. We omit alpha bit if     hasAlpha is false but JCK test
>> tries to call getComponentSize() with index 3 which throws
>> ArrayIndexOutOfBoundsException.
>>
>> Thanks,
>> Jay
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Wednesday, February 08, 2017 3:41 PM
>> To: Jim Graham; Philip Race; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> Hello All,
>>
>> I have updated the webrev to include the following changes.
>>
>>     1) Have identity as equals check in equals() method of ColorModel
>> but elaborate the specification of equals() and hashCode() in
>> ColorModel on what properties to          check in subclasses of
>> ColorModel.
>>     2) Made changes to test case to have single helper method
>> wherever we have same equals/hashCode() check.
>>     3) Updated IndexColorModel equals() method to use Arrays.equals()
>> for rgb[] data.
>>     4) Add comment on why we are not using validBits to calculate
>> hashCode() in IndexColorModel hashCode() method.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Thursday, February 02, 2017 2:51 AM
>> To: Phil Race; Jayathirth D V; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> I think we should move this issue (array size returned from
>> getCompSizes) into a separate bug entry and a separate fix.
>> I don't think we need to fix the clone() in the constructor and the
>> getter just to get hashcode/equals right...
>>
>>             ...jim
>>
>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>> For an application to run into this same issue they'd have to expect
>>> getCompSizes() to return data for components that don't exist.  It's
>>> unlikely they would use that data if they really understand the
>>> objects.  While that would be odd, I guess I can see someone might
>>> be constructing all of their CM's from an array of 4 components
>>> regardless of the number of actual components and we'd be happily
>>> remembering the useless extra components and returning an array of 4
>>> from getCompSizes().  As I said, they shouldn't really be reading
>>> and interpreting those extra components for any image processing,
>>> but I can imagine that they might do something like create a variant
>>> CM by calling the CompSizes() and copying them into a new array to
>>> construct a new CM with modifications.  They might just robotically
>>> always copy 4 values without really checking how many are valid.
>>> That's a stretch, and their code is weak.  I can conceive of how
>>> this might happen, but I really have no idea how likely it is...
>>>
>>>             ...jim
>>>
>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>> Sounds like we should at least try to get the tests updated so they
>>>> only test what the spec. says.
>>>> Although it does indicate that there is at least a chance that
>>>> application code might also fail due to similar assumptions.
>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>
>>>> -phil.
>>>>
>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>> Hmmm.  Sounds like the test cases were written based on bugs in
>>>>> the implementation.  I'm not sure what the best tactic is here for
>>>>> the short term for getting this in, but many of these changes
>>>>> should eventually be considered bugs in the tests.  Is it
>>>>> acceptable to break API tests like this at the last minute even if
>>>>> the tests are at fault?  Phil?
>>>>>
>>>>> Notes on specific instances below...
>>>>>
>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>> Hi Phil,
>>>>>>
>>>>>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>>> ColorModel2001
>>>>>>
>>>>>>     This test fails because getComponentSize() returned an array
>>>>>> with length 3 but it expects the length to be 4. In
>>>>>> the test case they have bits per component array     of length 4
>>>>>> like {8, 8, 8, 8}. But in the test case wherever they are passing
>>>>>> "has Alpha" as "false" we omit the alpha
>>>>>> component bit. This is because of tighter check     that we
>>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>>
>>>>> This is a bug in the test then, especially if the size of our
>>>>> array matches the return value of getNumComponents.
>>>>>
>>>>>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>>> failure: ColorModel0004
>>>>>>
>>>>>>     Here they check for equality between 2 ColorModel objects
>>>>>> having same values, but it fails because now we are using
>>>>>> identity-as-equals check in ColorModel.
>>>>>
>>>>> How do they accomplish this when the CM class is abstract?  Do
>>>>> they create a relatively empty subclass and instantiate that?
>>>>>
>>>>> The documentation for the equals() method does not document the
>>>>> conditions under which it returns true, it uses a vague concept of
>>>>> "equals this ColorModel".  I don't see how they could test
>>>>> anything given that documentation.
>>>>>
>>>>>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>>> failure: ColorModel2006
>>>>>>
>>>>>>     Here they check for hashCode equality between 2 ColorModel
>>>>>> objects having same values, but it fails since we
>>>>>> don't have hashCode check in ColorModel and it     will be
>>>>>> different between 2 Objects.
>>>>>
>>>>> Same as above, there are no promises documented.
>>>>>
>>>>>>
>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTe
>>>>>> st
>>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>>> failed: 1; first test case failure: testCase1
>>>>>>
>>>>>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>>>>>> also happening because of same reason as why the first JCK test
>>>>>> is failing. We omit alpha bit if hasAlpha is false but JCK test
>>>>>> tries to call getComponentSize() with index 3 which throws
>>>>>> ArrayIndexOutOfBoundsException.
>>>>>
>>>>> Same assessment as #1 above...
>>>>>
>>>>> Again, while these are my recommendations about the correctness of
>>>>> these tests, the question remains whether we want to introduce a
>>>>> change at this point in the release cycle that will essentially
>>>>> invalidate a number of tests that have been working for several
>>>>> releases already.  I'll leave that tactic issue to Phil...
>>>>>
>>>>>                 ...jim
>>>>>
>>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/c71a98ce/attachment.html>

End of 2d-dev Digest, Vol 117, Issue 3
**************************************
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Prahalad kumar Narayanan
Hello Phil

A very good feedback:
> I am OK with this if you can confirm that removing Lucida Sans Regular from the JDK does not cause any nasty exceptions.

I believe, removing Lucida Sans Regular from JDK won't cause any trouble.

Reason is that- FontManager.findFont2D should return NULL in this case. The logic that follows this call in CFont.java checks if number of physical fonts is lesser than expected count and updates the list of fonts accordingly.
 234         if (idx < fonts.length) {
 235             PhysicalFont[] orig = fonts;
 236             fonts = new PhysicalFont[idx];
 237             System.arraycopy(orig, 0, fonts, 0, idx);
 238         }

A quick look at SunFontManager.findFont2D : The definition invokes findJREDeferredFont method to first search for an entry in jreFontMap and then initialize the Lucida Sans font. If the initialization succeeds a valid physical font is returned. Upon failure, it should return NULL.

Thanks
Have a good day

Prahalad N.

----------------------------------------------------------------------
Message: 1
Date: Thu, 09 Feb 2017 19:09:00 -0800
From: Philip Race <[hidden email]>
To: Prasanta Sadhukhan <[hidden email]>
Cc: 2d-dev <[hidden email]>
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
        character cannot be rendered on MacOS X
Message-ID: <[hidden email]>
Content-Type: text/plain; charset="utf-8"; Format="flowed"

I am OK with this if you can confirm that removing Lucida Sans Regular from the JDK does not cause any nasty exceptions.

Apple use the PostScript name but our lookup code should work for the TrueType name.

So overall I think this should be fine - from visual inspection - and assuming it has been tested :-)

+1

-phil.

On 2/9/17, 1:08 AM, Prasanta Sadhukhan wrote:

>
> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef"
> to be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its
> creation mode in which we have added cascaded font list to find the
> real fonts that CFont uses, so that there is no need to use "negative"
> glyph code for finding the fallback fonts using the
> "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not
> take into account of using JRE provided fonts like all those Lucida*
> ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef'
> character) is intended to be rendered in Menlo font in italic style,
> osx will not be able to find the glyph in Menlo-Italic font and
> neither in all the cascaded system fonts provided by CoreText, so it
> results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode()
> uses JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the
> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> Regular" is, because it is the largest font file in jre and has all
> the glyph codepoints that no other font in the jre has, so we will not
> lose out on any codepoints and will help us in not getting missing
> glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/
> <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>
> Regards
> Prasanta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/37baa90a/attachment-0001.html>

------------------------------

Message: 2
Date: Thu, 9 Feb 2017 19:34:34 -0800 (PST)
From: Prahalad Kumar Narayanan <[hidden email]>
To: [hidden email]
Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
        character cannot be rendered on MacOS X
Message-ID: <98e3b75f-b2e5-41a4-9489-62ec34b05985@default>
Content-Type: text/plain; charset=us-ascii

The change looks good. +1.

Minor observation: The copyright should be updated to 2017 in the CFont.java file.
If you use any script to update the same, then ignore this observation.

Thanks
Have a good day

Prahalad N.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]]
Sent: Friday, February 10, 2017 3:37 AM
To: [hidden email]
Subject: 2d-dev Digest, Vol 117, Issue 3

Send 2d-dev mailing list submissions to
        [hidden email]

To subscribe or unsubscribe via the World Wide Web, visit
        http://mail.openjdk.java.net/mailman/listinfo/2d-dev
or, via email, send a message with subject or body 'help' to
        [hidden email]

You can reach the person managing the list at
        [hidden email]

When replying, please edit your Subject line so it is more specific than "Re: Contents of 2d-dev digest..."


Today's Topics:

   1.  [9] RFR JDK-8147002:[macosx] Arabic character cannot be
      rendered on MacOS X (Prasanta Sadhukhan)
   2.  RFR: 8172967: [macosx] Exception while working with layout
      for text containing unmappable character (Philip Race)
   3. Re:  Review Request for JDK-7107905: ColorModel subclasses
      are missing hashCode() or equals() or both methods (Jim Graham)
   4. Re:  Review Request for JDK-7107905: ColorModel subclasses
      are missing hashCode() or equals() or both methods (Phil Race)


----------------------------------------------------------------------

Message: 1
Date: Thu, 9 Feb 2017 14:38:11 +0530
From: Prasanta Sadhukhan <[hidden email]>
To: 2d-dev <[hidden email]>, Philip Race
        <[hidden email]>
Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
        character cannot be rendered on MacOS X
Message-ID: <[hidden email]>
Content-Type: text/plain; charset="utf-8"; Format="flowed"

Hi All,

Please review a fix for an issue which causes arabic character "alef" to be not rendered in osx for menlo font in italic style.

Bug: https://bugs.openjdk.java.net/browse/JDK-8147002

The issue was actually a regression caused by the fix to JDK-7162125:
[macosx] A font has different behaviour for ligatures depending on its creation mode in which we have added cascaded font list to find the real fonts that CFont uses, so that there is no need to use "negative" glyph code for finding the fallback fonts using the "subsititution"/"fallback" mechanism used by osx code.

However, the above logic of using cascaded font list in CFont does not take into account of using JRE provided fonts like all those Lucida* ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef' character) is intended to be rendered in Menlo font in italic style, osx will not be able to find the glyph in Menlo-Italic font and neither in all the cascaded system fonts provided by CoreText, so it results in empty box.

Before 7162125 fix, the fallback code in
CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
JRSFontCreateFallbackFontForCharacters()
was adding jre/lib/fonts to the fallback list which was causing the glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was rendered.

So, the proposed fix is to add jre provided font "Lucida Sans Regular"
to the cascaded list so that we get the "alef" glyph.
The reason for choosing "Lucida Sans Regular" over "Lucida Bright Regular" is, because it is the largest font file in jre and has all the glyph codepoints that no other font in the jre has, so we will not lose out on any codepoints and will help us in not getting missing glyph.

webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/

Regards
Prasanta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/4ad48b17/attachment-0001.html>

------------------------------

Message: 2
Date: Thu, 09 Feb 2017 11:30:26 -0800
From: Philip Race <[hidden email]>
To: 2d-dev <[hidden email]>
Subject: [OpenJDK 2D-Dev] RFR: 8172967: [macosx] Exception while
        working with layout for text containing unmappable character
Message-ID: <[hidden email]>
Content-Type: text/plain; charset=UTF-8; format=flowed


http://cr.openjdk.java.net/~prr/8172967/
https://bugs.openjdk.java.net/browse/JDK-8172967

Full evaluation in the bug report.
Short summary: avoid AIOB and NPE when Mac glyph mapper returns a negated unicode which is misinterpreted as having composite font slot 255

-phil.


------------------------------

Message: 3
Date: Thu, 9 Feb 2017 13:59:37 -0800
From: Jim Graham <[hidden email]>
To: Jayathirth D V <[hidden email]>, Philip Race
        <[hidden email]>, [hidden email]
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
        ColorModel subclasses are missing hashCode() or equals() or both
        methods
Message-ID: <[hidden email]>
Content-Type: text/plain; charset=windows-1252; format=flowed

 From my end this looks good.  +1 except for 2 outstanding review issues:

- Would like to hear back final comments from Joe Darcy on the new doc changes/CCC request
- Phil pointed out that there is an unneeded import in some of the files.  I agree that we should make a final webrev to delete them, but I don't need to approve it if that is the only change...

                        ...jim

On 2/8/17 11:56 PM, Jayathirth D V wrote:

> Hello All,
>
> There was a closed test which was failing because of identity-as-equals approach for ColorModel equals() method.
> I have modified it and added in the webrev. Along with this we are now using colorspace.hashCode() in hashCode() functions instead of Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in IndexColorModel equals() method because Arrays.copyOf() takes lot of time.
>
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>
> Ran jtreg test and JCK there are no additional test case failures because of the above change. Only 4 JCK tests are failing as it was happening previously.
>
> Just copy pasted my observation regarding JCK failures so that we can trace it easily:
>
> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
> cases: 4; passed: 3; failed: 1; first test case failure:
> ColorModel2001
>
> This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In the test case they have bits per component array of length 4 like {8, 8, 8, 8}. But in the test case wherever they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check that we have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>
> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases:
> 3; passed: 2; failed: 1; first test case failure: ColorModel0004
>
> Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>
> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
> cases: 2; passed: 1; failed: 1; first test case failure:
> ColorModel2006
>
> Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we don't have hashCode check in ColorModel and it will be different between 2 Objects.
>
> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttes
> tCase1: Failed. test cases: 2; passed: 1; failed: 1; first test case
> failure: testCase1
>
> Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the first JCK test is failing. We omit alpha bit if hasAlpha is false but JCK test tries to call getComponentSize() with index 3 which throws ArrayIndexOutOfBoundsException.
>
> Thanks,
> Jay
> -----Original Message-----
> From: Jayathirth D V
> Sent: Wednesday, February 08, 2017 3:41 PM
> To: Jim Graham; Philip Race; [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
>
> Hello All,
>
> I have updated the webrev to include the following changes.
>
> 1) Have identity as equals check in equals() method of ColorModel but elaborate the specification of equals() and hashCode() in ColorModel on what properties to     check in subclasses of ColorModel.
> 2) Made changes to test case to have single helper method wherever we have same equals/hashCode() check.
> 3) Updated IndexColorModel equals() method to use Arrays.equals() for rgb[] data.
> 4) Add comment on why we are not using validBits to calculate hashCode() in IndexColorModel hashCode() method.
>
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, February 02, 2017 2:51 AM
> To: Phil Race; Jayathirth D V; [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
>
> I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix.
> I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...
>
> ...jim
>
> On 1/31/17 2:34 PM, Jim Graham wrote:
>> For an application to run into this same issue they'd have to expect
>> getCompSizes() to return data for components that don't exist.  It's
>> unlikely they would use that data if they really understand the
>> objects.  While that would be odd, I guess I can see someone might be
>> constructing all of their CM's from an array of 4 components
>> regardless of the number of actual components and we'd be happily
>> remembering the useless extra components and returning an array of 4
>> from getCompSizes().  As I said, they shouldn't really be reading and
>> interpreting those extra components for any image processing, but I can imagine that they might do something like create a variant CM by calling the CompSizes() and copying them into a new array to construct a new CM with modifications.  They might just robotically always copy 4 values without really checking how many are valid.  That's a stretch, and their code is weak.  I can conceive of how this might happen, but I really have no idea how likely it is...
>>
>>             ...jim
>>
>> On 1/30/17 3:56 PM, Phil Race wrote:
>>> Sounds like we should at least try to get the tests updated so they only test what the spec. says.
>>> Although it does indicate that there is at least a chance that
>>> application code might also fail due to similar assumptions.
>>> Does #1 not fail with the previous iteration of this change too ?
>>>
>>> -phil.
>>>
>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>> Hmmm.  Sounds like the test cases were written based on bugs in the
>>>> implementation.  I'm not sure what the best tactic is here for the
>>>> short term for getting this in, but many of these changes should eventually be considered bugs in the tests.  Is it acceptable to break API tests like this at the last minute even if the tests are at fault?  Phil?
>>>>
>>>> Notes on specific instances below...
>>>>
>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>> Hi Phil,
>>>>>
>>>>>     1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>> ColorModel2001
>>>>>
>>>>>     This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In
>>>>> the test case they have bits per component array     of length 4 like {8, 8, 8, 8}. But in the test case wherever
>>>>> they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check     that we
>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>
>>>> This is a bug in the test then, especially if the size of our array matches the return value of getNumComponents.
>>>>
>>>>>     2)api/java_awt/Image/ColorModel/index.html#Equals: Failed.
>>>>> test
>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>> failure: ColorModel0004
>>>>>
>>>>>     Here they check for equality between 2 ColorModel objects
>>>>> having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>>>
>>>> How do they accomplish this when the CM class is abstract?  Do they
>>>> create a relatively empty subclass and instantiate that?
>>>>
>>>> The documentation for the equals() method does not document the
>>>> conditions under which it returns true, it uses a vague concept of "equals this ColorModel".  I don't see how they could test anything given that documentation.
>>>>
>>>>>     3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>> failure: ColorModel2006
>>>>>
>>>>>     Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we
>>>>> don't have hashCode check in ColorModel and it     will be different between 2 Objects.
>>>>
>>>> Same as above, there are no promises documented.
>>>>
>>>>>
>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTes
>>>>> t
>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>> failed: 1; first test case failure: testCase1
>>>>>
>>>>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the
>>>>> first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK test tries to call getComponentSize()
>>>>> with index 3 which throws ArrayIndexOutOfBoundsException.
>>>>
>>>> Same assessment as #1 above...
>>>>
>>>> Again, while these are my recommendations about the correctness of
>>>> these tests, the question remains whether we want to introduce a
>>>> change at this point in the release cycle that will essentially invalidate a number of tests that have been working for several releases already.  I'll leave that tactic issue to Phil...
>>>>
>>>>                 ...jim
>>>>
>>>


------------------------------

Message: 4
Date: Thu, 9 Feb 2017 14:05:24 -0800
From: Phil Race <[hidden email]>
To: Jim Graham <[hidden email]>, Jayathirth D V
        <[hidden email]>, [hidden email]
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
        ColorModel subclasses are missing hashCode() or equals() or both
        methods
Message-ID: <[hidden email]>
Content-Type: text/plain; charset="windows-1252"; Format="flowed"

Oh .. my reply was to an off-list email. I did not notice that.
So I should repeat that here :

On 2/9/17 12:38 PM, Phil Race wrote:
> 32 import java.util.Objects;
>
> This is now un-used, isn't it ? Yet all 3 subclasses still have this
> import.
>
> I don't need to "approve" a new webrev containing that but it would be
> good to publish one.
>
> +1

-phil.


On 02/09/2017 01:59 PM, Jim Graham wrote:

> From my end this looks good.  +1 except for 2 outstanding review issues:
>
> - Would like to hear back final comments from Joe Darcy on the new doc
> changes/CCC request
> - Phil pointed out that there is an unneeded import in some of the
> files.  I agree that we should make a final webrev to delete them, but
> I don't need to approve it if that is the only change...
>
>             ...jim
>
> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>> Hello All,
>>
>> There was a closed test which was failing because of
>> identity-as-equals approach for ColorModel equals() method.
>> I have modified it and added in the webrev. Along with this we are
>> now using colorspace.hashCode() in hashCode() functions instead of
>> Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in
>> IndexColorModel equals() method because Arrays.copyOf() takes lot of
>> time.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>
>> Ran jtreg test and JCK there are no additional test case failures
>> because of the above change. Only 4 JCK tests are failing as it was
>> happening previously.
>>
>> Just copy pasted my observation regarding JCK failures so that we can
>> trace it easily:
>>
>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
>> cases: 4; passed: 3; failed: 1; first test case failure:
>> ColorModel2001
>>
>>     This test fails because getComponentSize() returned an array with
>> length 3 but it expects the length to be 4. In the test case they
>> have bits per component array     of length 4 like {8, 8, 8, 8}. But
>> in the test case wherever they are passing "has Alpha" as "false" we
>> omit the alpha component bit. This is because of tighter check    
>> that we have in ColorModel class as "nBits = Arrays.copyOf(bits,
>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>
>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>> cases: 3; passed: 2; failed: 1; first test case failure:
>> ColorModel0004
>>
>>     Here they check for equality between 2 ColorModel objects having
>> same values, but it fails because now we are using identity-as-equals
>> check in ColorModel.
>>
>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
>> cases: 2; passed: 1; failed: 1; first test case failure:
>> ColorModel2006
>>
>>     Here they check for hashCode equality between 2 ColorModel
>> objects having same values, but it fails since we don't have hashCode
>> check in ColorModel and it     will be different between 2 Objects.
>>
>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1:
>> Failed. test cases: 2; passed: 1; failed: 1; first test case failure:
>> testCase1
>>
>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>> also happening because of same reason as why the first JCK test is
>> failing. We omit alpha bit if     hasAlpha is false but JCK test
>> tries to call getComponentSize() with index 3 which throws
>> ArrayIndexOutOfBoundsException.
>>
>> Thanks,
>> Jay
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Wednesday, February 08, 2017 3:41 PM
>> To: Jim Graham; Philip Race; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> Hello All,
>>
>> I have updated the webrev to include the following changes.
>>
>>     1) Have identity as equals check in equals() method of ColorModel
>> but elaborate the specification of equals() and hashCode() in
>> ColorModel on what properties to          check in subclasses of
>> ColorModel.
>>     2) Made changes to test case to have single helper method
>> wherever we have same equals/hashCode() check.
>>     3) Updated IndexColorModel equals() method to use Arrays.equals()
>> for rgb[] data.
>>     4) Add comment on why we are not using validBits to calculate
>> hashCode() in IndexColorModel hashCode() method.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Thursday, February 02, 2017 2:51 AM
>> To: Phil Race; Jayathirth D V; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> I think we should move this issue (array size returned from
>> getCompSizes) into a separate bug entry and a separate fix.
>> I don't think we need to fix the clone() in the constructor and the
>> getter just to get hashcode/equals right...
>>
>>             ...jim
>>
>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>> For an application to run into this same issue they'd have to expect
>>> getCompSizes() to return data for components that don't exist.  It's
>>> unlikely they would use that data if they really understand the
>>> objects.  While that would be odd, I guess I can see someone might
>>> be constructing all of their CM's from an array of 4 components
>>> regardless of the number of actual components and we'd be happily
>>> remembering the useless extra components and returning an array of 4
>>> from getCompSizes().  As I said, they shouldn't really be reading
>>> and interpreting those extra components for any image processing,
>>> but I can imagine that they might do something like create a variant
>>> CM by calling the CompSizes() and copying them into a new array to
>>> construct a new CM with modifications.  They might just robotically
>>> always copy 4 values without really checking how many are valid.
>>> That's a stretch, and their code is weak.  I can conceive of how
>>> this might happen, but I really have no idea how likely it is...
>>>
>>>             ...jim
>>>
>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>> Sounds like we should at least try to get the tests updated so they
>>>> only test what the spec. says.
>>>> Although it does indicate that there is at least a chance that
>>>> application code might also fail due to similar assumptions.
>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>
>>>> -phil.
>>>>
>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>> Hmmm.  Sounds like the test cases were written based on bugs in
>>>>> the implementation.  I'm not sure what the best tactic is here for
>>>>> the short term for getting this in, but many of these changes
>>>>> should eventually be considered bugs in the tests.  Is it
>>>>> acceptable to break API tests like this at the last minute even if
>>>>> the tests are at fault?  Phil?
>>>>>
>>>>> Notes on specific instances below...
>>>>>
>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>> Hi Phil,
>>>>>>
>>>>>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>>> ColorModel2001
>>>>>>
>>>>>>     This test fails because getComponentSize() returned an array
>>>>>> with length 3 but it expects the length to be 4. In
>>>>>> the test case they have bits per component array     of length 4
>>>>>> like {8, 8, 8, 8}. But in the test case wherever they are passing
>>>>>> "has Alpha" as "false" we omit the alpha
>>>>>> component bit. This is because of tighter check     that we
>>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>>
>>>>> This is a bug in the test then, especially if the size of our
>>>>> array matches the return value of getNumComponents.
>>>>>
>>>>>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>>> failure: ColorModel0004
>>>>>>
>>>>>>     Here they check for equality between 2 ColorModel objects
>>>>>> having same values, but it fails because now we are using
>>>>>> identity-as-equals check in ColorModel.
>>>>>
>>>>> How do they accomplish this when the CM class is abstract?  Do
>>>>> they create a relatively empty subclass and instantiate that?
>>>>>
>>>>> The documentation for the equals() method does not document the
>>>>> conditions under which it returns true, it uses a vague concept of
>>>>> "equals this ColorModel".  I don't see how they could test
>>>>> anything given that documentation.
>>>>>
>>>>>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>>> failure: ColorModel2006
>>>>>>
>>>>>>     Here they check for hashCode equality between 2 ColorModel
>>>>>> objects having same values, but it fails since we
>>>>>> don't have hashCode check in ColorModel and it     will be
>>>>>> different between 2 Objects.
>>>>>
>>>>> Same as above, there are no promises documented.
>>>>>
>>>>>>
>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTe
>>>>>> st
>>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>>> failed: 1; first test case failure: testCase1
>>>>>>
>>>>>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>>>>>> also happening because of same reason as why the first JCK test
>>>>>> is failing. We omit alpha bit if hasAlpha is false but JCK test
>>>>>> tries to call getComponentSize() with index 3 which throws
>>>>>> ArrayIndexOutOfBoundsException.
>>>>>
>>>>> Same assessment as #1 above...
>>>>>
>>>>> Again, while these are my recommendations about the correctness of
>>>>> these tests, the question remains whether we want to introduce a
>>>>> change at this point in the release cycle that will essentially
>>>>> invalidate a number of tests that have been working for several
>>>>> releases already.  I'll leave that tactic issue to Phil...
>>>>>
>>>>>                 ...jim
>>>>>
>>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/c71a98ce/attachment.html>

End of 2d-dev Digest, Vol 117, Issue 3
**************************************


End of 2d-dev Digest, Vol 117, Issue 4
**************************************
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

prasanta sadhukhan
Hi Phil,

Yes, I agree with Prahalad's findings. If "Lucida Sans Regular" is
removed, then findFont2D() call, later on called in
createCompositeFont() will return null and so the font2D object will not
get added to the list of Physical fonts
and we will get missing glyph ie empty box and NOT an exception. I have
tested this scenario too.

Regards
Prasanta
On 2/10/2017 9:51 AM, Prahalad Kumar Narayanan wrote:

> Hello Phil
>
> A very good feedback:
>> I am OK with this if you can confirm that removing Lucida Sans Regular from the JDK does not cause any nasty exceptions.
> I believe, removing Lucida Sans Regular from JDK won't cause any trouble.
>
> Reason is that- FontManager.findFont2D should return NULL in this case. The logic that follows this call in CFont.java checks if number of physical fonts is lesser than expected count and updates the list of fonts accordingly.
>   234         if (idx < fonts.length) {
>   235             PhysicalFont[] orig = fonts;
>   236             fonts = new PhysicalFont[idx];
>   237             System.arraycopy(orig, 0, fonts, 0, idx);
>   238         }
>
> A quick look at SunFontManager.findFont2D : The definition invokes findJREDeferredFont method to first search for an entry in jreFontMap and then initialize the Lucida Sans font. If the initialization succeeds a valid physical font is returned. Upon failure, it should return NULL.
>
> Thanks
> Have a good day
>
> Prahalad N.
>
> ----------------------------------------------------------------------
> Message: 1
> Date: Thu, 09 Feb 2017 19:09:00 -0800
> From: Philip Race <[hidden email]>
> To: Prasanta Sadhukhan <[hidden email]>
> Cc: 2d-dev <[hidden email]>
> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
> character cannot be rendered on MacOS X
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="utf-8"; Format="flowed"
>
> I am OK with this if you can confirm that removing Lucida Sans Regular from the JDK does not cause any nasty exceptions.
>
> Apple use the PostScript name but our lookup code should work for the TrueType name.
>
> So overall I think this should be fine - from visual inspection - and assuming it has been tested :-)
>
> +1
>
> -phil.
>
> On 2/9/17, 1:08 AM, Prasanta Sadhukhan wrote:
>> Hi All,
>>
>> Please review a fix for an issue which causes arabic character "alef"
>> to be not rendered in osx for menlo font in italic style.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>>
>> The issue was actually a regression caused by the fix to JDK-7162125:
>> [macosx] A font has different behaviour for ligatures depending on its
>> creation mode in which we have added cascaded font list to find the
>> real fonts that CFont uses, so that there is no need to use "negative"
>> glyph code for finding the fallback fonts using the
>> "subsititution"/"fallback" mechanism used by osx code.
>>
>> However, the above logic of using cascaded font list in CFont does not
>> take into account of using JRE provided fonts like all those Lucida*
>> ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef'
>> character) is intended to be rendered in Menlo font in italic style,
>> osx will not be able to find the glyph in Menlo-Italic font and
>> neither in all the cascaded system fonts provided by CoreText, so it
>> results in empty box.
>>
>> Before 7162125 fix, the fallback code in
>> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode()
>> uses JRSFontCreateFallbackFontForCharacters()
>> was adding jre/lib/fonts to the fallback list which was causing the
>> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
>> rendered.
>>
>> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
>> to the cascaded list so that we get the "alef" glyph.
>> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
>> Regular" is, because it is the largest font file in jre and has all
>> the glyph codepoints that no other font in the jre has, so we will not
>> lose out on any codepoints and will help us in not getting missing
>> glyph.
>>
>> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/
>> <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>>
>> Regards
>> Prasanta
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/37baa90a/attachment-0001.html>
>
> ------------------------------
>
> Message: 2
> Date: Thu, 9 Feb 2017 19:34:34 -0800 (PST)
> From: Prahalad Kumar Narayanan <[hidden email]>
> To: [hidden email]
> Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
> character cannot be rendered on MacOS X
> Message-ID: <98e3b75f-b2e5-41a4-9489-62ec34b05985@default>
> Content-Type: text/plain; charset=us-ascii
>
> The change looks good. +1.
>
> Minor observation: The copyright should be updated to 2017 in the CFont.java file.
> If you use any script to update the same, then ignore this observation.
>
> Thanks
> Have a good day
>
> Prahalad N.
>
> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]]
> Sent: Friday, February 10, 2017 3:37 AM
> To: [hidden email]
> Subject: 2d-dev Digest, Vol 117, Issue 3
>
> Send 2d-dev mailing list submissions to
> [hidden email]
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://mail.openjdk.java.net/mailman/listinfo/2d-dev
> or, via email, send a message with subject or body 'help' to
> [hidden email]
>
> You can reach the person managing the list at
> [hidden email]
>
> When replying, please edit your Subject line so it is more specific than "Re: Contents of 2d-dev digest..."
>
>
> Today's Topics:
>
>     1.  [9] RFR JDK-8147002:[macosx] Arabic character cannot be
>        rendered on MacOS X (Prasanta Sadhukhan)
>     2.  RFR: 8172967: [macosx] Exception while working with layout
>        for text containing unmappable character (Philip Race)
>     3. Re:  Review Request for JDK-7107905: ColorModel subclasses
>        are missing hashCode() or equals() or both methods (Jim Graham)
>     4. Re:  Review Request for JDK-7107905: ColorModel subclasses
>        are missing hashCode() or equals() or both methods (Phil Race)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Thu, 9 Feb 2017 14:38:11 +0530
> From: Prasanta Sadhukhan <[hidden email]>
> To: 2d-dev <[hidden email]>, Philip Race
> <[hidden email]>
> Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
> character cannot be rendered on MacOS X
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="utf-8"; Format="flowed"
>
> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its creation mode in which we have added cascaded font list to find the real fonts that CFont uses, so that there is no need to use "negative" glyph code for finding the fallback fonts using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not take into account of using JRE provided fonts like all those Lucida* ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef' character) is intended to be rendered in Menlo font in italic style, osx will not be able to find the glyph in Menlo-Italic font and neither in all the cascaded system fonts provided by CoreText, so it results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright Regular" is, because it is the largest font file in jre and has all the glyph codepoints that no other font in the jre has, so we will not lose out on any codepoints and will help us in not getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/
>
> Regards
> Prasanta
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/4ad48b17/attachment-0001.html>
>
> ------------------------------
>
> Message: 2
> Date: Thu, 09 Feb 2017 11:30:26 -0800
> From: Philip Race <[hidden email]>
> To: 2d-dev <[hidden email]>
> Subject: [OpenJDK 2D-Dev] RFR: 8172967: [macosx] Exception while
> working with layout for text containing unmappable character
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
>
> http://cr.openjdk.java.net/~prr/8172967/
> https://bugs.openjdk.java.net/browse/JDK-8172967
>
> Full evaluation in the bug report.
> Short summary: avoid AIOB and NPE when Mac glyph mapper returns a negated unicode which is misinterpreted as having composite font slot 255
>
> -phil.
>
>
> ------------------------------
>
> Message: 3
> Date: Thu, 9 Feb 2017 13:59:37 -0800
> From: Jim Graham <[hidden email]>
> To: Jayathirth D V <[hidden email]>, Philip Race
> <[hidden email]>, [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset=windows-1252; format=flowed
>
>   From my end this looks good.  +1 except for 2 outstanding review issues:
>
> - Would like to hear back final comments from Joe Darcy on the new doc changes/CCC request
> - Phil pointed out that there is an unneeded import in some of the files.  I agree that we should make a final webrev to delete them, but I don't need to approve it if that is the only change...
>
> ...jim
>
> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>> Hello All,
>>
>> There was a closed test which was failing because of identity-as-equals approach for ColorModel equals() method.
>> I have modified it and added in the webrev. Along with this we are now using colorspace.hashCode() in hashCode() functions instead of Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in IndexColorModel equals() method because Arrays.copyOf() takes lot of time.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>
>> Ran jtreg test and JCK there are no additional test case failures because of the above change. Only 4 JCK tests are failing as it was happening previously.
>>
>> Just copy pasted my observation regarding JCK failures so that we can trace it easily:
>>
>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
>> cases: 4; passed: 3; failed: 1; first test case failure:
>> ColorModel2001
>>
>> This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In the test case they have bits per component array of length 4 like {8, 8, 8, 8}. But in the test case wherever they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check that we have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>
>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases:
>> 3; passed: 2; failed: 1; first test case failure: ColorModel0004
>>
>> Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>
>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
>> cases: 2; passed: 1; failed: 1; first test case failure:
>> ColorModel2006
>>
>> Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we don't have hashCode check in ColorModel and it will be different between 2 Objects.
>>
>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttes
>> tCase1: Failed. test cases: 2; passed: 1; failed: 1; first test case
>> failure: testCase1
>>
>> Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the first JCK test is failing. We omit alpha bit if hasAlpha is false but JCK test tries to call getComponentSize() with index 3 which throws ArrayIndexOutOfBoundsException.
>>
>> Thanks,
>> Jay
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Wednesday, February 08, 2017 3:41 PM
>> To: Jim Graham; Philip Race; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> Hello All,
>>
>> I have updated the webrev to include the following changes.
>>
>> 1) Have identity as equals check in equals() method of ColorModel but elaborate the specification of equals() and hashCode() in ColorModel on what properties to     check in subclasses of ColorModel.
>> 2) Made changes to test case to have single helper method wherever we have same equals/hashCode() check.
>> 3) Updated IndexColorModel equals() method to use Arrays.equals() for rgb[] data.
>> 4) Add comment on why we are not using validBits to calculate hashCode() in IndexColorModel hashCode() method.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Thursday, February 02, 2017 2:51 AM
>> To: Phil Race; Jayathirth D V; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix.
>> I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...
>>
>> ...jim
>>
>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>> For an application to run into this same issue they'd have to expect
>>> getCompSizes() to return data for components that don't exist.  It's
>>> unlikely they would use that data if they really understand the
>>> objects.  While that would be odd, I guess I can see someone might be
>>> constructing all of their CM's from an array of 4 components
>>> regardless of the number of actual components and we'd be happily
>>> remembering the useless extra components and returning an array of 4
>>> from getCompSizes().  As I said, they shouldn't really be reading and
>>> interpreting those extra components for any image processing, but I can imagine that they might do something like create a variant CM by calling the CompSizes() and copying them into a new array to construct a new CM with modifications.  They might just robotically always copy 4 values without really checking how many are valid.  That's a stretch, and their code is weak.  I can conceive of how this might happen, but I really have no idea how likely it is...
>>>
>>>              ...jim
>>>
>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>> Sounds like we should at least try to get the tests updated so they only test what the spec. says.
>>>> Although it does indicate that there is at least a chance that
>>>> application code might also fail due to similar assumptions.
>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>
>>>> -phil.
>>>>
>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>> Hmmm.  Sounds like the test cases were written based on bugs in the
>>>>> implementation.  I'm not sure what the best tactic is here for the
>>>>> short term for getting this in, but many of these changes should eventually be considered bugs in the tests.  Is it acceptable to break API tests like this at the last minute even if the tests are at fault?  Phil?
>>>>>
>>>>> Notes on specific instances below...
>>>>>
>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>> Hi Phil,
>>>>>>
>>>>>>      1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>>> ColorModel2001
>>>>>>
>>>>>>      This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In
>>>>>> the test case they have bits per component array     of length 4 like {8, 8, 8, 8}. But in the test case wherever
>>>>>> they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check     that we
>>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>> This is a bug in the test then, especially if the size of our array matches the return value of getNumComponents.
>>>>>
>>>>>>      2)api/java_awt/Image/ColorModel/index.html#Equals: Failed.
>>>>>> test
>>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>>> failure: ColorModel0004
>>>>>>
>>>>>>      Here they check for equality between 2 ColorModel objects
>>>>>> having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>>>> How do they accomplish this when the CM class is abstract?  Do they
>>>>> create a relatively empty subclass and instantiate that?
>>>>>
>>>>> The documentation for the equals() method does not document the
>>>>> conditions under which it returns true, it uses a vague concept of "equals this ColorModel".  I don't see how they could test anything given that documentation.
>>>>>
>>>>>>      3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>>> failure: ColorModel2006
>>>>>>
>>>>>>      Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we
>>>>>> don't have hashCode check in ColorModel and it     will be different between 2 Objects.
>>>>> Same as above, there are no promises documented.
>>>>>
>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTes
>>>>>> t
>>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>>> failed: 1; first test case failure: testCase1
>>>>>>
>>>>>>      Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the
>>>>>> first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK test tries to call getComponentSize()
>>>>>> with index 3 which throws ArrayIndexOutOfBoundsException.
>>>>> Same assessment as #1 above...
>>>>>
>>>>> Again, while these are my recommendations about the correctness of
>>>>> these tests, the question remains whether we want to introduce a
>>>>> change at this point in the release cycle that will essentially invalidate a number of tests that have been working for several releases already.  I'll leave that tactic issue to Phil...
>>>>>
>>>>>                  ...jim
>>>>>
>
> ------------------------------
>
> Message: 4
> Date: Thu, 9 Feb 2017 14:05:24 -0800
> From: Phil Race <[hidden email]>
> To: Jim Graham <[hidden email]>, Jayathirth D V
> <[hidden email]>, [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="windows-1252"; Format="flowed"
>
> Oh .. my reply was to an off-list email. I did not notice that.
> So I should repeat that here :
>
> On 2/9/17 12:38 PM, Phil Race wrote:
>> 32 import java.util.Objects;
>>
>> This is now un-used, isn't it ? Yet all 3 subclasses still have this
>> import.
>>
>> I don't need to "approve" a new webrev containing that but it would be
>> good to publish one.
>>
>> +1
> -phil.
>
>
> On 02/09/2017 01:59 PM, Jim Graham wrote:
>>  From my end this looks good.  +1 except for 2 outstanding review issues:
>>
>> - Would like to hear back final comments from Joe Darcy on the new doc
>> changes/CCC request
>> - Phil pointed out that there is an unneeded import in some of the
>> files.  I agree that we should make a final webrev to delete them, but
>> I don't need to approve it if that is the only change...
>>
>>              ...jim
>>
>> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>>> Hello All,
>>>
>>> There was a closed test which was failing because of
>>> identity-as-equals approach for ColorModel equals() method.
>>> I have modified it and added in the webrev. Along with this we are
>>> now using colorspace.hashCode() in hashCode() functions instead of
>>> Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in
>>> IndexColorModel equals() method because Arrays.copyOf() takes lot of
>>> time.
>>>
>>> Please find updated webrev for review :
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>>
>>> Ran jtreg test and JCK there are no additional test case failures
>>> because of the above change. Only 4 JCK tests are failing as it was
>>> happening previously.
>>>
>>> Just copy pasted my observation regarding JCK failures so that we can
>>> trace it easily:
>>>
>>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
>>> cases: 4; passed: 3; failed: 1; first test case failure:
>>> ColorModel2001
>>>
>>>      This test fails because getComponentSize() returned an array with
>>> length 3 but it expects the length to be 4. In the test case they
>>> have bits per component array     of length 4 like {8, 8, 8, 8}. But
>>> in the test case wherever they are passing "has Alpha" as "false" we
>>> omit the alpha component bit. This is because of tighter check
>>> that we have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>
>>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>> cases: 3; passed: 2; failed: 1; first test case failure:
>>> ColorModel0004
>>>
>>>      Here they check for equality between 2 ColorModel objects having
>>> same values, but it fails because now we are using identity-as-equals
>>> check in ColorModel.
>>>
>>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
>>> cases: 2; passed: 1; failed: 1; first test case failure:
>>> ColorModel2006
>>>
>>>      Here they check for hashCode equality between 2 ColorModel
>>> objects having same values, but it fails since we don't have hashCode
>>> check in ColorModel and it     will be different between 2 Objects.
>>>
>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1:
>>> Failed. test cases: 2; passed: 1; failed: 1; first test case failure:
>>> testCase1
>>>
>>>      Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>>> also happening because of same reason as why the first JCK test is
>>> failing. We omit alpha bit if     hasAlpha is false but JCK test
>>> tries to call getComponentSize() with index 3 which throws
>>> ArrayIndexOutOfBoundsException.
>>>
>>> Thanks,
>>> Jay
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Wednesday, February 08, 2017 3:41 PM
>>> To: Jim Graham; Philip Race; [hidden email]
>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>>> ColorModel subclasses are missing hashCode() or equals() or both
>>> methods
>>>
>>> Hello All,
>>>
>>> I have updated the webrev to include the following changes.
>>>
>>>      1) Have identity as equals check in equals() method of ColorModel
>>> but elaborate the specification of equals() and hashCode() in
>>> ColorModel on what properties to          check in subclasses of
>>> ColorModel.
>>>      2) Made changes to test case to have single helper method
>>> wherever we have same equals/hashCode() check.
>>>      3) Updated IndexColorModel equals() method to use Arrays.equals()
>>> for rgb[] data.
>>>      4) Add comment on why we are not using validBits to calculate
>>> hashCode() in IndexColorModel hashCode() method.
>>>
>>> Please find updated webrev for review :
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Thursday, February 02, 2017 2:51 AM
>>> To: Phil Race; Jayathirth D V; [hidden email]
>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>>> ColorModel subclasses are missing hashCode() or equals() or both
>>> methods
>>>
>>> I think we should move this issue (array size returned from
>>> getCompSizes) into a separate bug entry and a separate fix.
>>> I don't think we need to fix the clone() in the constructor and the
>>> getter just to get hashcode/equals right...
>>>
>>>              ...jim
>>>
>>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>>> For an application to run into this same issue they'd have to expect
>>>> getCompSizes() to return data for components that don't exist.  It's
>>>> unlikely they would use that data if they really understand the
>>>> objects.  While that would be odd, I guess I can see someone might
>>>> be constructing all of their CM's from an array of 4 components
>>>> regardless of the number of actual components and we'd be happily
>>>> remembering the useless extra components and returning an array of 4
>>>> from getCompSizes().  As I said, they shouldn't really be reading
>>>> and interpreting those extra components for any image processing,
>>>> but I can imagine that they might do something like create a variant
>>>> CM by calling the CompSizes() and copying them into a new array to
>>>> construct a new CM with modifications.  They might just robotically
>>>> always copy 4 values without really checking how many are valid.
>>>> That's a stretch, and their code is weak.  I can conceive of how
>>>> this might happen, but I really have no idea how likely it is...
>>>>
>>>>              ...jim
>>>>
>>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>>> Sounds like we should at least try to get the tests updated so they
>>>>> only test what the spec. says.
>>>>> Although it does indicate that there is at least a chance that
>>>>> application code might also fail due to similar assumptions.
>>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>>> Hmmm.  Sounds like the test cases were written based on bugs in
>>>>>> the implementation.  I'm not sure what the best tactic is here for
>>>>>> the short term for getting this in, but many of these changes
>>>>>> should eventually be considered bugs in the tests.  Is it
>>>>>> acceptable to break API tests like this at the last minute even if
>>>>>> the tests are at fault?  Phil?
>>>>>>
>>>>>> Notes on specific instances below...
>>>>>>
>>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>>> Hi Phil,
>>>>>>>
>>>>>>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>>>> ColorModel2001
>>>>>>>
>>>>>>>      This test fails because getComponentSize() returned an array
>>>>>>> with length 3 but it expects the length to be 4. In
>>>>>>> the test case they have bits per component array     of length 4
>>>>>>> like {8, 8, 8, 8}. But in the test case wherever they are passing
>>>>>>> "has Alpha" as "false" we omit the alpha
>>>>>>> component bit. This is because of tighter check     that we
>>>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>>> This is a bug in the test then, especially if the size of our
>>>>>> array matches the return value of getNumComponents.
>>>>>>
>>>>>>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>>>> failure: ColorModel0004
>>>>>>>
>>>>>>>      Here they check for equality between 2 ColorModel objects
>>>>>>> having same values, but it fails because now we are using
>>>>>>> identity-as-equals check in ColorModel.
>>>>>> How do they accomplish this when the CM class is abstract?  Do
>>>>>> they create a relatively empty subclass and instantiate that?
>>>>>>
>>>>>> The documentation for the equals() method does not document the
>>>>>> conditions under which it returns true, it uses a vague concept of
>>>>>> "equals this ColorModel".  I don't see how they could test
>>>>>> anything given that documentation.
>>>>>>
>>>>>>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>>>> failure: ColorModel2006
>>>>>>>
>>>>>>>      Here they check for hashCode equality between 2 ColorModel
>>>>>>> objects having same values, but it fails since we
>>>>>>> don't have hashCode check in ColorModel and it     will be
>>>>>>> different between 2 Objects.
>>>>>> Same as above, there are no promises documented.
>>>>>>
>>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTe
>>>>>>> st
>>>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>>>> failed: 1; first test case failure: testCase1
>>>>>>>
>>>>>>>      Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>>>>>>> also happening because of same reason as why the first JCK test
>>>>>>> is failing. We omit alpha bit if hasAlpha is false but JCK test
>>>>>>> tries to call getComponentSize() with index 3 which throws
>>>>>>> ArrayIndexOutOfBoundsException.
>>>>>> Same assessment as #1 above...
>>>>>>
>>>>>> Again, while these are my recommendations about the correctness of
>>>>>> these tests, the question remains whether we want to introduce a
>>>>>> change at this point in the release cycle that will essentially
>>>>>> invalidate a number of tests that have been working for several
>>>>>> releases already.  I'll leave that tactic issue to Phil...
>>>>>>
>>>>>>                  ...jim
>>>>>>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/c71a98ce/attachment.html>
>
> End of 2d-dev Digest, Vol 117, Issue 3
> **************************************
>
>
> End of 2d-dev Digest, Vol 117, Issue 4
> **************************************

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Jayathirth D v
Hi Prasanta,

Changes are fine.

Thanks,
Jay

-----Original Message-----
From: Prasanta Sadhukhan
Sent: Friday, February 10, 2017 10:32 AM
To: Prahalad Kumar Narayanan; Philip Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Hi Phil,

Yes, I agree with Prahalad's findings. If "Lucida Sans Regular" is removed, then findFont2D() call, later on called in
createCompositeFont() will return null and so the font2D object will not get added to the list of Physical fonts and we will get missing glyph ie empty box and NOT an exception. I have tested this scenario too.

Regards
Prasanta
On 2/10/2017 9:51 AM, Prahalad Kumar Narayanan wrote:

> Hello Phil
>
> A very good feedback:
>> I am OK with this if you can confirm that removing Lucida Sans Regular from the JDK does not cause any nasty exceptions.
> I believe, removing Lucida Sans Regular from JDK won't cause any trouble.
>
> Reason is that- FontManager.findFont2D should return NULL in this case. The logic that follows this call in CFont.java checks if number of physical fonts is lesser than expected count and updates the list of fonts accordingly.
>   234         if (idx < fonts.length) {
>   235             PhysicalFont[] orig = fonts;
>   236             fonts = new PhysicalFont[idx];
>   237             System.arraycopy(orig, 0, fonts, 0, idx);
>   238         }
>
> A quick look at SunFontManager.findFont2D : The definition invokes findJREDeferredFont method to first search for an entry in jreFontMap and then initialize the Lucida Sans font. If the initialization succeeds a valid physical font is returned. Upon failure, it should return NULL.
>
> Thanks
> Have a good day
>
> Prahalad N.
>
> ----------------------------------------------------------------------
> Message: 1
> Date: Thu, 09 Feb 2017 19:09:00 -0800
> From: Philip Race <[hidden email]>
> To: Prasanta Sadhukhan <[hidden email]>
> Cc: 2d-dev <[hidden email]>
> Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
> character cannot be rendered on MacOS X
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="utf-8"; Format="flowed"
>
> I am OK with this if you can confirm that removing Lucida Sans Regular from the JDK does not cause any nasty exceptions.
>
> Apple use the PostScript name but our lookup code should work for the TrueType name.
>
> So overall I think this should be fine - from visual inspection - and
> assuming it has been tested :-)
>
> +1
>
> -phil.
>
> On 2/9/17, 1:08 AM, Prasanta Sadhukhan wrote:
>> Hi All,
>>
>> Please review a fix for an issue which causes arabic character "alef"
>> to be not rendered in osx for menlo font in italic style.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>>
>> The issue was actually a regression caused by the fix to JDK-7162125:
>> [macosx] A font has different behaviour for ligatures depending on
>> its creation mode in which we have added cascaded font list to find
>> the real fonts that CFont uses, so that there is no need to use "negative"
>> glyph code for finding the fallback fonts using the
>> "subsititution"/"fallback" mechanism used by osx code.
>>
>> However, the above logic of using cascaded font list in CFont does
>> not take into account of using JRE provided fonts like all those
>> Lucida* ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef'
>> character) is intended to be rendered in Menlo font in italic style,
>> osx will not be able to find the glyph in Menlo-Italic font and
>> neither in all the cascaded system fonts provided by CoreText, so it
>> results in empty box.
>>
>> Before 7162125 fix, the fallback code in
>> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode()
>> uses JRSFontCreateFallbackFontForCharacters()
>> was adding jre/lib/fonts to the fallback list which was causing the
>> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
>> rendered.
>>
>> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
>> to the cascaded list so that we get the "alef" glyph.
>> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
>> Regular" is, because it is the largest font file in jre and has all
>> the glyph codepoints that no other font in the jre has, so we will
>> not lose out on any codepoints and will help us in not getting
>> missing glyph.
>>
>> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/
>> <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>>
>> Regards
>> Prasanta
> -------------- next part -------------- An HTML attachment was
> scrubbed...
> URL:
> <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/37
> baa90a/attachment-0001.html>
>
> ------------------------------
>
> Message: 2
> Date: Thu, 9 Feb 2017 19:34:34 -0800 (PST)
> From: Prahalad Kumar Narayanan <[hidden email]>
> To: [hidden email]
> Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
> character cannot be rendered on MacOS X
> Message-ID: <98e3b75f-b2e5-41a4-9489-62ec34b05985@default>
> Content-Type: text/plain; charset=us-ascii
>
> The change looks good. +1.
>
> Minor observation: The copyright should be updated to 2017 in the CFont.java file.
> If you use any script to update the same, then ignore this observation.
>
> Thanks
> Have a good day
>
> Prahalad N.
>
> -----Original Message-----
> From: [hidden email]
> [mailto:[hidden email]]
> Sent: Friday, February 10, 2017 3:37 AM
> To: [hidden email]
> Subject: 2d-dev Digest, Vol 117, Issue 3
>
> Send 2d-dev mailing list submissions to
> [hidden email]
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://mail.openjdk.java.net/mailman/listinfo/2d-dev
> or, via email, send a message with subject or body 'help' to
> [hidden email]
>
> You can reach the person managing the list at
> [hidden email]
>
> When replying, please edit your Subject line so it is more specific than "Re: Contents of 2d-dev digest..."
>
>
> Today's Topics:
>
>     1.  [9] RFR JDK-8147002:[macosx] Arabic character cannot be
>        rendered on MacOS X (Prasanta Sadhukhan)
>     2.  RFR: 8172967: [macosx] Exception while working with layout
>        for text containing unmappable character (Philip Race)
>     3. Re:  Review Request for JDK-7107905: ColorModel subclasses
>        are missing hashCode() or equals() or both methods (Jim Graham)
>     4. Re:  Review Request for JDK-7107905: ColorModel subclasses
>        are missing hashCode() or equals() or both methods (Phil Race)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Thu, 9 Feb 2017 14:38:11 +0530
> From: Prasanta Sadhukhan <[hidden email]>
> To: 2d-dev <[hidden email]>, Philip Race
> <[hidden email]>
> Subject: [OpenJDK 2D-Dev] [9] RFR JDK-8147002:[macosx] Arabic
> character cannot be rendered on MacOS X
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="utf-8"; Format="flowed"
>
> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its creation mode in which we have added cascaded font list to find the real fonts that CFont uses, so that there is no need to use "negative" glyph code for finding the fallback fonts using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not take into account of using JRE provided fonts like all those Lucida* ttf in jdk/lib/fonts/, so when a glyph (in this intance, arabic 'alef' character) is intended to be rendered in Menlo font in italic style, osx will not be able to find the glyph in Menlo-Italic font and neither in all the cascaded system fonts provided by CoreText, so it results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode()
> uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright Regular" is, because it is the largest font file in jre and has all the glyph codepoints that no other font in the jre has, so we will not lose out on any codepoints and will help us in not getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/
>
> Regards
> Prasanta
> -------------- next part -------------- An HTML attachment was
> scrubbed...
> URL:
> <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/4a
> d48b17/attachment-0001.html>
>
> ------------------------------
>
> Message: 2
> Date: Thu, 09 Feb 2017 11:30:26 -0800
> From: Philip Race <[hidden email]>
> To: 2d-dev <[hidden email]>
> Subject: [OpenJDK 2D-Dev] RFR: 8172967: [macosx] Exception while
> working with layout for text containing unmappable character
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
>
> http://cr.openjdk.java.net/~prr/8172967/
> https://bugs.openjdk.java.net/browse/JDK-8172967
>
> Full evaluation in the bug report.
> Short summary: avoid AIOB and NPE when Mac glyph mapper returns a
> negated unicode which is misinterpreted as having composite font slot
> 255
>
> -phil.
>
>
> ------------------------------
>
> Message: 3
> Date: Thu, 9 Feb 2017 13:59:37 -0800
> From: Jim Graham <[hidden email]>
> To: Jayathirth D V <[hidden email]>, Philip Race
> <[hidden email]>, [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset=windows-1252; format=flowed
>
>   From my end this looks good.  +1 except for 2 outstanding review issues:
>
> - Would like to hear back final comments from Joe Darcy on the new doc
> changes/CCC request
> - Phil pointed out that there is an unneeded import in some of the files.  I agree that we should make a final webrev to delete them, but I don't need to approve it if that is the only change...
>
> ...jim
>
> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>> Hello All,
>>
>> There was a closed test which was failing because of identity-as-equals approach for ColorModel equals() method.
>> I have modified it and added in the webrev. Along with this we are now using colorspace.hashCode() in hashCode() functions instead of Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in IndexColorModel equals() method because Arrays.copyOf() takes lot of time.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>
>> Ran jtreg test and JCK there are no additional test case failures because of the above change. Only 4 JCK tests are failing as it was happening previously.
>>
>> Just copy pasted my observation regarding JCK failures so that we can trace it easily:
>>
>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
>> cases: 4; passed: 3; failed: 1; first test case failure:
>> ColorModel2001
>>
>> This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In the test case they have bits per component array of length 4 like {8, 8, 8, 8}. But in the test case wherever they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check that we have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>
>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases:
>> 3; passed: 2; failed: 1; first test case failure: ColorModel0004
>>
>> Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>
>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
>> cases: 2; passed: 1; failed: 1; first test case failure:
>> ColorModel2006
>>
>> Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we don't have hashCode check in ColorModel and it will be different between 2 Objects.
>>
>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTestte
>> s
>> tCase1: Failed. test cases: 2; passed: 1; failed: 1; first test case
>> failure: testCase1
>>
>> Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the first JCK test is failing. We omit alpha bit if hasAlpha is false but JCK test tries to call getComponentSize() with index 3 which throws ArrayIndexOutOfBoundsException.
>>
>> Thanks,
>> Jay
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Wednesday, February 08, 2017 3:41 PM
>> To: Jim Graham; Philip Race; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> Hello All,
>>
>> I have updated the webrev to include the following changes.
>>
>> 1) Have identity as equals check in equals() method of ColorModel but elaborate the specification of equals() and hashCode() in ColorModel on what properties to     check in subclasses of ColorModel.
>> 2) Made changes to test case to have single helper method wherever we have same equals/hashCode() check.
>> 3) Updated IndexColorModel equals() method to use Arrays.equals() for rgb[] data.
>> 4) Add comment on why we are not using validBits to calculate hashCode() in IndexColorModel hashCode() method.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Thursday, February 02, 2017 2:51 AM
>> To: Phil Race; Jayathirth D V; [hidden email]
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix.
>> I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...
>>
>> ...jim
>>
>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>> For an application to run into this same issue they'd have to expect
>>> getCompSizes() to return data for components that don't exist.  It's
>>> unlikely they would use that data if they really understand the
>>> objects.  While that would be odd, I guess I can see someone might
>>> be constructing all of their CM's from an array of 4 components
>>> regardless of the number of actual components and we'd be happily
>>> remembering the useless extra components and returning an array of 4
>>> from getCompSizes().  As I said, they shouldn't really be reading
>>> and interpreting those extra components for any image processing, but I can imagine that they might do something like create a variant CM by calling the CompSizes() and copying them into a new array to construct a new CM with modifications.  They might just robotically always copy 4 values without really checking how many are valid.  That's a stretch, and their code is weak.  I can conceive of how this might happen, but I really have no idea how likely it is...
>>>
>>>              ...jim
>>>
>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>> Sounds like we should at least try to get the tests updated so they only test what the spec. says.
>>>> Although it does indicate that there is at least a chance that
>>>> application code might also fail due to similar assumptions.
>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>
>>>> -phil.
>>>>
>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>> Hmmm.  Sounds like the test cases were written based on bugs in
>>>>> the implementation.  I'm not sure what the best tactic is here for
>>>>> the short term for getting this in, but many of these changes should eventually be considered bugs in the tests.  Is it acceptable to break API tests like this at the last minute even if the tests are at fault?  Phil?
>>>>>
>>>>> Notes on specific instances below...
>>>>>
>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>> Hi Phil,
>>>>>>
>>>>>>      1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>>> ColorModel2001
>>>>>>
>>>>>>      This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In
>>>>>> the test case they have bits per component array     of length 4 like {8, 8, 8, 8}. But in the test case wherever
>>>>>> they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check     that we
>>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>> This is a bug in the test then, especially if the size of our array matches the return value of getNumComponents.
>>>>>
>>>>>>      2)api/java_awt/Image/ColorModel/index.html#Equals: Failed.
>>>>>> test
>>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>>> failure: ColorModel0004
>>>>>>
>>>>>>      Here they check for equality between 2 ColorModel objects
>>>>>> having same values, but it fails because now we are using identity-as-equals check in ColorModel.
>>>>> How do they accomplish this when the CM class is abstract?  Do
>>>>> they create a relatively empty subclass and instantiate that?
>>>>>
>>>>> The documentation for the equals() method does not document the
>>>>> conditions under which it returns true, it uses a vague concept of "equals this ColorModel".  I don't see how they could test anything given that documentation.
>>>>>
>>>>>>      3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>>> failure: ColorModel2006
>>>>>>
>>>>>>      Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we
>>>>>> don't have hashCode check in ColorModel and it     will be different between 2 Objects.
>>>>> Same as above, there are no promises documented.
>>>>>
>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTe
>>>>>> s
>>>>>> t
>>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>>> failed: 1; first test case failure: testCase1
>>>>>>
>>>>>>      Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the
>>>>>> first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK test tries to call getComponentSize()
>>>>>> with index 3 which throws ArrayIndexOutOfBoundsException.
>>>>> Same assessment as #1 above...
>>>>>
>>>>> Again, while these are my recommendations about the correctness of
>>>>> these tests, the question remains whether we want to introduce a
>>>>> change at this point in the release cycle that will essentially invalidate a number of tests that have been working for several releases already.  I'll leave that tactic issue to Phil...
>>>>>
>>>>>                  ...jim
>>>>>
>
> ------------------------------
>
> Message: 4
> Date: Thu, 9 Feb 2017 14:05:24 -0800
> From: Phil Race <[hidden email]>
> To: Jim Graham <[hidden email]>, Jayathirth D V
> <[hidden email]>, [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="windows-1252"; Format="flowed"
>
> Oh .. my reply was to an off-list email. I did not notice that.
> So I should repeat that here :
>
> On 2/9/17 12:38 PM, Phil Race wrote:
>> 32 import java.util.Objects;
>>
>> This is now un-used, isn't it ? Yet all 3 subclasses still have this
>> import.
>>
>> I don't need to "approve" a new webrev containing that but it would
>> be good to publish one.
>>
>> +1
> -phil.
>
>
> On 02/09/2017 01:59 PM, Jim Graham wrote:
>>  From my end this looks good.  +1 except for 2 outstanding review issues:
>>
>> - Would like to hear back final comments from Joe Darcy on the new
>> doc changes/CCC request
>> - Phil pointed out that there is an unneeded import in some of the
>> files.  I agree that we should make a final webrev to delete them,
>> but I don't need to approve it if that is the only change...
>>
>>              ...jim
>>
>> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>>> Hello All,
>>>
>>> There was a closed test which was failing because of
>>> identity-as-equals approach for ColorModel equals() method.
>>> I have modified it and added in the webrev. Along with this we are
>>> now using colorspace.hashCode() in hashCode() functions instead of
>>> Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in
>>> IndexColorModel equals() method because Arrays.copyOf() takes lot of
>>> time.
>>>
>>> Please find updated webrev for review :
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>>
>>> Ran jtreg test and JCK there are no additional test case failures
>>> because of the above change. Only 4 JCK tests are failing as it was
>>> happening previously.
>>>
>>> Just copy pasted my observation regarding JCK failures so that we
>>> can trace it easily:
>>>
>>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
>>> cases: 4; passed: 3; failed: 1; first test case failure:
>>> ColorModel2001
>>>
>>>      This test fails because getComponentSize() returned an array
>>> with length 3 but it expects the length to be 4. In the test case they
>>> have bits per component array     of length 4 like {8, 8, 8, 8}. But
>>> in the test case wherever they are passing "has Alpha" as "false" we
>>> omit the alpha component bit. This is because of tighter check that
>>> we have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>
>>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>> cases: 3; passed: 2; failed: 1; first test case failure:
>>> ColorModel0004
>>>
>>>      Here they check for equality between 2 ColorModel objects
>>> having same values, but it fails because now we are using
>>> identity-as-equals check in ColorModel.
>>>
>>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test
>>> cases: 2; passed: 1; failed: 1; first test case failure:
>>> ColorModel2006
>>>
>>>      Here they check for hashCode equality between 2 ColorModel
>>> objects having same values, but it fails since we don't have hashCode
>>> check in ColorModel and it     will be different between 2 Objects.
>>>
>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1:
>>> Failed. test cases: 2; passed: 1; failed: 1; first test case failure:
>>> testCase1
>>>
>>>      Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is
>>> also happening because of same reason as why the first JCK test is
>>> failing. We omit alpha bit if     hasAlpha is false but JCK test
>>> tries to call getComponentSize() with index 3 which throws
>>> ArrayIndexOutOfBoundsException.
>>>
>>> Thanks,
>>> Jay
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Wednesday, February 08, 2017 3:41 PM
>>> To: Jim Graham; Philip Race; [hidden email]
>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>>> ColorModel subclasses are missing hashCode() or equals() or both
>>> methods
>>>
>>> Hello All,
>>>
>>> I have updated the webrev to include the following changes.
>>>
>>>      1) Have identity as equals check in equals() method of
>>> ColorModel but elaborate the specification of equals() and hashCode() in
>>> ColorModel on what properties to          check in subclasses of
>>> ColorModel.
>>>      2) Made changes to test case to have single helper method
>>> wherever we have same equals/hashCode() check.
>>>      3) Updated IndexColorModel equals() method to use
>>> Arrays.equals() for rgb[] data.
>>>      4) Add comment on why we are not using validBits to calculate
>>> hashCode() in IndexColorModel hashCode() method.
>>>
>>> Please find updated webrev for review :
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Thursday, February 02, 2017 2:51 AM
>>> To: Phil Race; Jayathirth D V; [hidden email]
>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>>> ColorModel subclasses are missing hashCode() or equals() or both
>>> methods
>>>
>>> I think we should move this issue (array size returned from
>>> getCompSizes) into a separate bug entry and a separate fix.
>>> I don't think we need to fix the clone() in the constructor and the
>>> getter just to get hashcode/equals right...
>>>
>>>              ...jim
>>>
>>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>>> For an application to run into this same issue they'd have to
>>>> expect
>>>> getCompSizes() to return data for components that don't exist.  
>>>> It's unlikely they would use that data if they really understand
>>>> the objects.  While that would be odd, I guess I can see someone
>>>> might be constructing all of their CM's from an array of 4
>>>> components regardless of the number of actual components and we'd
>>>> be happily remembering the useless extra components and returning
>>>> an array of 4 from getCompSizes().  As I said, they shouldn't
>>>> really be reading and interpreting those extra components for any
>>>> image processing, but I can imagine that they might do something
>>>> like create a variant CM by calling the CompSizes() and copying
>>>> them into a new array to construct a new CM with modifications.  
>>>> They might just robotically always copy 4 values without really checking how many are valid.
>>>> That's a stretch, and their code is weak.  I can conceive of how
>>>> this might happen, but I really have no idea how likely it is...
>>>>
>>>>              ...jim
>>>>
>>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>>> Sounds like we should at least try to get the tests updated so
>>>>> they only test what the spec. says.
>>>>> Although it does indicate that there is at least a chance that
>>>>> application code might also fail due to similar assumptions.
>>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>>
>>>>> -phil.
>>>>>
>>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>>> Hmmm.  Sounds like the test cases were written based on bugs in
>>>>>> the implementation.  I'm not sure what the best tactic is here
>>>>>> for the short term for getting this in, but many of these changes
>>>>>> should eventually be considered bugs in the tests.  Is it
>>>>>> acceptable to break API tests like this at the last minute even
>>>>>> if the tests are at fault?  Phil?
>>>>>>
>>>>>> Notes on specific instances below...
>>>>>>
>>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>>> Hi Phil,
>>>>>>>
>>>>>>> 1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed.
>>>>>>> test cases: 4; passed: 3; failed: 1; first test case failure:
>>>>>>> ColorModel2001
>>>>>>>
>>>>>>>      This test fails because getComponentSize() returned an
>>>>>>> array with length 3 but it expects the length to be 4. In
>>>>>>> the test case they have bits per component array     of length 4
>>>>>>> like {8, 8, 8, 8}. But in the test case wherever they are
>>>>>>> passing "has Alpha" as "false" we omit the alpha
>>>>>>> component bit. This is because of tighter check     that we
>>>>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits,
>>>>>>> numComponents);" . "numComponents" will be 3 if hasAlpha is false.
>>>>>> This is a bug in the test then, especially if the size of our
>>>>>> array matches the return value of getNumComponents.
>>>>>>
>>>>>>> 2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test
>>>>>>> cases: 3; passed: 2; failed: 1; first test case
>>>>>>> failure: ColorModel0004
>>>>>>>
>>>>>>>      Here they check for equality between 2 ColorModel objects
>>>>>>> having same values, but it fails because now we are using
>>>>>>> identity-as-equals check in ColorModel.
>>>>>> How do they accomplish this when the CM class is abstract?  Do
>>>>>> they create a relatively empty subclass and instantiate that?
>>>>>>
>>>>>> The documentation for the equals() method does not document the
>>>>>> conditions under which it returns true, it uses a vague concept
>>>>>> of "equals this ColorModel".  I don't see how they could test
>>>>>> anything given that documentation.
>>>>>>
>>>>>>> 3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed.
>>>>>>> test cases: 2; passed: 1; failed: 1; first test case
>>>>>>> failure: ColorModel2006
>>>>>>>
>>>>>>>      Here they check for hashCode equality between 2 ColorModel
>>>>>>> objects having same values, but it fails since we
>>>>>>> don't have hashCode check in ColorModel and it     will be
>>>>>>> different between 2 Objects.
>>>>>> Same as above, there are no promises documented.
>>>>>>
>>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorT
>>>>>>> e
>>>>>>> st
>>>>>>> testCase1: Failed. test cases: 2; passed: 1;
>>>>>>> failed: 1; first test case failure: testCase1
>>>>>>>
>>>>>>>      Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This
>>>>>>> is also happening because of same reason as why the first JCK
>>>>>>> test is failing. We omit alpha bit if hasAlpha is false but JCK
>>>>>>> test tries to call getComponentSize() with index 3 which throws
>>>>>>> ArrayIndexOutOfBoundsException.
>>>>>> Same assessment as #1 above...
>>>>>>
>>>>>> Again, while these are my recommendations about the correctness
>>>>>> of these tests, the question remains whether we want to introduce
>>>>>> a change at this point in the release cycle that will essentially
>>>>>> invalidate a number of tests that have been working for several
>>>>>> releases already.  I'll leave that tactic issue to Phil...
>>>>>>
>>>>>>                  ...jim
>>>>>>
> -------------- next part -------------- An HTML attachment was
> scrubbed...
> URL:
> <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/c7
> 1a98ce/attachment.html>
>
> End of 2d-dev Digest, Vol 117, Issue 3
> **************************************
>
>
> End of 2d-dev Digest, Vol 117, Issue 4
> **************************************

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Dmitry Batrak
In reply to this post by Prahalad kumar Narayanan
Hi,

I've tried to build OpenJDK from 'client' repository with the fix, and the
issue is still reproducible for me. Are you fixing it only for Oracle JDK
(including Lucida Sans Regular font), and not for OpenJDK? The problem is not
that some character cannot be rendered using a particular font, it's that
rendering behaviour doesn't match what 'canDisplay' returns.

If you're interested I can share the way we've fixed this issue in our
OpenJDK-based runtime.

As you've mentioned, the problem is that
JRSFontCreateFallbackFontForCharacters returns fallback font, which is not on
the cascaded font list, and it can be absent even in the whole list of fonts
reported by the system. On my machine, with OpenJDK, it's .GeezaProInterface
(with name starting with dot). I guess such fonts are treated by macOS as
'hidden' (just like files with names starting with dots), still they seem to be
usable from application if requested by their name.

As JRSFontCreateFallbackFontForCharacters is a 'black box', it's hard to make
sure cascade list we're building matches its internal logic, so we've chosen to
build the list dynamically based on the output of
JRSFontCreateFallbackFontForCharacters itself. I can try to prepare
corresponding webrev, if you think that can be useful.

Alternative solution (and, probably, a better one strategically), would be not
using JRSFontCreateFallbackFontForCharacters at all. The cascade list, that is
built currently, can probably be used to perform font fallback in all cases.

Best regards,
Dmitry Batrak


> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to
> be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its
> creation mode
> in which we have added cascaded font list to find the real fonts that
> CFont uses, so that there is no need to use "negative" glyph code for
> finding the fallback fonts
> using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not
> take into account of using JRE provided fonts like all those Lucida* ttf
> in jdk/lib/fonts/, so
> when a glyph (in this intance, arabic 'alef' character) is intended to
> be rendered in Menlo font in italic style, osx will not be able to find
> the glyph in Menlo-Italic font
> and neither in all the cascaded system fonts provided by CoreText, so it
> results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the
> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> Regular" is, because it is the largest font file in jre and has all the
> glyph codepoints that no other font in the jre has,
> so we will not lose out on any codepoints and will help us in not
> getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/
>
> Regards
> Prasanta

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Philip Race
I think that so long as we are using JRS* in some places and CoreText's
cascading list
in others there is the likelihood of some inconsistency where JRS
introduces some magic
that we don't know about

So canDisplay() needs to be fixed to be consistent and perhaps we also
need to see
if we are always getting the glyphs from the same fonts for the cases
where there
is no missing glyph.

I do agree that getting away from JRS* support is the way to go so we
are in control.
However not at this time in JDK 9.

Yes "." fonts are not enumerated by OS X but if you can find them they work.
This is how the UI fonts are handled.

Yes, we are aware this fixed it only for Oracle JDK.
The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
So we could add that as well.
If you have any other fonts you think should be added to the tail of
that list we
can add them too.

-phil.

On 02/10/2017 07:59 AM, Dmitry Batrak wrote:

> Hi,
>
> I've tried to build OpenJDK from 'client' repository with the fix, and the
> issue is still reproducible for me. Are you fixing it only for Oracle JDK
> (including Lucida Sans Regular font), and not for OpenJDK? The problem
> is not
> that some character cannot be rendered using a particular font, it's that
> rendering behaviour doesn't match what 'canDisplay' returns.
>
> If you're interested I can share the way we've fixed this issue in our
> OpenJDK-based runtime.
>
> As you've mentioned, the problem is that
> JRSFontCreateFallbackFontForCharacters returns fallback font, which is
> not on
> the cascaded font list, and it can be absent even in the whole list of
> fonts
> reported by the system. On my machine, with OpenJDK, it's
> .GeezaProInterface
> (with name starting with dot). I guess such fonts are treated by macOS as
> 'hidden' (just like files with names starting with dots), still they
> seem to be
> usable from application if requested by their name.
>
> As JRSFontCreateFallbackFontForCharacters is a 'black box', it's hard
> to make
> sure cascade list we're building matches its internal logic, so we've
> chosen to
> build the list dynamically based on the output of
> JRSFontCreateFallbackFontForCharacters itself. I can try to prepare
> corresponding webrev, if you think that can be useful.
>
> Alternative solution (and, probably, a better one strategically),
> would be not
> using JRSFontCreateFallbackFontForCharacters at all. The cascade list,
> that is
> built currently, can probably be used to perform font fallback in all
> cases.
>
> Best regards,
> Dmitry Batrak
>
>
> > Hi All,
> >
> > Please review a fix for an issue which causes arabic character "alef" to
> > be not rendered in osx for menlo font in italic style.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
> >
> > The issue was actually a regression caused by the fix to JDK-7162125:
> > [macosx] A font has different behaviour for ligatures depending on its
> > creation mode
> > in which we have added cascaded font list to find the real fonts that
> > CFont uses, so that there is no need to use "negative" glyph code for
> > finding the fallback fonts
> > using the "subsititution"/"fallback" mechanism used by osx code.
> >
> > However, the above logic of using cascaded font list in CFont does not
> > take into account of using JRE provided fonts like all those Lucida* ttf
> > in jdk/lib/fonts/, so
> > when a glyph (in this intance, arabic 'alef' character) is intended to
> > be rendered in Menlo font in italic style, osx will not be able to find
> > the glyph in Menlo-Italic font
> > and neither in all the cascaded system fonts provided by CoreText, so it
> > results in empty box.
> >
> > Before 7162125 fix, the fallback code in
> > CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> > JRSFontCreateFallbackFontForCharacters()
> > was adding jre/lib/fonts to the fallback list which was causing the
> > glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> > rendered.
> >
> > So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> > to the cascaded list so that we get the "alef" glyph.
> > The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> > Regular" is, because it is the largest font file in jre and has all the
> > glyph codepoints that no other font in the jre has,
> > so we will not lose out on any codepoints and will help us in not
> > getting missing glyph.
> >
> > webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/ 
> <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
> >
> > Regards
> > Prasanta
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Dmitry Batrak
> Yes, we are aware this fixed it only for Oracle JDK.
> The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
> So we could add that as well.
> If you have any other fonts you think should be added to the tail of that list we
> can add them too.

I ran JRSFontCreateFallbackFontForCharacters against all Unicode code points with Menlo-Italic as base font, 
and got 63 possible fallback fonts - vs 29 present in cascade list, currently used by JDK 9 (given in JDK-8147002).
So the mismatch is quite large. But the interesting thing is that cascade list returned by 
CTFontCopyDefaultCascadeListForLanguages still covers that whole Unicode range. The problem is that JDK
doesn't use that list fully. In particular, Arabic alef letter U+0627 can be displayed using the font
.NotoSansUniversal from that list. This font doesn't get into composite font, as it's not found among the fonts
known to Java. So a better fix (and still quite small) will probably be using those 'hidden' fonts as part of
cascade composites - by creating a new, 'local' CFont instance if lookup in the known font list fails
(this, btw, seems to work fine for us). If even this is considered risky for JDK 9, then probably adding
.NotoSansUniversal with or instead of .GeezaProInterface as a hardcoded component makes sense.
 
Best regards,
Dmitry Batrak

On Fri, Feb 10, 2017 at 8:19 PM, Phil Race <[hidden email]> wrote:
I think that so long as we are using JRS* in some places and CoreText's cascading list
in others there is the likelihood of some inconsistency where JRS introduces some magic
that we don't know about

So canDisplay() needs to be fixed to be consistent and perhaps we also need to see
if we are always getting the glyphs from the same fonts for the cases where there
is no missing glyph.

I do agree that getting away from JRS* support is the way to go so we are in control.
However not at this time in JDK 9.

Yes "." fonts are not enumerated by OS X but if you can find them they work.
This is how the UI fonts are handled.

Yes, we are aware this fixed it only for Oracle JDK.
The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
So we could add that as well.
If you have any other fonts you think should be added to the tail of that list we
can add them too.

-phil.

On 02/10/2017 07:59 AM, Dmitry Batrak wrote:
Hi,

I've tried to build OpenJDK from 'client' repository with the fix, and the
issue is still reproducible for me. Are you fixing it only for Oracle JDK
(including Lucida Sans Regular font), and not for OpenJDK? The problem is not
that some character cannot be rendered using a particular font, it's that
rendering behaviour doesn't match what 'canDisplay' returns.

If you're interested I can share the way we've fixed this issue in our
OpenJDK-based runtime.

As you've mentioned, the problem is that
JRSFontCreateFallbackFontForCharacters returns fallback font, which is not on
the cascaded font list, and it can be absent even in the whole list of fonts
reported by the system. On my machine, with OpenJDK, it's .GeezaProInterface
(with name starting with dot). I guess such fonts are treated by macOS as
'hidden' (just like files with names starting with dots), still they seem to be
usable from application if requested by their name.

As JRSFontCreateFallbackFontForCharacters is a 'black box', it's hard to make
sure cascade list we're building matches its internal logic, so we've chosen to
build the list dynamically based on the output of
JRSFontCreateFallbackFontForCharacters itself. I can try to prepare
corresponding webrev, if you think that can be useful.

Alternative solution (and, probably, a better one strategically), would be not
using JRSFontCreateFallbackFontForCharacters at all. The cascade list, that is
built currently, can probably be used to perform font fallback in all cases.

Best regards,
Dmitry Batrak


> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to
> be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its
> creation mode
> in which we have added cascaded font list to find the real fonts that
> CFont uses, so that there is no need to use "negative" glyph code for
> finding the fallback fonts
> using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not
> take into account of using JRE provided fonts like all those Lucida* ttf
> in jdk/lib/fonts/, so
> when a glyph (in this intance, arabic 'alef' character) is intended to
> be rendered in Menlo font in italic style, osx will not be able to find
> the glyph in Menlo-Italic font
> and neither in all the cascaded system fonts provided by CoreText, so it
> results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the
> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> Regular" is, because it is the largest font file in jre and has all the
> glyph codepoints that no other font in the jre has,
> so we will not lose out on any codepoints and will help us in not
> getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/ <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>
> Regards
> Prasanta




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Philip Race
Hi,
I don't know what versions of MacOS X either of you (Dmitry+Prasanta) used but I am going to guess they are different
(can you both report these, for the record).

In https://bugs.openjdk.java.net/browse/JDK-8147002, Prasanta enumerates
the cascade list for the Italic case and I don't see .NotoSansUniversal anywhere on that list.

So although if enumerated it likely still won't work (since it is not enumerated by the other APIs),
there does not appear to be any font in the cascade list reported by Prasanta that "would not work",
so the fix still seems fine to me as it is plugging a gap.

A fix for using "hidden" fonts in the cascade list seems like a JDK 10 candidate unless there
is some critical case we are still missing.

-phil.



On 02/13/2017 01:56 AM, Dmitry Batrak wrote:
> Yes, we are aware this fixed it only for Oracle JDK.
> The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
> So we could add that as well.
> If you have any other fonts you think should be added to the tail of that list we
> can add them too.

I ran JRSFontCreateFallbackFontForCharacters against all Unicode code points with Menlo-Italic as base font, 
and got 63 possible fallback fonts - vs 29 present in cascade list, currently used by JDK 9 (given in JDK-8147002).
So the mismatch is quite large. But the interesting thing is that cascade list returned by 
CTFontCopyDefaultCascadeListForLanguages still covers that whole Unicode range. The problem is that JDK
doesn't use that list fully. In particular, Arabic alef letter U+0627 can be displayed using the font
.NotoSansUniversal from that list. This font doesn't get into composite font, as it's not found among the fonts
known to Java. So a better fix (and still quite small) will probably be using those 'hidden' fonts as part of
cascade composites - by creating a new, 'local' CFont instance if lookup in the known font list fails
(this, btw, seems to work fine for us). If even this is considered risky for JDK 9, then probably adding
.NotoSansUniversal with or instead of .GeezaProInterface as a hardcoded component makes sense.
 
Best regards,
Dmitry Batrak

On Fri, Feb 10, 2017 at 8:19 PM, Phil Race <[hidden email]> wrote:
I think that so long as we are using JRS* in some places and CoreText's cascading list
in others there is the likelihood of some inconsistency where JRS introduces some magic
that we don't know about

So canDisplay() needs to be fixed to be consistent and perhaps we also need to see
if we are always getting the glyphs from the same fonts for the cases where there
is no missing glyph.

I do agree that getting away from JRS* support is the way to go so we are in control.
However not at this time in JDK 9.

Yes "." fonts are not enumerated by OS X but if you can find them they work.
This is how the UI fonts are handled.

Yes, we are aware this fixed it only for Oracle JDK.
The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
So we could add that as well.
If you have any other fonts you think should be added to the tail of that list we
can add them too.

-phil.

On 02/10/2017 07:59 AM, Dmitry Batrak wrote:
Hi,

I've tried to build OpenJDK from 'client' repository with the fix, and the
issue is still reproducible for me. Are you fixing it only for Oracle JDK
(including Lucida Sans Regular font), and not for OpenJDK? The problem is not
that some character cannot be rendered using a particular font, it's that
rendering behaviour doesn't match what 'canDisplay' returns.

If you're interested I can share the way we've fixed this issue in our
OpenJDK-based runtime.

As you've mentioned, the problem is that
JRSFontCreateFallbackFontForCharacters returns fallback font, which is not on
the cascaded font list, and it can be absent even in the whole list of fonts
reported by the system. On my machine, with OpenJDK, it's .GeezaProInterface
(with name starting with dot). I guess such fonts are treated by macOS as
'hidden' (just like files with names starting with dots), still they seem to be
usable from application if requested by their name.

As JRSFontCreateFallbackFontForCharacters is a 'black box', it's hard to make
sure cascade list we're building matches its internal logic, so we've chosen to
build the list dynamically based on the output of
JRSFontCreateFallbackFontForCharacters itself. I can try to prepare
corresponding webrev, if you think that can be useful.

Alternative solution (and, probably, a better one strategically), would be not
using JRSFontCreateFallbackFontForCharacters at all. The cascade list, that is
built currently, can probably be used to perform font fallback in all cases.

Best regards,
Dmitry Batrak


> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to
> be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its
> creation mode
> in which we have added cascaded font list to find the real fonts that
> CFont uses, so that there is no need to use "negative" glyph code for
> finding the fallback fonts
> using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not
> take into account of using JRE provided fonts like all those Lucida* ttf
> in jdk/lib/fonts/, so
> when a glyph (in this intance, arabic 'alef' character) is intended to
> be rendered in Menlo font in italic style, osx will not be able to find
> the glyph in Menlo-Italic font
> and neither in all the cascaded system fonts provided by CoreText, so it
> results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the
> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> Regular" is, because it is the largest font file in jre and has all the
> glyph codepoints that no other font in the jre has,
> so we will not lose out on any codepoints and will help us in not
> getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/ <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>
> Regards
> Prasanta





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

prasanta sadhukhan

Hi Phil,

I was using osx10.11 but the list I enumerated in JBS is actually the composite font I get in CCompositeGlyphMapper#convertToGlyph() which contains those 29 slots [CCompositeGlyphMapper#convertToGlyph(unicode=value 1575) gets the glyph code(909) from slot[4] which is CourierNewPSMT, for italic the list was taken from convertToGlyph()]

But, if we are considering the cascade list "listOfString", it contains .NotoSansUniversal just before  AppleColorEmoji.

However, it does not get added to CompositeFont list because findFont2D() returned null for it. There was another hidden font in the cascade list ".AppleSymbolsFB" but it was changed to "AppleSymbols" and that along with other fonts findFont2D() was able to find and could add to CompositeFont list for convertGlyph() to use.

Regards
Prasanta
On 2/15/2017 10:40 PM, Phil Race wrote:
Hi,
I don't know what versions of MacOS X either of you (Dmitry+Prasanta) used but I am going to guess they are different
(can you both report these, for the record).

In https://bugs.openjdk.java.net/browse/JDK-8147002, Prasanta enumerates
the cascade list for the Italic case and I don't see .NotoSansUniversal anywhere on that list.

So although if enumerated it likely still won't work (since it is not enumerated by the other APIs),
there does not appear to be any font in the cascade list reported by Prasanta that "would not work",
so the fix still seems fine to me as it is plugging a gap.

A fix for using "hidden" fonts in the cascade list seems like a JDK 10 candidate unless there
is some critical case we are still missing.

-phil.



On 02/13/2017 01:56 AM, Dmitry Batrak wrote:
> Yes, we are aware this fixed it only for Oracle JDK.
> The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
> So we could add that as well.
> If you have any other fonts you think should be added to the tail of that list we
> can add them too.

I ran JRSFontCreateFallbackFontForCharacters against all Unicode code points with Menlo-Italic as base font, 
and got 63 possible fallback fonts - vs 29 present in cascade list, currently used by JDK 9 (given in JDK-8147002).
So the mismatch is quite large. But the interesting thing is that cascade list returned by 
CTFontCopyDefaultCascadeListForLanguages still covers that whole Unicode range. The problem is that JDK
doesn't use that list fully. In particular, Arabic alef letter U+0627 can be displayed using the font
.NotoSansUniversal from that list. This font doesn't get into composite font, as it's not found among the fonts
known to Java. So a better fix (and still quite small) will probably be using those 'hidden' fonts as part of
cascade composites - by creating a new, 'local' CFont instance if lookup in the known font list fails
(this, btw, seems to work fine for us). If even this is considered risky for JDK 9, then probably adding
.NotoSansUniversal with or instead of .GeezaProInterface as a hardcoded component makes sense.
 
Best regards,
Dmitry Batrak

On Fri, Feb 10, 2017 at 8:19 PM, Phil Race <[hidden email]> wrote:
I think that so long as we are using JRS* in some places and CoreText's cascading list
in others there is the likelihood of some inconsistency where JRS introduces some magic
that we don't know about

So canDisplay() needs to be fixed to be consistent and perhaps we also need to see
if we are always getting the glyphs from the same fonts for the cases where there
is no missing glyph.

I do agree that getting away from JRS* support is the way to go so we are in control.
However not at this time in JDK 9.

Yes "." fonts are not enumerated by OS X but if you can find them they work.
This is how the UI fonts are handled.

Yes, we are aware this fixed it only for Oracle JDK.
The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
So we could add that as well.
If you have any other fonts you think should be added to the tail of that list we
can add them too.

-phil.

On 02/10/2017 07:59 AM, Dmitry Batrak wrote:
Hi,

I've tried to build OpenJDK from 'client' repository with the fix, and the
issue is still reproducible for me. Are you fixing it only for Oracle JDK
(including Lucida Sans Regular font), and not for OpenJDK? The problem is not
that some character cannot be rendered using a particular font, it's that
rendering behaviour doesn't match what 'canDisplay' returns.

If you're interested I can share the way we've fixed this issue in our
OpenJDK-based runtime.

As you've mentioned, the problem is that
JRSFontCreateFallbackFontForCharacters returns fallback font, which is not on
the cascaded font list, and it can be absent even in the whole list of fonts
reported by the system. On my machine, with OpenJDK, it's .GeezaProInterface
(with name starting with dot). I guess such fonts are treated by macOS as
'hidden' (just like files with names starting with dots), still they seem to be
usable from application if requested by their name.

As JRSFontCreateFallbackFontForCharacters is a 'black box', it's hard to make
sure cascade list we're building matches its internal logic, so we've chosen to
build the list dynamically based on the output of
JRSFontCreateFallbackFontForCharacters itself. I can try to prepare
corresponding webrev, if you think that can be useful.

Alternative solution (and, probably, a better one strategically), would be not
using JRSFontCreateFallbackFontForCharacters at all. The cascade list, that is
built currently, can probably be used to perform font fallback in all cases.

Best regards,
Dmitry Batrak


> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to
> be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its
> creation mode
> in which we have added cascaded font list to find the real fonts that
> CFont uses, so that there is no need to use "negative" glyph code for
> finding the fallback fonts
> using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not
> take into account of using JRE provided fonts like all those Lucida* ttf
> in jdk/lib/fonts/, so
> when a glyph (in this intance, arabic 'alef' character) is intended to
> be rendered in Menlo font in italic style, osx will not be able to find
> the glyph in Menlo-Italic font
> and neither in all the cascaded system fonts provided by CoreText, so it
> results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the
> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> Regular" is, because it is the largest font file in jre and has all the
> glyph codepoints that no other font in the jre has,
> so we will not lose out on any codepoints and will help us in not
> getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/ <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>
> Regards
> Prasanta






Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR JDK-8147002:[macosx] Arabic character cannot be rendered on MacOS X

Dmitry Batrak
I used macOS 10.12.3 and I got the same cascade list for Menlo-Italic as Prasanta mentioned (with .AppleSymbolsFB substituted and .NotoSansUniversal omitted in Java code).

If it's not possible to add .NotoSansUniversal now, some Unicode ranges, apart from Arabic, will still remain not covered in OpenJDK builds (e.g. Ethiopic and Dingbats) for the Menlo-Italic case. Don't know whether that can be considered critical. Other non-hidden fonts can probably be added to cover those ranges as well, if needed.

Best regards,
Dmitry Batrak

On Thu, Feb 16, 2017 at 8:33 AM, Prasanta Sadhukhan <[hidden email]> wrote:

Hi Phil,

I was using osx10.11 but the list I enumerated in JBS is actually the composite font I get in CCompositeGlyphMapper#convertToGlyph() which contains those 29 slots [CCompositeGlyphMapper#convertToGlyph(unicode=value 1575) gets the glyph code(909) from slot[4] which is CourierNewPSMT, for italic the list was taken from convertToGlyph()]

But, if we are considering the cascade list "listOfString", it contains .NotoSansUniversal just before  AppleColorEmoji.

However, it does not get added to CompositeFont list because findFont2D() returned null for it. There was another hidden font in the cascade list ".AppleSymbolsFB" but it was changed to "AppleSymbols" and that along with other fonts findFont2D() was able to find and could add to CompositeFont list for convertGlyph() to use.

Regards
Prasanta
On 2/15/2017 10:40 PM, Phil Race wrote:
Hi,
I don't know what versions of MacOS X either of you (Dmitry+Prasanta) used but I am going to guess they are different
(can you both report these, for the record).

In https://bugs.openjdk.java.net/browse/JDK-8147002, Prasanta enumerates
the cascade list for the Italic case and I don't see .NotoSansUniversal anywhere on that list.

So although if enumerated it likely still won't work (since it is not enumerated by the other APIs),
there does not appear to be any font in the cascade list reported by Prasanta that "would not work",
so the fix still seems fine to me as it is plugging a gap.

A fix for using "hidden" fonts in the cascade list seems like a JDK 10 candidate unless there
is some critical case we are still missing.

-phil.



On 02/13/2017 01:56 AM, Dmitry Batrak wrote:
> Yes, we are aware this fixed it only for Oracle JDK.
> The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
> So we could add that as well.
> If you have any other fonts you think should be added to the tail of that list we
> can add them too.

I ran JRSFontCreateFallbackFontForCharacters against all Unicode code points with Menlo-Italic as base font, 
and got 63 possible fallback fonts - vs 29 present in cascade list, currently used by JDK 9 (given in JDK-8147002).
So the mismatch is quite large. But the interesting thing is that cascade list returned by 
CTFontCopyDefaultCascadeListForLanguages still covers that whole Unicode range. The problem is that JDK
doesn't use that list fully. In particular, Arabic alef letter U+0627 can be displayed using the font
.NotoSansUniversal from that list. This font doesn't get into composite font, as it's not found among the fonts
known to Java. So a better fix (and still quite small) will probably be using those 'hidden' fonts as part of
cascade composites - by creating a new, 'local' CFont instance if lookup in the known font list fails
(this, btw, seems to work fine for us). If even this is considered risky for JDK 9, then probably adding
.NotoSansUniversal with or instead of .GeezaProInterface as a hardcoded component makes sense.
 
Best regards,
Dmitry Batrak

On Fri, Feb 10, 2017 at 8:19 PM, Phil Race <[hidden email]> wrote:
I think that so long as we are using JRS* in some places and CoreText's cascading list
in others there is the likelihood of some inconsistency where JRS introduces some magic
that we don't know about

So canDisplay() needs to be fixed to be consistent and perhaps we also need to see
if we are always getting the glyphs from the same fonts for the cases where there
is no missing glyph.

I do agree that getting away from JRS* support is the way to go so we are in control.
However not at this time in JDK 9.

Yes "." fonts are not enumerated by OS X but if you can find them they work.
This is how the UI fonts are handled.

Yes, we are aware this fixed it only for Oracle JDK.
The bit I missed is that GeezaProInterface was then used on OpenJDK by JRS.
So we could add that as well.
If you have any other fonts you think should be added to the tail of that list we
can add them too.

-phil.

On 02/10/2017 07:59 AM, Dmitry Batrak wrote:
Hi,

I've tried to build OpenJDK from 'client' repository with the fix, and the
issue is still reproducible for me. Are you fixing it only for Oracle JDK
(including Lucida Sans Regular font), and not for OpenJDK? The problem is not
that some character cannot be rendered using a particular font, it's that
rendering behaviour doesn't match what 'canDisplay' returns.

If you're interested I can share the way we've fixed this issue in our
OpenJDK-based runtime.

As you've mentioned, the problem is that
JRSFontCreateFallbackFontForCharacters returns fallback font, which is not on
the cascaded font list, and it can be absent even in the whole list of fonts
reported by the system. On my machine, with OpenJDK, it's .GeezaProInterface
(with name starting with dot). I guess such fonts are treated by macOS as
'hidden' (just like files with names starting with dots), still they seem to be
usable from application if requested by their name.

As JRSFontCreateFallbackFontForCharacters is a 'black box', it's hard to make
sure cascade list we're building matches its internal logic, so we've chosen to
build the list dynamically based on the output of
JRSFontCreateFallbackFontForCharacters itself. I can try to prepare
corresponding webrev, if you think that can be useful.

Alternative solution (and, probably, a better one strategically), would be not
using JRSFontCreateFallbackFontForCharacters at all. The cascade list, that is
built currently, can probably be used to perform font fallback in all cases.

Best regards,
Dmitry Batrak


> Hi All,
>
> Please review a fix for an issue which causes arabic character "alef" to
> be not rendered in osx for menlo font in italic style.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8147002
>
> The issue was actually a regression caused by the fix to JDK-7162125:
> [macosx] A font has different behaviour for ligatures depending on its
> creation mode
> in which we have added cascaded font list to find the real fonts that
> CFont uses, so that there is no need to use "negative" glyph code for
> finding the fallback fonts
> using the "subsititution"/"fallback" mechanism used by osx code.
>
> However, the above logic of using cascaded font list in CFont does not
> take into account of using JRE provided fonts like all those Lucida* ttf
> in jdk/lib/fonts/, so
> when a glyph (in this intance, arabic 'alef' character) is intended to
> be rendered in Menlo font in italic style, osx will not be able to find
> the glyph in Menlo-Italic font
> and neither in all the cascaded system fonts provided by CoreText, so it
> results in empty box.
>
> Before 7162125 fix, the fallback code in
> CoreTextSupport.m#CTS_CopyCTFallbackFontAndGlyphForJavaGlyphCode() uses
> JRSFontCreateFallbackFontForCharacters()
> was adding jre/lib/fonts to the fallback list which was causing the
> glyph to be found in "LucidaBrightRegular.ttf" font and the glyph was
> rendered.
>
> So, the proposed fix is to add jre provided font "Lucida Sans Regular"
> to the cascaded list so that we get the "alef" glyph.
> The reason for choosing "Lucida Sans Regular" over "Lucida Bright
> Regular" is, because it is the largest font file in jre and has all the
> glyph codepoints that no other font in the jre has,
> so we will not lose out on any codepoints and will help us in not
> getting missing glyph.
>
> webrev: http://cr.openjdk.java.net/~psadhukhan/8147002/webrev.00/ <http://cr.openjdk.java.net/%7Epsadhukhan/8147002/webrev.00/>
>
> Regards
> Prasanta







Loading...