8293587: Fix mistakes in FX API docs #1025

nlisker · 2023-02-06T23:00:17Z

Fixes and cleanup in the areas in the linked issue.

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-8293587: Fix mistakes in FX API docs

Reviewers

John Hendrikx (@hjohn - Committer)
Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1025

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

Using diff file

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

bridgekeeper · 2023-02-06T23:00:36Z

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

nlisker · 2023-02-06T23:01:29Z

Integrating can wait until a bit before the release to allow for more mistakes to be included.

mlbridge · 2023-02-06T23:05:32Z

Webrevs

kevinrushforth · 2023-02-06T23:17:13Z

/reviewers 2

openjdk · 2023-02-06T23:18:14Z

@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).

hjohn

Looks good to me, I added some more minor suggestions as well.

modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java

modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java

kevinrushforth

Other than John's comments, this looks good to me. I noted one more possible formatting change and an unrelated follow-up cleanup issue I plan to file.

kevinrushforth · 2023-02-07T12:17:43Z

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

        private void processDropEnd(DragEvent de) {
            if (source == null) {
-                System.out.println("Scene.DnDGesture.processDropEnd() - UNEXPECTD - source is NULL");
+                System.out.println("Scene.DnDGesture.processDropEnd() - UNEXPECTED - source is NULL");


This reminds me... I see that this warning is printed to System.out. I plan to file a follow-up cleanup bug (not for jfx20) to fix this, here and in other places, since runtime warnings should be printed to System.err (or else logged using the platform logger, but that would be a larger change, so I expect we'll opt for the simple substitution).

I never understood when something should be logged vs. printed. And do we ever print to out?

I wish we were using logging...

We aren't very consistent on this. Most of the places where we produce warnings, or where we provide verbose output, are done by printing. At a minimum, those should go to System.err rather than System.out. Unless there is an API whose expected result is to print something to System.out, a library really shouldn't print to System.out (as opposed to a test or a demo app, where it's fine). So the only places I would expect to see it in the library -- meaning source code under modules/javafx.*/src/main/java/ -- would be in examples in javadoc comments.

We could file a second follow-up issue (RFE) to consider changing some of our print statements to use logging.

Some investigation is needed if source can ever be null in such cases. I think some of these are to inform the programmer (not the user) something unexpected happens, in which case it should be an assert or exception. If it then ever does happen, it should be fixed.

The drag/drop code is full of what looks like asserts that are just dumped to System.err. It looks like left overs that helped during development of the underlying state machine, but it shouldn't get in such states normally. When I run the tests, none of the exceptional cases or System.err code has any coverage which can be a sign that these cases can't be reached.

modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java

…tView.java Co-authored-by: John Hendrikx <john.hendrikx+github@gmail.com>

…ow.java Co-authored-by: John Hendrikx <john.hendrikx+github@gmail.com>

andy-goryachev-oracle · 2023-02-07T19:31:58Z

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

        private void processDropEnd(DragEvent de) {
            if (source == null) {
-                System.out.println("Scene.DnDGesture.processDropEnd() - UNEXPECTD - source is NULL");
+                System.out.println("Scene.DnDGesture.processDropEnd() - UNEXPECTED - source is NULL");


I wish we were using logging...

modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java

modules/javafx.graphics/src/main/java/javafx/scene/text/TextFlow.java

…ow.java

andy-goryachev-oracle

definitely an improvement, thank you.

openjdk · 2023-02-08T01:15:43Z

@nlisker 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:

8293587: Fix mistakes in FX API docs

Reviewed-by: jhendrikx, kcr, angorya

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

abe8058: 8301832: InputMethodEvents are not enabled for text input controls
192ba4f: 8301797: Pagination control has the wrong size

Please see this link for an up-to-date comparison between the source branch of this pull request and the jfx20 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 jfx20 branch, type /integrate in a new comment.

nlisker · 2023-02-24T14:03:38Z

/integrate

openjdk · 2023-02-24T14:05:19Z

Going to push as commit 7bf2372.
Since your change was applied there have been 2 commits pushed to the jfx20 branch:

abe8058: 8301832: InputMethodEvents are not enabled for text input controls
192ba4f: 8301797: Pagination control has the wrong size

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-24T14:05:35Z

@nlisker Pushed as commit 7bf2372.

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

Initial commit

7d422a9

openjdk bot added the rfr Ready for review label Feb 6, 2023

hjohn approved these changes Feb 7, 2023

View reviewed changes

kevinrushforth reviewed Feb 7, 2023

View reviewed changes

nlisker and others added 4 commits February 7, 2023 18:56

Update modules/javafx.controls/src/main/java/javafx/scene/control/Lis…

d2155e5

…tView.java Co-authored-by: John Hendrikx <john.hendrikx+github@gmail.com>

Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFl…

613ef6e

…ow.java Co-authored-by: John Hendrikx <john.hendrikx+github@gmail.com>

Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFl…

504b2af

…ow.java Co-authored-by: John Hendrikx <john.hendrikx+github@gmail.com>

Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFl…

fe9c988

…ow.java Co-authored-by: John Hendrikx <john.hendrikx+github@gmail.com>

andy-goryachev-oracle reviewed Feb 7, 2023

View reviewed changes

nlisker added 2 commits February 8, 2023 01:55

Update modules/javafx.graphics/src/main/java/javafx/scene/text/TextFl…

a563200

…ow.java

Addressed review comments

328bdeb

andy-goryachev-oracle approved these changes Feb 8, 2023

View reviewed changes

hjohn approved these changes Feb 8, 2023

View reviewed changes

kevinrushforth approved these changes Feb 8, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 8, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 24, 2023

openjdk bot closed this Feb 24, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 24, 2023

nlisker deleted the 8293587_Fix_mistakes_in_FX_API_docs branch February 24, 2023 14:05

8293587: Fix mistakes in FX API docs #1025

8293587: Fix mistakes in FX API docs #1025

Uh oh!

Conversation

nlisker commented Feb 6, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 6, 2023

Uh oh!

nlisker commented Feb 6, 2023

Uh oh!

mlbridge bot commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth commented Feb 6, 2023

Uh oh!

openjdk bot commented Feb 6, 2023

Uh oh!

hjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

nlisker Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

hjohn Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

hjohn Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andy-goryachev-oracle Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlisker commented Feb 24, 2023

Uh oh!

openjdk bot commented Feb 24, 2023

Uh oh!

openjdk bot commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

nlisker commented Feb 6, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 6, 2023 •

edited

Loading

openjdk bot commented Feb 8, 2023 •

edited

Loading