8302511: HitInfo.toString() throws IllegalArgumentException #1154

andy-goryachev-oracle · 2023-06-14T15:40:41Z

A recent change in JDK-8304831 ensured that insertionIndex is always computed prior to HitInfo construction, making it possible to remove buggy logic in HitInfo. This change also allows for removal of the 'text' field.

There should be no impact since the constructor is package protected.

Progress

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

Issue

JDK-8302511: HitInfo.toString() throws IllegalArgumentException (Bug - P4)

Reviewers

Karthik P K (@karthikpandelu - Committer)
Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1154

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1154.diff

Webrev

Link to Webrev Comment

…t cases

bridgekeeper · 2023-06-14T15:41:26Z

👋 Welcome back angorya! 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.

mlbridge · 2023-06-14T16:13:26Z

Webrevs

kevinrushforth · 2023-06-14T16:16:47Z

Reviewers: @prrace @karthikpandelu

/reviewers 2

openjdk · 2023-06-14T16:17:00Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

kevinrushforth · 2023-06-14T16:18:55Z

The fix looks good to me, but I'll let Phil and Karthik formally review it.

karthikpandelu

The fix looks good to me.
Can we add a test to verify the changes?
Another minor comment: copyright year can be updated in Text.java as well.

andy-goryachev-oracle · 2023-06-15T18:39:54Z

thank you for the suggestions, @karthikpandelu !

modified TextFlowSurrogatePaitInsertionIndexTest to test for invariants
added a similar test for Text
updated copyright year

karthikpandelu

Thanks for updating the test and adding new test @andy-goryachev-oracle.
Please check the jcheck failure.
I have one minor comment added inline.

karthikpandelu · 2023-06-16T08:34:29Z

tests/system/src/test/java/test/robot/javafx/scene/TextSurrogatePairInsertionIndexTest.java

+    @BeforeClass
+    public static void initFX() {
+        long seed = new Random().nextLong();
+        System.out.println("seed=" + seed);


Do we need this print statement?

yes. I've added a brief comment, but let me explain in more detail:

these tests use random patterns to avoid hitting the same values each time. for example, in TextFlowSurrogatePairInsertionIndexTest many tests incremented the x coordinate by 5 prior to this change, but what if we had an error condition happening with x = 21?. the old test never hit that value, but the new one might (at some point). so we effectively extending the test coverage every time the test runs.

and, if we ever encounter a failure, it will be possible to reproduce the exact failure scenario by setting the same seed as found in the failed log.

Thanks for the details @andy-goryachev-oracle

aghaisas

LGTM.

openjdk · 2023-06-23T10:43:56Z

@andy-goryachev-oracle 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:

8302511: HitInfo.toString() throws IllegalArgumentException

Reviewed-by: kpk, aghaisas

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 7 new commits pushed to the master branch:

c224615: 8180066: Convert package.html files to package-info.java
a17a714: 8299348: Size-restricted window can be observed with incorrect dimensions
77c43e0: 8304290: Some JNI calls made without checking exceptions in media
0d9dcf3: 8264449: Enable reproducible builds with SOURCE_DATE_EPOCH
4232183: 8307542: Call to FcConfigAppFontAddFile uses wrong prototype, arguments
7eb9a1c: 8306121: Scene not rendered initially when changing scenes after fix for JDK-8296621
8d13ba9: 8310024: Skip failing scene change tests on macOS

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

andy-goryachev-oracle · 2023-06-23T15:34:36Z

/integrate

openjdk · 2023-06-23T15:36:01Z

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

152a43e: 8310667: Normalize comment blocks in newly-converted package-info.java files
c224615: 8180066: Convert package.html files to package-info.java
a17a714: 8299348: Size-restricted window can be observed with incorrect dimensions
77c43e0: 8304290: Some JNI calls made without checking exceptions in media
0d9dcf3: 8264449: Enable reproducible builds with SOURCE_DATE_EPOCH
4232183: 8307542: Call to FcConfigAppFontAddFile uses wrong prototype, arguments
7eb9a1c: 8306121: Scene not rendered initially when changing scenes after fix for JDK-8296621
8d13ba9: 8310024: Skip failing scene change tests on macOS

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-23T15:36:11Z

@andy-goryachev-oracle Pushed as commit d2683b9.

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

karthikpandelu and others added 11 commits April 14, 2023 12:40

Fix insertion index calculation issue in TextFlow

f741feb

Fix insertion index calculation issue with emojis. Add additional tes…

e35f5bf

…t cases

Address code review

0360055

Initialize insertion index in PrismTextLayout

7919d5b

Change insertion index initialization approach. Add additional test

1eb1174

Add new test. Optimizations to make the tests robust

a43bbdb

Stabilizing the system tests

6684075

Merge master to resolve merge conflict

4d6dd9f

8194704: Text/TextFlow hitTest() javadoc

570a977

8302511: HitInfo.toString() throws IllegalArgumentException

fcd4879

Merge branch 'master' into 8302511.hit.info

8fa98c0

no change

c2685ba

andy-goryachev-oracle marked this pull request as ready for review June 14, 2023 16:06

openjdk bot added the rfr Ready for review label Jun 14, 2023

karthikpandelu reviewed Jun 15, 2023

View reviewed changes

review comments

8014ba3

openjdk bot removed the rfr Ready for review label Jun 15, 2023

karthikpandelu suggested changes Jun 16, 2023

View reviewed changes

whitespace

d5fdff4

openjdk bot added the rfr Ready for review label Jun 16, 2023

karthikpandelu approved these changes Jun 19, 2023

View reviewed changes

aghaisas approved these changes Jun 23, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jun 23, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 23, 2023

openjdk bot closed this Jun 23, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jun 23, 2023

andy-goryachev-oracle deleted the 8302511.hit.info branch June 23, 2023 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8302511: HitInfo.toString() throws IllegalArgumentException #1154

8302511: HitInfo.toString() throws IllegalArgumentException #1154

andy-goryachev-oracle commented Jun 14, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 14, 2023

mlbridge bot commented Jun 14, 2023 •

edited

Loading

kevinrushforth commented Jun 14, 2023

openjdk bot commented Jun 14, 2023

kevinrushforth commented Jun 14, 2023

karthikpandelu left a comment

andy-goryachev-oracle commented Jun 15, 2023

karthikpandelu left a comment

karthikpandelu Jun 16, 2023

andy-goryachev-oracle Jun 16, 2023

karthikpandelu Jun 19, 2023

aghaisas left a comment

openjdk bot commented Jun 23, 2023

andy-goryachev-oracle commented Jun 23, 2023

openjdk bot commented Jun 23, 2023

openjdk bot commented Jun 23, 2023

8302511: HitInfo.toString() throws IllegalArgumentException #1154

8302511: HitInfo.toString() throws IllegalArgumentException #1154

Conversation

andy-goryachev-oracle commented Jun 14, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 14, 2023

mlbridge bot commented Jun 14, 2023 • edited Loading

Webrevs

kevinrushforth commented Jun 14, 2023

openjdk bot commented Jun 14, 2023

kevinrushforth commented Jun 14, 2023

karthikpandelu left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Jun 15, 2023

karthikpandelu left a comment

Choose a reason for hiding this comment

karthikpandelu Jun 16, 2023

Choose a reason for hiding this comment

andy-goryachev-oracle Jun 16, 2023

Choose a reason for hiding this comment

karthikpandelu Jun 19, 2023

Choose a reason for hiding this comment

aghaisas left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 23, 2023

andy-goryachev-oracle commented Jun 23, 2023

openjdk bot commented Jun 23, 2023

openjdk bot commented Jun 23, 2023

andy-goryachev-oracle commented Jun 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 14, 2023 •

edited

Loading