8165943: LineBreakMeasurer does not measure correctly if TextAttribute.TRACKING is set. by omikhaltsova · Pull Request #10289 · openjdk/jdk

omikhaltsova · 2022-09-15T16:51:27Z

This is a fix for LineBreakMeasurer. It takes into account the TextAttribute.TRACKING value (not eq 0) while calculating the line breaks.

Tested on Linux x64, Windows x64, macOS x64 with the reproducer (LineBreakSample.java) attached to JDK-8165943 and the following group of tests:
$JTREG_HOME/bin/jtreg -jdk:$BUILD_HOME ./test/jdk/java/awt/font

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8165943: LineBreakMeasurer does not measure correctly if TextAttribute.TRACKING is set.

Reviewers

@kiraka42 (no known github.com user name / role) ⚠️ Review applies to 3564751d
Phil Race (@prrace - Reviewer)

Contributors

Jason Fordham <jclf@azul.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10289/head:pull/10289
$ git checkout pull/10289

Update a local copy of the PR:
$ git checkout pull/10289
$ git pull https://git.openjdk.org/jdk pull/10289/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10289

View PR using the GUI difftool:
$ git pr show -t 10289

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10289.diff

…e.TRACKING is set.

bridgekeeper · 2022-09-15T16:53:48Z

👋 Welcome back omikhaltcova! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

omikhaltsova · 2022-09-15T16:55:39Z

/contributor add Jason Fordham jclf@azul.com

openjdk · 2022-09-15T16:56:03Z

@omikhaltsova
Contributor Jason Fordham <jclf@azul.com> successfully added.

openjdk · 2022-09-15T16:57:57Z

@omikhaltsova The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-09-15T20:01:17Z

Webrevs

bridgekeeper · 2022-10-13T23:06:20Z

@omikhaltsova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

omikhaltsova · 2022-11-08T16:09:09Z

gentle ping; pls review this pr!

omikhaltsova · 2022-11-18T14:00:50Z

@prrace Could you please take a look at the proposed changes! Is this fix needed or not? It fixes JDK-8165943/JDK-6598756. I opened PR on JDK-8165943 because it had more active conversation. But maybe I made a mistake and it should be opened on JDK-6598756 (created by you) because it was created earlier then JDK-8165943?

prrace · 2022-11-20T03:39:56Z

I haven't had time to look at this. Maybe I can look at it some time in the 1st week of December. Ping me then.

omikhaltsova · 2022-11-21T10:44:25Z

Thanks! I'll ping you in 1,5 weeks.

azul-jf · 2022-11-21T14:50:55Z

Hi @prrace, this came to our attention as a customer issue. The reproducer we used was the one from JDK-8165943.

As a reminder of the issue and an illustration of the desired outcome, here are some screen captures.

Existing code, TRACKING = 0.0:

Existing code, TRACKING = 0.1:

Submitted code, TRACKING = 0.1:

mlbridge · 2022-11-22T20:24:48Z

Mailing list message from Patrick Chen on client-libs-dev:

I think it is good
reviewed

Le lun. 21 nov. 2022 ? 16:08, azul-jf <duke at openjdk.org> a ?crit :

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20221122/c90ebeb2/attachment.htm>

jayathirthrao · 2022-11-23T08:37:31Z

Change looks okay to me.
Verified the fix locally and line break works properly with tracking now.

Attached image with tracking 0.1 before and after fix.

Before fix :

After fix :

Also client test run in our CI with this fix is green.
Let's wait for Phil's review also.

jayathirthrao · 2022-11-23T08:40:09Z

Its better if we can add jtreg(regression) test along with fix.
It can be a manual test, if we can't automate it.

prrace · 2022-11-29T23:02:18Z

I'm sure an automated test is possible - it should be easy enough to adjust the tracking and verify the breaks are different.
The code paths here don't do complex text. Tracking doesn't really work for things like Arabic .. but it would be good to
verify that Arabic measures as it lays out with tracking - ignoring it I expect, but I could be surprised if there's something I am overlooking. Hence why a test of that would be good.

prrace · 2022-12-01T20:16:19Z

test/jdk/java/awt/font/LineBreakMeasurer/LineBreakWithTracking.java

+/*
+  @test
+  @key headful
+  @bug 8165943


headful isn't needed on manual tests.

Also whilst you can keep this manual test, I still think an automated test that verifies tracking is making a difference to the advance should be provided.

prrace · 2022-12-01T20:18:06Z

test/jdk/java/awt/font/LineBreakMeasurer/LineBreakWithTracking.java

+    if (lineMeasurer == null) {
+      Float regular = new Float(16.0);
+      Float big = new Float(24.0);
+


LineBreakWithTracking.java:74: warning: [removal] Float(double) in Float has been deprecated and marked for removal
Float regular = new Float(16.0);
^
LineBreakWithTracking.java:75: warning: [removal] Float(double) in Float has been deprecated and marked for removal
Float big = new Float(24.0);

Use Float.valueOf(float) instead

prrace · 2022-12-01T20:26:26Z

test/jdk/java/awt/font/LineBreakMeasurer/LineBreakWithTracking.java

+    JButton btn = new JButton("Pass");
+    btn.addActionListener(new ActionListener(){
+      public void actionPerformed(ActionEvent e){
+        System.exit(0);


Do not call System.exit() in jtreg tests - even manual ones or ones that run in their own VM.
Instead dispose() the UI and then either let main() exit normally or throw RuntimeException() to indicate failure.

Also be sure that ALL exit paths clean up windows.

You may want to use PassFailJFrame.java to help you avoid writing the boilerplate and to get some of these things right

https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/regtesthelpers/PassFailJFrame.java

omikhaltsova · 2022-12-06T21:30:54Z

The following fixes are made:

fixed the lines breaks calculation for the negative tracking values;
included an epsilon for floating point operation;
fixed the manual test according to the above comments;
added a simple automated test similar to TestLineBreakWithFontSub.

prrace · 2022-12-06T22:26:38Z

test/jdk/java/awt/font/LineBreakMeasurer/LineBreakWithTracking.java

+class LineBreakPanel extends JPanel implements ActionListener {
+
+  private float textTracking = 0.0f;
+  private static String fontName = "Bitstream Cyberbit";


Did you previously use this font ? I must have over-looked that.
Do not use fonts that aren't available on the OS .. no SQE person will ever install - or even know a test uses it - and JDK will just use Dialog instead and people will get confused over why their experience is different than yours.
And of course each OS (desktop) will have different fonts. Overall best to just pick a logical font. If you want to know if the font supports all your code points you can iterate over your text using Font.canDisplay(int codePoint)

I'll submit a test job using your automated test.

I noticed the usage of this font "Bitstream Cyberbit" in TestJustification.java that was added by you. I used the test string from there because it includes Latin, Arabic, CJK and Hebrew. "Bitstream Cyberbit" has all nessesary symbols used in the test string. At the moment I have no idea what font I can use instead.

I'm pretty sure it wasn't me who created that.
I may have updated it at some point (actually I suspect what you see is that I moved
it from closed to open) but that test was created by someone else 23 years ago (1999).
Different world back then. Likely it should not use the font either.

Your best best is to use Dialog. On all platforms where a decent set of fonts is installed then all those scripts will be supported by Dialog.

prrace

This now is fine by me. It didn't break any of our automated tests.

openjdk · 2022-12-07T16:37:24Z

@omikhaltsova This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8165943: LineBreakMeasurer does not measure correctly if TextAttribute.TRACKING is set.

Co-authored-by: Jason Fordham <jclf@azul.com>
Reviewed-by: prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1191 new commits pushed to the master branch:

3934484: 8298205: Prefer Member Initialization Lists for JFR classes in os_perf.hpp
389b8f4: 8297298: SequenceInputStream should override transferTo
dd7385d: 8298202: [AIX] Dead code elimination removed jfr constructor used by AIX
29f1c3c: 8298274: Problem list TestSPISigned on Windows
3de7750: 8298177: Various java.lang.invoke cleanups
6ed3683: 8297209: Serial: Refactor GenCollectedHeap::full_process_roots
86270e3: 8269820: C2 PhaseIdealLoop::do_unroll get wrong opaque node
cf63f2e: 8298184: Incorrect record component type in record patterns
58170f6: 8298035: Provide better descriptions for JIT compiler JFR events
bfcc238: 8297964: Jetty.java fails "assert(_no_handle_mark_nesting == 0) failed: allocating handle inside NoHandleMark"
... and 1181 more: https://git.openjdk.org/jdk/compare/cea409cc2822ccdc9cbf6df04d46742e3c73b0fe...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

omikhaltsova · 2022-12-07T17:40:57Z

/integrate

omikhaltsova · 2022-12-07T17:41:56Z

Thanks a lot for reviewing!

openjdk · 2022-12-07T17:43:53Z

@omikhaltsova
Your change (at version 34764d8) is now ready to be sponsored by a Committer.

bae · 2022-12-07T17:58:55Z

/sponsor

openjdk · 2022-12-07T18:02:35Z

Going to push as commit 8edb98d.
Since your change was applied there have been 1191 commits pushed to the master branch:

3934484: 8298205: Prefer Member Initialization Lists for JFR classes in os_perf.hpp
389b8f4: 8297298: SequenceInputStream should override transferTo
dd7385d: 8298202: [AIX] Dead code elimination removed jfr constructor used by AIX
29f1c3c: 8298274: Problem list TestSPISigned on Windows
3de7750: 8298177: Various java.lang.invoke cleanups
6ed3683: 8297209: Serial: Refactor GenCollectedHeap::full_process_roots
86270e3: 8269820: C2 PhaseIdealLoop::do_unroll get wrong opaque node
cf63f2e: 8298184: Incorrect record component type in record patterns
58170f6: 8298035: Provide better descriptions for JIT compiler JFR events
bfcc238: 8297964: Jetty.java fails "assert(_no_handle_mark_nesting == 0) failed: allocating handle inside NoHandleMark"
... and 1181 more: https://git.openjdk.org/jdk/compare/cea409cc2822ccdc9cbf6df04d46742e3c73b0fe...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-07T18:03:06Z

@bae @omikhaltsova Pushed as commit 8edb98d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8165943: LineBreakMeasurer does not measure correctly if TextAttribut…

88d90df

…e.TRACKING is set.

openjdk bot added the client client-libs-dev@openjdk.org label Sep 15, 2022

Removed unnecessary comment

3564751

omikhaltsova marked this pull request as ready for review September 15, 2022 19:56

openjdk bot added the rfr Pull request is ready for review label Sep 15, 2022

kiraka42 approved these changes Nov 23, 2022

View reviewed changes

omikhaltsova added 2 commits December 1, 2022 21:14

Manual test

caf32d5

Fix tracking for CJK, Arabic and Hebrew

1818c7d

prrace suggested changes Dec 1, 2022

View reviewed changes

omikhaltsova added 3 commits December 7, 2022 00:05

Fix manual test

a00c282

Fix for negative tracking and float

d713810

Added an automated test

b1b4b14

openjdk bot removed the rfr Pull request is ready for review label Dec 6, 2022

Deleted trailing whitespaces, added new line at the end

c136384

openjdk bot added the rfr Pull request is ready for review label Dec 6, 2022

prrace reviewed Dec 6, 2022

View reviewed changes

omikhaltsova added 2 commits December 7, 2022 18:58

Replaced BitstreamCyberbit with Dialog

b0d44ce

Deleted empty line inserted by mistake

34764d8

prrace approved these changes Dec 7, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 7, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 7, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 7, 2022

openjdk bot closed this Dec 7, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 7, 2022

Conversation

omikhaltsova commented Sep 15, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Sep 15, 2022

Uh oh!

omikhaltsova commented Sep 15, 2022

Uh oh!

openjdk bot commented Sep 15, 2022

Uh oh!

openjdk bot commented Sep 15, 2022

Uh oh!

mlbridge bot commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

bridgekeeper bot commented Oct 13, 2022

Uh oh!

omikhaltsova commented Nov 8, 2022

Uh oh!

omikhaltsova commented Nov 18, 2022

Uh oh!

prrace commented Nov 20, 2022

Uh oh!

omikhaltsova commented Nov 21, 2022

Uh oh!

azul-jf commented Nov 21, 2022 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 22, 2022

Uh oh!

jayathirthrao commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayathirthrao commented Nov 23, 2022

Uh oh!

prrace commented Nov 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omikhaltsova commented Dec 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prrace left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omikhaltsova commented Dec 7, 2022

Uh oh!

omikhaltsova commented Dec 7, 2022

Uh oh!

openjdk bot commented Dec 7, 2022

Uh oh!

bae commented Dec 7, 2022

Uh oh!

openjdk bot commented Dec 7, 2022

Uh oh!

openjdk bot commented Dec 7, 2022

omikhaltsova commented Sep 15, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 15, 2022 •

edited

Loading

azul-jf commented Nov 21, 2022 •

edited by bridgekeeper bot

Loading

jayathirthrao commented Nov 23, 2022 •

edited

Loading

openjdk bot commented Dec 7, 2022 •

edited

Loading