Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v5]
On Tue, 23 Aug 2022 16:34:59 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comment Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v4]
On Tue, 23 Aug 2022 15:36:35 GMT, Nir Lisker wrote:
>> Fixes the mistakes in the JBS ticket and some additional minor corrections.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressed review comments
Looks good with one optional suggestion.
modules/javafx.controls/src/main/java/javafx/scene/control/TextFormatter.java
line 50:
> 48: * setting a value will result in an {@code IllegalStateException} and
> the value is always {@code null}.
> 49: *
> 50: * Since {@code Formatter} contains a value which represents the state of
> the {@code TextInputControl} to which it is
One more thing I noticed: "contains a value which" could be changed to
"contains a value that"
-
Marked as reviewed by kcr (Lead).
PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v5]
> Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comment - Changes: - all: https://git.openjdk.org/jfx/pull/880/files - new: https://git.openjdk.org/jfx/pull/880/files/e9df3a27..c537fa93 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=880=04 - incr: https://webrevs.openjdk.org/?repo=jfx=880=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/880.diff Fetch: git fetch https://git.openjdk.org/jfx pull/880/head:pull/880 PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v4]
On Tue, 23 Aug 2022 15:36:35 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comments > When looking at `Service` I noticed that in `Worker` the property docs are on > the `get` methods instead of on the `property` methods ... > > This causes `Service` to inherit these as well. Docs should only appear on > the property (except in special circumstances), but I didn't want to touch > all of this here. Should I create a new issue for this or is it not worth it? I recommend to file a new issue. We can target it for JavaFX 20. - PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v4]
On Tue, 23 Aug 2022 15:36:35 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comments When looking at `Service` I noticed that in `Worker` the property docs are on the `get` methods instead of on the `property` methods: https://openjfx.io/javadoc/18/javafx.graphics/javafx/concurrent/Worker.html#getState() This causes `Service` to inherit these as well. Docs should only appear on the property (except in special circumstances), but I didn't want to touch all of this here. Should I create a new issue for this or is it not worth it? - PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v4]
> Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments - Changes: - all: https://git.openjdk.org/jfx/pull/880/files - new: https://git.openjdk.org/jfx/pull/880/files/6e9eda88..e9df3a27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=880=03 - incr: https://webrevs.openjdk.org/?repo=jfx=880=02-03 Stats: 20 lines in 1 file changed: 0 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jfx/pull/880.diff Fetch: git fetch https://git.openjdk.org/jfx pull/880/head:pull/880 PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Tue, 23 Aug 2022 11:37:38 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/java/javafx/concurrent/ScheduledService.java
>> line 130:
>>
>>> 128: * will treat that duration as if it were Duration.ZERO. Likewise, any
>>> Duration which answers true
>>> 129: * to {@link javafx.util.Duration#isIndefinite()} will be treated as
>>> if it were a duration of Double.MAX_VALUE
>>> 130: * milliseconds. Any {@code null} Duration is treated as
>>> Duration.ZERO. Any custom implementation of a backoff strategy
>>
>> Since you changed `null` to use code style, maybe also do it for
>> `Duration.ZERO`?
>
> There are many places that are missing it in this class. Should I do them all
> while here?
I'll leave that up to you.
-
PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v3]
On Tue, 23 Aug 2022 11:46:47 GMT, Nir Lisker wrote:
>> Fixes the mistakes in the JBS ticket and some additional minor corrections.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Addressed review comments
modules/javafx.graphics/src/main/java/javafx/concurrent/Service.java line 104:
> 102: * For example, to write a Service that reads the first line
> 103: * from any URL and returned it as a String, it might be defined
> 104: * such that it had a single property, {@code url}, and might be
> implemented
I like the rewrite, although a couple of the verb tenses are now mismatched:
"returned" --> "returns"
"had" --> "has" (or replace "such that it had" by "with")
-
PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v3]
> Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Addressed review comments - Changes: - all: https://git.openjdk.org/jfx/pull/880/files - new: https://git.openjdk.org/jfx/pull/880/files/d8ccb5ae..6e9eda88 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=880=02 - incr: https://webrevs.openjdk.org/?repo=jfx=880=01-02 Stats: 8 lines in 4 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/880.diff Fetch: git fetch https://git.openjdk.org/jfx pull/880/head:pull/880 PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Mon, 22 Aug 2022 22:21:29 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix typo
>
> modules/javafx.graphics/src/main/java/javafx/concurrent/ScheduledService.java
> line 130:
>
>> 128: * will treat that duration as if it were Duration.ZERO. Likewise, any
>> Duration which answers true
>> 129: * to {@link javafx.util.Duration#isIndefinite()} will be treated as if
>> it were a duration of Double.MAX_VALUE
>> 130: * milliseconds. Any {@code null} Duration is treated as Duration.ZERO.
>> Any custom implementation of a backoff strategy
>
> Since you changed `null` to use code style, maybe also do it for
> `Duration.ZERO`?
There are many places that are missing it in this class. Should I do them all
while here?
> modules/javafx.graphics/src/main/java/javafx/concurrent/Service.java line 102:
>
>> 100: * Because a Service is intended to simplify declarative use cases,
>> subclasses
>> 101: * should expose as properties the input parameters to the work to
>> be done.
>> 102: * For example, suppose I wanted to write a Service that reads the
>> first line
>
> As long as you are changing this sentence, can you also change `I` to `you`?
> The unintended use of first person here is a bit jarring.
I rephrased the sentence.
-
PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Fri, 19 Aug 2022 00:37:34 GMT, Nir Lisker wrote:
>> Fixes the mistakes in the JBS ticket and some additional minor corrections.
>
> Nir Lisker has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix typo
A few comments inline.
modules/javafx.controls/src/main/java/javafx/scene/control/TextFormatter.java
line 47:
> 45: *
> 46: *
> 47: * It's possible to have a formatter with just filter or value converter.
> If value converter is not provided however,
The "however" in the middle of the sentence is a bit awkward, and not really
needed here. I suggest dropping it.
modules/javafx.controls/src/main/java/javafx/scene/control/TextFormatter.java
line 48:
> 46: *
> 47: * It's possible to have a formatter with just filter or value converter.
> If value converter is not provided however,
> 48: * setting a value will result in an {@code IllegalStateException} and
> the value is always {#code null}.
That should be `{@code null}`
modules/javafx.graphics/src/main/java/javafx/concurrent/ScheduledService.java
line 130:
> 128: * will treat that duration as if it were Duration.ZERO. Likewise, any
> Duration which answers true
> 129: * to {@link javafx.util.Duration#isIndefinite()} will be treated as if
> it were a duration of Double.MAX_VALUE
> 130: * milliseconds. Any {@code null} Duration is treated as Duration.ZERO.
> Any custom implementation of a backoff strategy
Since you changed `null` to use code style, maybe also do it for
`Duration.ZERO`?
modules/javafx.graphics/src/main/java/javafx/concurrent/Service.java line 102:
> 100: * Because a Service is intended to simplify declarative use cases,
> subclasses
> 101: * should expose as properties the input parameters to the work to
> be done.
> 102: * For example, suppose I wanted to write a Service that reads the
> first line
As long as you are changing this sentence, can you also change `I` to `you`?
The unintended use of first person here is a bit jarring.
modules/javafx.graphics/src/main/java/javafx/concurrent/package.html line 36:
> 34:
> 35:
> 36: Provides the set of classes for javafx.concurrent.
Can you make this same change to the page title?
-
PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Fri, 19 Aug 2022 00:37:34 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo I took a quick look and don't see anything that would warrant a CSR. I'll confirm when I review. - PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Fri, 19 Aug 2022 14:54:11 GMT, Andy Goryachev wrote: > Would these changes require a CSR? Or, since they are just corrections, the > API did not really change? Otherwise, looks good. Thank you for cleaning up > the docs! Usually they don't since there are no spec changes. - PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Fri, 19 Aug 2022 00:37:34 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo Would these changes require a CSR? Or, since they are just corrections, the API did not really change? Otherwise, looks good. Thank you for cleaning up the docs! - Marked as reviewed by angorya (Author). PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v3]
On Thu, 18 Aug 2022 23:50:03 GMT, Nir Lisker wrote: > ``` > fatal: ambiguous argument 'upstream/jfx19': unknown revision or path not in > the working tree. > ``` I should have said "presuming you have a remote configured with the name `upstream`". Anyway, what you did was good. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
> Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir Lisker has updated the pull request incrementally with one additional commit since the last revision: Fix typo - Changes: - all: https://git.openjdk.org/jfx/pull/880/files - new: https://git.openjdk.org/jfx/pull/880/files/6f2894de..d8ccb5ae Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=880=01 - incr: https://webrevs.openjdk.org/?repo=jfx=880=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/880.diff Fetch: git fetch https://git.openjdk.org/jfx pull/880/head:pull/880 PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 23:52:48 GMT, Nir Lisker wrote: > Fixes the mistakes in the JBS ticket and some additional minor corrections. modules/javafx.base/src/main/java/javafx/beans/property/Property.java line 102: > 100: * > 101: * > 102: * property1.bindBicirectional(property2); Almost, but not quite. - PR: https://git.openjdk.org/jfx/pull/880
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v3]
On Thu, 18 Aug 2022 23:43:33 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Initial commit #880 is the new PR. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v3]
On Thu, 18 Aug 2022 23:43:33 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has refreshed the contents of this pull request, and previous > commits have been removed. Incremental views are not available. `git reset --hard upstream/jfx19` That gives an error fatal: ambiguous argument 'upstream/jfx19': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git [...] -- [...]' I will close this PR and resubmit. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v3]
> Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir Lisker has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Initial commit - Changes: - all: https://git.openjdk.org/jfx/pull/877/files - new: https://git.openjdk.org/jfx/pull/877/files/263061e3..5d0fbdea Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=877=02 - incr: https://webrevs.openjdk.org/?repo=jfx=877=01-02 Stats: 7926 lines in 80 files changed: 6688 ins; 1116 del; 122 mod Patch: https://git.openjdk.org/jfx/pull/877.diff Fetch: git fetch https://git.openjdk.org/jfx pull/877/head:pull/877 PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Thu, 18 Aug 2022 23:19:49 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains 16 new > commits since the last revision: > > - Initial commit > - 8290473: update Eclipse .classpath in apps, buildSrc > >Reviewed-by: nlisker > - 8181084: JavaFX show big icons in system menu on macOS with Retina display > >Reviewed-by: jpereda, kcr > - 8291908: VirtualFlow creates unneeded empty cells > >Reviewed-by: kcr, aghaisas > - 8291087: Wrong position of focus of screen reader on Windows with screen > scale > 1 > >Reviewed-by: aghaisas, arapte > - 8218826: TableRowSkinBase: horizontal layout broken if row has padding > >Reviewed-by: aghaisas > - 8289605: Robot capture tests fail intermittently on Mac M1 > >Reviewed-by: kcr > - 8289397: Fix warnings: Possible accidental assignment in place of a > comparison. A condition expression should not be reduced to an assignment > >Reviewed-by: kcr > - 8290990: Clear .root style class from a root node that is removed from a > Scene/SubScene > >Reviewed-by: jhendrikx, aghaisas, mhanl > - 8290527: Bump macOS GitHub actions to macOS 11 > >Reviewed-by: arapte > - ... and 6 more: https://git.openjdk.org/jfx/compare/c4b2bcd4...263061e3 Oh, there is only a single unique commit in your branch (or rather there was before you rebased some of the commits from master), so the following should work: git reset --hard upstream/jfx19 git cherry-pick 263061e366e5c7b4d0928b33d7bc4da90b9fa519 Check that there is only the one commit, and that the diffs look good: git log upstream/jfx19.. git diff upstream/jfx19.. then force push. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
On Thu, 18 Aug 2022 23:19:49 GMT, Nir Lisker wrote: >> Fixes the mistakes in the JBS ticket and some additional minor corrections. > > Nir Lisker has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains 16 new > commits since the last revision: > > - Initial commit > - 8290473: update Eclipse .classpath in apps, buildSrc > >Reviewed-by: nlisker > - 8181084: JavaFX show big icons in system menu on macOS with Retina display > >Reviewed-by: jpereda, kcr > - 8291908: VirtualFlow creates unneeded empty cells > >Reviewed-by: kcr, aghaisas > - 8291087: Wrong position of focus of screen reader on Windows with screen > scale > 1 > >Reviewed-by: aghaisas, arapte > - 8218826: TableRowSkinBase: horizontal layout broken if row has padding > >Reviewed-by: aghaisas > - 8289605: Robot capture tests fail intermittently on Mac M1 > >Reviewed-by: kcr > - 8289397: Fix warnings: Possible accidental assignment in place of a > comparison. A condition expression should not be reduced to an assignment > >Reviewed-by: kcr > - 8290990: Clear .root style class from a root node that is removed from a > Scene/SubScene > >Reviewed-by: jhendrikx, aghaisas, mhanl > - 8290527: Bump macOS GitHub actions to macOS 11 > >Reviewed-by: arapte > - ... and 6 more: https://git.openjdk.org/jfx/compare/c4b2bcd4...263061e3 How did you do the rebase? It should be something like: git fetch upstream git rebase -i upstream/master Alternatively, you can `git reset --hard upstream/jfx19` to reset your branch, and then cherry pick all of the commits (or apply the aggregate diffs as one commit). - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 19:08:19 GMT, Nir Lisker wrote: > Fixes the mistakes in the JBS ticket and some additional minor corrections. Looks like the rebase missed a few commits. Not sure why. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs [v2]
> Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir Lisker has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 16 new commits since the last revision: - Initial commit - 8290473: update Eclipse .classpath in apps, buildSrc Reviewed-by: nlisker - 8181084: JavaFX show big icons in system menu on macOS with Retina display Reviewed-by: jpereda, kcr - 8291908: VirtualFlow creates unneeded empty cells Reviewed-by: kcr, aghaisas - 8291087: Wrong position of focus of screen reader on Windows with screen scale > 1 Reviewed-by: aghaisas, arapte - 8218826: TableRowSkinBase: horizontal layout broken if row has padding Reviewed-by: aghaisas - 8289605: Robot capture tests fail intermittently on Mac M1 Reviewed-by: kcr - 8289397: Fix warnings: Possible accidental assignment in place of a comparison. A condition expression should not be reduced to an assignment Reviewed-by: kcr - 8290990: Clear .root style class from a root node that is removed from a Scene/SubScene Reviewed-by: jhendrikx, aghaisas, mhanl - 8290527: Bump macOS GitHub actions to macOS 11 Reviewed-by: arapte - ... and 6 more: https://git.openjdk.org/jfx/compare/c4b2bcd4...263061e3 - Changes: - all: https://git.openjdk.org/jfx/pull/877/files - new: https://git.openjdk.org/jfx/pull/877/files/c4b2bcd4..263061e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=877=01 - incr: https://webrevs.openjdk.org/?repo=jfx=877=00-01 Stats: 448344 lines in 7349 files changed: 103818 ins; 301779 del; 42747 mod Patch: https://git.openjdk.org/jfx/pull/877.diff Fetch: git fetch https://git.openjdk.org/jfx pull/877/head:pull/877 PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 19:08:19 GMT, Nir Lisker wrote: > Fixes the mistakes in the JBS ticket and some additional minor corrections. I'll review it. @arapte or @aghaisas Can one of you be the second reviewer? - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 22:40:09 GMT, Nir Lisker wrote: > It's a PR against the javafx19 branch. I think I forked the master branch > though, which is why it is showing all the commits. Yeah, this is a fairly easy mistake to make. It's also very obvious when it happens. > I might need to rebase, but if the changes are approved it should integrate > without issues. You will definitely need to rebase and force push before this can be reviewed. No, it otherwise won't integrate without issues. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 19:08:19 GMT, Nir Lisker wrote: > Fixes the mistakes in the JBS ticket and some additional minor corrections. you probably need to rebase. impossible to review. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 19:08:19 GMT, Nir Lisker wrote: > Fixes the mistakes in the JBS ticket and some additional minor corrections. It's a PR against the javafx19 branch. I think I forked the master branch though, which is why it is showing all the commits. I might need to rebase, but if the changes are approved it should integrate without issues. - PR: https://git.openjdk.org/jfx/pull/877
Re: [jfx19] RFR: 8286678: Fix mistakes in FX API docs
On Thu, 18 Aug 2022 19:08:19 GMT, Nir Lisker wrote: > Fixes the mistakes in the JBS ticket and some additional minor corrections. Nir, have you sync'ed your branch with the latest master? There are 5000+ files changed listed. - PR: https://git.openjdk.org/jfx/pull/877
