Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v25]
On Thu, 2 May 2024 10:03:46 GMT, Ambarish Rapte wrote: > 1. The test does not compile without fix, hence it won't fail without fix > as we are only testing the newly added helper method. Yes, we could not build on existing a11y test. > 2. It is not required to change the error stream, as the test does/need > not inspect the error output. Nice! > 3. The test method and parameter source method names can be changed a > little. Note that `@MethodSoruce` can be used without any parameter - then the same name is used. OK, this is not the style of JFX. For JUnit5, the `test` prefix is not necessary any more (and can be removed - see https://docs.openrewrite.org/recipes/java/testing/cleanup/removetestprefix), but I think, because of consistency, it is kept. > 4. We should use the Util class for standard setup like initializing > JavaFX/ shutting it down. Nice! > I tried above changes to test locally, attaching the file for ease. > [WinTextRangeProviderTest.zip](https://github.com/openjdk/jfx/files/15186626/WinTextRangeProviderTest.zip) Thank you. I committed at d03bdd40a3340bd85397f (with you as author, hope this is in line with the policies?!). - PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2091906463
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v26]
> Fixes https://bugs.openjdk.org/browse/JDK-8330462. > > If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, then > an addition of `start` to it leads to a negative value. This is "fixed" by > using `Math.max` comparing the `maxLength` and `maxLength + start`. Oliver Kopp has updated the pull request incrementally with one additional commit since the last revision: Streamline WinTextRangeProviderTest Source: https://github.com/openjdk/jfx/pull/1442#pullrequestreview-2035329983 - Changes: - all: https://git.openjdk.org/jfx/pull/1442/files - new: https://git.openjdk.org/jfx/pull/1442/files/45a56c6f..d03bdd40 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1442&range=25 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1442&range=24-25 Stats: 28 lines in 1 file changed: 1 ins; 19 del; 8 mod Patch: https://git.openjdk.org/jfx/pull/1442.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1442/head:pull/1442 PR: https://git.openjdk.org/jfx/pull/1442
Re: RFR: 8092102: Labeled: truncated property [v11]
On Thu, 2 May 2024 22:10:24 GMT, Andy Goryachev wrote: >> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property reacts to changes in the following properties: >> - ellipsisString >> - font >> - height >> - text >> - width >> - wrapText >> >> I don't think it's worth creating a headful test (headless won't work) due >> to relative simplicity of the code. >> >> **Alternative** >> >> The desired functionality can be just as easily achieved on an application >> level, by adding a similar property to a subclass. What is the benefit of >> adding this functionality to the core? >> >> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as >> well) lives by different rules (TableCellSkinBase:152, >> TreeTableCellSkin:126). The consequence of this is that the new >> functionality **cannot** be fully implemented with the public APIs alone. >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > using properties The latest code changes look good. I need to do some more testing tomorrow. I have what I think is a simple test program that isn't behaving like I expected. I'll double-check and let you know. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2091883561
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 18:21:52 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - missing )
>> - review comments
>> - Merge branch 'master' into 8092102.truncated
>> - add exports
>> - added unit tests
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - test
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - Merge branch 'master' into 8092102.truncated
>> - labeled helper
>> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
> line 45:
>
>> 43: * in their skins to different code paths.
>> 44: */
>> 45: public class LabeledTruncatedTest {
>
> It might be worth adding a test for `Button`.
test added
> modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
> line 93:
>
>> 91: firePulse();
>> 92:
>> 93: assertFalse(control.isTextTruncated());
>
> Can you add a test for the case where we are wrapping and the preferred
> height is exceeded?
test added
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588474995
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588474917
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 18:34:04 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line >> 114: >> >>> 112: * by itself looks rather weird. >>> 113: */ >>> 114: private static boolean useActualContentWidth; >> >> Even given your comment about why it's safe to use a global variable, >> wouldn't it be cleaner to make it an instance variable? I suppose it might >> be OK to keep it global, but only if you can show that there are no issues >> with reentrancy. > > re-entrancy should not be an issue: the flag is used in the context of a > single method which is always being called from an fx application thread. > > I do not want to increase the memory footprint by making it an instance > variable. on second thought, you are right. we cannot guarantee that someone won't stick a TableView as a graphic into another TableView cell. Switched to use an ephemeral entry in `Node.getProperties()`, same as `Properties.DEFER_TO_PARENT_PREF_WIDTH`. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588476651
Re: RFR: 8092102: Labeled: truncated property [v11]
> Adds **Labeled.textTruncated** property which indicates when the text is > visually truncated (and the ellipsis string is inserted) in order to fit the > available width. > > The new property reacts to changes in the following properties: > - ellipsisString > - font > - height > - text > - width > - wrapText > > I don't think it's worth creating a headful test (headless won't work) due to > relative simplicity of the code. > > **Alternative** > > The desired functionality can be just as easily achieved on an application > level, by adding a similar property to a subclass. What is the benefit of > adding this functionality to the core? > > UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as > well) lives by different rules (TableCellSkinBase:152, > TreeTableCellSkin:126). The consequence of this is that the new > functionality **cannot** be fully implemented with the public APIs alone. > > **See Also** > > * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow > for tooltip when cell text is truncated > * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show > Tooltip only when text is shown with ellipsis (...) Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: using properties - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/062d47ac..6ea4c698 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1389&range=10 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1389&range=09-10 Stats: 54 lines in 5 files changed: 29 ins; 14 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Re: RFR: 8092102: Labeled: truncated property [v10]
> Adds **Labeled.textTruncated** property which indicates when the text is > visually truncated (and the ellipsis string is inserted) in order to fit the > available width. > > The new property reacts to changes in the following properties: > - ellipsisString > - font > - height > - text > - width > - wrapText > > I don't think it's worth creating a headful test (headless won't work) due to > relative simplicity of the code. > > **Alternative** > > The desired functionality can be just as easily achieved on an application > level, by adding a similar property to a subclass. What is the benefit of > adding this functionality to the core? > > UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as > well) lives by different rules (TableCellSkinBase:152, > TreeTableCellSkin:126). The consequence of this is that the new > functionality **cannot** be fully implemented with the public APIs alone. > > **See Also** > > * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow > for tooltip when cell text is truncated > * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show > Tooltip only when text is shown with ellipsis (...) Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - csr - Merge branch 'master' into 8092102.truncated - missing ) - review comments - Merge branch 'master' into 8092102.truncated - add exports - added unit tests - Merge remote-tracking branch 'origin/master' into 8092102.truncated - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - ... and 7 more: https://git.openjdk.org/jfx/compare/aa9907a5...062d47ac - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1389&range=09 Stats: 309 lines in 8 files changed: 279 ins; 19 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 17:37:52 GMT, Kevin Rushforth wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 15 commits: >> >> - missing ) >> - review comments >> - Merge branch 'master' into 8092102.truncated >> - add exports >> - added unit tests >> - Merge remote-tracking branch 'origin/master' into 8092102.truncated >> - test >> - Merge remote-tracking branch 'origin/master' into 8092102.truncated >> - Merge branch 'master' into 8092102.truncated >> - labeled helper >> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e > > modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line > 114: > >> 112: * by itself looks rather weird. >> 113: */ >> 114: private static boolean useActualContentWidth; > > Even given your comment about why it's safe to use a global variable, > wouldn't it be cleaner to make it an instance variable? I suppose it might be > OK to keep it global, but only if you can show that there are no issues with > reentrancy. re-entrancy should not be an issue: the flag is used in the context of a single method which is always being called from an fx application thread. I do not want to increase the memory footprint by making it an instance variable. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588132322
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 18:07:12 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - missing )
>> - review comments
>> - Merge branch 'master' into 8092102.truncated
>> - add exports
>> - added unit tests
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - test
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - Merge branch 'master' into 8092102.truncated
>> - labeled helper
>> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java
> line 33:
>
>> 31: * Labeled Helper.
>> 32: */
>> 33: public class LabeledHelper {
>
> You might consider making `LabeledHelper` a subclass of `ControlHelper`,
> which is the usual pattern for helper classes of nodes, although it may not
> matter here (and could be done later if there was a need).
Thinking on it further, let's not do this unless / until there is a need (there
likely won't be).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588118640
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 GMT, Andy Goryachev wrote:
>> Adds **Labeled.textTruncated** property which indicates when the text is
>> visually truncated (and the ellipsis string is inserted) in order to fit the
>> available width.
>>
>> The new property reacts to changes in the following properties:
>> - ellipsisString
>> - font
>> - height
>> - text
>> - width
>> - wrapText
>>
>> I don't think it's worth creating a headful test (headless won't work) due
>> to relative simplicity of the code.
>>
>> **Alternative**
>>
>> The desired functionality can be just as easily achieved on an application
>> level, by adding a similar property to a subclass. What is the benefit of
>> adding this functionality to the core?
>>
>> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as
>> well) lives by different rules (TableCellSkinBase:152,
>> TreeTableCellSkin:126). The consequence of this is that the new
>> functionality **cannot** be fully implemented with the public APIs alone.
>>
>> **See Also**
>>
>> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow
>> for tooltip when cell text is truncated
>> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show
>> Tooltip only when text is shown with ellipsis (...)
>
> Andy Goryachev has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 15 commits:
>
> - missing )
> - review comments
> - Merge branch 'master' into 8092102.truncated
> - add exports
> - added unit tests
> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
> - test
> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
> - Merge branch 'master' into 8092102.truncated
> - labeled helper
> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e
The API changes look good. I left a couple comments on the API docs.
I also left a couple questions / comments on the fix and test. I haven't tested
it yet.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java
line 33:
> 31: * Labeled Helper.
> 32: */
> 33: public class LabeledHelper {
You might consider making `LabeledHelper` a subclass of `ControlHelper`, which
is the usual pattern for helper classes of nodes, although it may not matter
here (and could be done later if there was a need).
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java
line 62:
> 60: * @return true to use the actual content width, false to delegate to
> the parent
> 61: */
> 62: public static boolean isUseActualContentWidth() {
If you make the `useActualContentWidth` flag an instance variable of `Labeled`
(see my comment in that class), then you would need to add the `Labeled` as an
argument to this method (here and in the accessor method).
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
114:
> 112: * by itself looks rather weird.
> 113: */
> 114: private static boolean useActualContentWidth;
Even given your comment about why it's safe to use a global variable, wouldn't
it be cleaner to make it an instance variable? I suppose it might be OK to keep
it global, but only if you can show that there are no issues with reentrancy.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
848:
> 846: /**
> 847: * Indicates whether the text has been truncated
> 848: * when it cannot fit into the available width.
"when" --> "because"
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
850:
> 848: * when it cannot fit into the available width.
> 849: *
> 850: * When truncated, the {@link #ellipsisStringProperty() ellipsis
> string}
I recommend either using `@linkplain` or changing the text to `ellipsisString`,
matching the name of the property.
modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
line 45:
> 43: * in their skins to different code paths.
> 44: */
> 45: public class LabeledTruncatedTest {
It might be worth adding a test for `Button`.
modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
line 93:
> 91: firePulse();
> 92:
> 93: assertFalse(control.isTextTruncated());
Can you add a test for the case where we are wrapping and the preferred height
is exceeded?
-
PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2036421013
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588087048
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588090900
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588033608
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588065486
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588067300
PR Review Comment: ht
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases [v2]
On Thu, 2 May 2024 09:27:41 GMT, drmarmac wrote: > I can create the PR. thank you! - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2090784273
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 GMT, Andy Goryachev wrote: >> Adds **Labeled.textTruncated** property which indicates when the text is >> visually truncated (and the ellipsis string is inserted) in order to fit the >> available width. >> >> The new property reacts to changes in the following properties: >> - ellipsisString >> - font >> - height >> - text >> - width >> - wrapText >> >> I don't think it's worth creating a headful test (headless won't work) due >> to relative simplicity of the code. >> >> **Alternative** >> >> The desired functionality can be just as easily achieved on an application >> level, by adding a similar property to a subclass. What is the benefit of >> adding this functionality to the core? >> >> UPDATE 2024/03/07: turns out Labeled in a TableView (in a TreeTableView as >> well) lives by different rules (TableCellSkinBase:152, >> TreeTableCellSkin:126). The consequence of this is that the new >> functionality **cannot** be fully implemented with the public APIs alone. >> >> **See Also** >> >> * [JDK-8327483](https://bugs.openjdk.org/browse/JDK-8327483) TreeView: Allow >> for tooltip when cell text is truncated >> * [JDK-8205211](https://bugs.openjdk.org/browse/JDK-8205211) Ability to show >> Tooltip only when text is shown with ellipsis (...) > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e Looks good. I tested a few scenarios with TableView, Button, Label. It behaved as expected - Marked as reviewed by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2035862805
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v3]
On Thu, 2 May 2024 13:12:09 GMT, drmarmac wrote: >> This PR adds the missing checks, as well as code documentation that an >> IndexOutOfBoundsException may be thrown. > > drmarmac 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 five additional commits since > the last revision: > > - Merge remote-tracking branch 'refs/remotes/origin/master' into > fixes/transformation-list-oob > - use `@code` syntax > - JavaDoc updates > - TransformationList.getSourceIndex/getViewIndex should throw IOOBE > - Add failing test Looks good. - Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1432#pullrequestreview-2035791854
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v3]
On Tue, 23 Apr 2024 06:55:18 GMT, Lukasz Kostyra wrote: >> drmarmac 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 five additional commits >> since the last revision: >> >> - Merge remote-tracking branch 'refs/remotes/origin/master' into >> fixes/transformation-list-oob >> - use `@code` syntax >> - JavaDoc updates >> - TransformationList.getSourceIndex/getViewIndex should throw IOOBE >> - Add failing test > > LGTM @lukostyra Can you re-review? - PR Comment: https://git.openjdk.org/jfx/pull/1432#issuecomment-2090541396
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v2]
On Thu, 2 May 2024 12:18:30 GMT, Kevin Rushforth wrote:
>> drmarmac has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> JavaDoc updates
>
> modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
> line 121:
>
>> 119: * @param index the index in this list
>> 120: * @return the index of the element's origin in the source list
>> 121: * @throws IndexOutOfBoundsException if the index is out of range
>> (index < 0 || index >= size())
>
> In javadoc comments, we prefer using `{@code ... }` rather than the raw HTML
> tags. This also allows using `<` rather than `<`.
>
> Suggested change:
>
>
> ... ({@code index < 0 || index >= size()})
ok, looks better!
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1432#discussion_r1587609916
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v3]
> This PR adds the missing checks, as well as code documentation that an > IndexOutOfBoundsException may be thrown. drmarmac 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 five additional commits since the last revision: - Merge remote-tracking branch 'refs/remotes/origin/master' into fixes/transformation-list-oob - use `@code` syntax - JavaDoc updates - TransformationList.getSourceIndex/getViewIndex should throw IOOBE - Add failing test - Changes: - all: https://git.openjdk.org/jfx/pull/1432/files - new: https://git.openjdk.org/jfx/pull/1432/files/b86cdfe8..e72f21cf Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1432&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1432&range=01-02 Stats: 18143 lines in 606 files changed: 11428 ins; 4729 del; 1986 mod Patch: https://git.openjdk.org/jfx/pull/1432.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1432/head:pull/1432 PR: https://git.openjdk.org/jfx/pull/1432
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v2]
On Thu, 2 May 2024 10:06:23 GMT, drmarmac wrote:
>> This PR adds the missing checks, as well as code documentation that an
>> IndexOutOfBoundsException may be thrown.
>
> drmarmac has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JavaDoc updates
Thanks for updating the docs. I did leave one more suggested change to use
`{@code ...}` for code case.
Also, can you please merge in the latest upstream master?
modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java
line 121:
> 119: * @param index the index in this list
> 120: * @return the index of the element's origin in the source list
> 121: * @throws IndexOutOfBoundsException if the index is out of range
> (index < 0 || index >= size())
In javadoc comments, we prefer using `{@code ... }` rather than the raw HTML
tags. This also allows using `<` rather than `<`.
Suggested change:
... ({@code index < 0 || index >= size()})
-
PR Review: https://git.openjdk.org/jfx/pull/1432#pullrequestreview-2035586643
PR Review Comment: https://git.openjdk.org/jfx/pull/1432#discussion_r1587540290
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values
On Sun, 24 Mar 2024 15:11:29 GMT, drmarmac wrote: > This PR adds the missing checks, as well as code documentation that an > IndexOutOfBoundsException may be thrown. I've updated the PR with Kevin's suggestions. - PR Comment: https://git.openjdk.org/jfx/pull/1432#issuecomment-2090076000
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v2]
On Mon, 29 Apr 2024 22:48:28 GMT, Kevin Rushforth wrote: >> drmarmac has updated the pull request incrementally with one additional >> commit since the last revision: >> >> JavaDoc updates > > modules/javafx.base/src/main/java/javafx/collections/transformation/TransformationList.java > line 134: > >> 132: * @param index the index of an element in this list >> 133: * @return the index of the element's origin in the provided list >> 134: * @throws IndexOutOfBoundsException if the index is out of range >> (index < 0 || index >= list.getSource().size()) > > There is no`getSource` method in `ObservableList`. That should be `... index > >= size())` fixed - PR Review Comment: https://git.openjdk.org/jfx/pull/1432#discussion_r1587375665
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v2]
> This PR adds the missing checks, as well as code documentation that an > IndexOutOfBoundsException may be thrown. drmarmac has updated the pull request incrementally with one additional commit since the last revision: JavaDoc updates - Changes: - all: https://git.openjdk.org/jfx/pull/1432/files - new: https://git.openjdk.org/jfx/pull/1432/files/df510c22..b86cdfe8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1432&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1432&range=00-01 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1432.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1432/head:pull/1432 PR: https://git.openjdk.org/jfx/pull/1432
Re: RFR: 8330462: StringIndexOutOfBoundException when typing anything into TextField [v25]
On Wed, 1 May 2024 04:46:29 GMT, Oliver Kopp wrote: >> Fixes https://bugs.openjdk.org/browse/JDK-8330462. >> >> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, >> then an addition of `start` to it leads to a negative value. This is "fixed" >> by using `Math.max` comparing the `maxLength` and `maxLength + start`. > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > Adress review comments The fix looks good, I have few comments about the test. 1. The test does not compile without fix, hence it won't fail without fix as we are only testing the newly added helper method. 2. It is not required to change the error stream, as the test does/need not inspect the error output. 3. The test method and parameter source method names can be changed a little. 4. We should use the Util class for standard setup like initializing JavaFX/ shutting it down. I tried above changes to test locally, attaching the file for ease. [WinTextRangeProviderTest.zip](https://github.com/openjdk/jfx/files/15186626/WinTextRangeProviderTest.zip) - Changes requested by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1442#pullrequestreview-2035329983
Re: RFR: 8242553: IntegerSpinner and DoubleSpinner do not wrap around values correctly in some cases [v2]
On Wed, 10 Apr 2024 11:47:28 GMT, drmarmac wrote: >> This PR should fix the issue and cover all relevant cases with new tests. >> >> Note: This involves a small behavior change, as can be seen in >> dblSpinner_testWrapAround_decrement_twoSteps() in SpinnerTest.java:749. With >> this change the wraparound behavior is similar to that of the IntegerSpinner. > > drmarmac has updated the pull request incrementally with one additional > commit since the last revision: > > Use direction-dependent modulo arithmetic in DoubleSpinnerValueFactory > wrap-around logic thanks for creating the issue, I can create the PR. - PR Comment: https://git.openjdk.org/jfx/pull/1431#issuecomment-2090006139
Re: RFR: 8311895: CSS Transitions [v16]
> Implementation of [CSS
> Transitions](https://gist.github.com/mstr2/c72f8c9faa87de14926978f517a6018a).
>
> ### Example
>
> .button {
> -fx-background-color: dodgerblue;
> }
>
> .button:hover {
> -fx-background-color: red;
> -fx-scale-x: 1.1;
> -fx-scale-y: 1.1;
>
> transition: -fx-background-color 0.5s ease,
> -fx-scale-x 0.5s ease,
> -fx-scale-y 0.5s ease;
> }
>
> src="https://user-images.githubusercontent.com/43553916/184781143-0520fbfe-54bf-4b8d-93ac-834708e46500.gif";
> width="200"/>
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
Michael Strauß has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 53 commits:
- Merge branch 'master' into feature/css-transitions
- update 'since' tags
- Fix javadoc error
- Change javadoc comment
- Merge branch 'master' into feature/css-transitions
- Discard redundant transitions in StyleableProperty impls
- Changes per review
- Merge branch 'master' into feature/css-transitions
- Merge branch 'master' into feature/css-transitions
- Add TransitionMediator
- ... and 43 more: https://git.openjdk.org/jfx/compare/aa9907a5...6614abb9
-
Changes: https://git.openjdk.org/jfx/pull/870/files
Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=870&range=15
Stats: 4673 lines in 43 files changed: 4630 ins; 4 del; 39 mod
Patch: https://git.openjdk.org/jfx/pull/870.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/870/head:pull/870
PR: https://git.openjdk.org/jfx/pull/870
