RFR: 8240756: [macos] SwingSet2:TableDemo:Printed Japanese characters were garbled [v3]

[Toshio Nakamura]

On Tue, 4 Jan 2022 02:12:41 GMT, Phil Race <prr at openjdk.org> wrote:

>> Toshio Nakamura has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
>> 
>>  - 8240756: [macos] SwingSet2:TableDemo:Printed Japanese characters were garbled
>>  - revert previous proposal
>>  - Merge branch 'master' into 8240756
>>  - 2nd proposal
>>  - Revert previous change
>>  - Merge branch 'master' into 8240756
>>    merge master
>>  - 8240756: [macos] SwingSet2:TableDemo:Printed Japanese characters were garbled
>
> Did this bug appear in some release, or do you think it never worked ?
> 
> Although I don't expect 255 slots your code like
> if (gv.getGlyphCode(i) >= 0x1000000)
> 
> and
> int slot = gV.getGlyphCode(i) >> 24;
> 
> will break if they are ..
> 
> The logic in drawGlyphVector where both initial slot != 0 and the final slot are handled
> specially make this code more complex than desirable. Can you make the logic cleaner ?
> 
> The test relies heavily on internals. I'm not sure how long it will last.
> "Pixel" (not Pixcel) count is an indicator that the rendering is the same, but why not
> direct pixel to pixel comparison ?
> Also it isn't actually testing that Japanese is rendered. It is testing that the images are the same
> so if they are both "wrong" the test will still pass.
> 
> Did you ever look into why drawString did not need fixing too ?
> 
> Have you run other printing tests besides the new one to verify this all works ?
> 
> More importantly what on-screen testing have you done ?
> Or have you proved CTextPipe is only used in printing ?

Thank you for the comments, @prrace

> Did this bug appear in some release, or do you think it never worked ?

I think it never worked.

> Although I don't expect 255 slots your code like if (gv.getGlyphCode(i) >= 0x1000000)
> and int slot = gV.getGlyphCode(i) >> 24;
> will break if they are ..

Fixed. Thank you for pointing out.

> The logic in drawGlyphVector where both initial slot != 0 and the final slot are handled specially make this code more complex than desirable. Can you make the logic cleaner ?

Revised the logic.

> The test relies heavily on internals. I'm not sure how long it will last.

I agree. But, without using OSXOffScreenSurfaceData class, printing test is required. I think it's hard to create automate tests.

> "Pixel" (not Pixcel) count is an indicator that the rendering is the same, but why not direct pixel to pixel comparison ? Also it isn't actually testing that Japanese is rendered. It is testing that the images are the same so if they are both "wrong" the test will still pass.

I'm sorry for misspelling. I revised the testcase. I realized that fallback fonts are different between CoreText library and Java side. So, now the test will check the drawing bounds of test string by drawGlyphVector. Also, if there is no capable font, it will be skipped.

> Did you ever look into why drawString did not need fixing too ?

I'm not fully understanding CoreText library, but CTLine object picked up automatically fallback fonts by Unicode data.

> Have you run other printing tests besides the new one to verify this all works ?

Yes. We've tested some printing based on J2Ddemo and SwingSet2 with non-ASCII characters.

> More importantly what on-screen testing have you done ? Or have you proved CTextPipe is only used in printing ?

I wasn't able to find the way to call the method except the scenario in the original report, which is to print JTable with non-ASCII characters. If this method had been used more, more people would have found it.

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

PR: https://git.openjdk.java.net/jdk/pull/3619