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 [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:
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: 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 Testing looks good. Works as expected. The change looks good. Please allow me a little more time to take a more closer look. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2085127845
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 @azuev-java please review - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2083402516
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 @aghaisas could you take a look at this PR please? - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2070351050
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 LGTM - Marked as reviewed by kpk (Committer). PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2000588287
Re: RFR: 8092102: Labeled: truncated property [v9]
> 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 - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=08 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
