Re: RFR: 8092102: Labeled: truncated property [v8]
On Wed, 10 Apr 2024 21:28:16 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java >> line 38: >> >>> 36: /** >>> 37: * Returns true when the Labeled must compute the actual >>> content width in computePrefWidth(). >>> 38: * @return whether computePrefWidth() must compute the actual >>> content width >> >> Do you think re-wording the sentence to explain when this function returns >> true/false would be better here? > > is the new version clearer? Yes. Thanks for updating. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1565508320
Re: RFR: 8092102: Labeled: truncated property [v8]
On Wed, 10 Apr 2024 09:03:33 GMT, Karthik P K wrote: >> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> add exports > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java > line 38: > >> 36: /** >> 37: * Returns true when the Labeled must compute the actual content >> width in computePrefWidth(). >> 38: * @return whether computePrefWidth() must compute the actual >> content width > > Do you think re-wording the sentence to explain when this function returns > true/false would be better here? is the new version clearer? - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1560075257
Re: RFR: 8092102: Labeled: truncated property [v8]
On Thu, 28 Mar 2024 21:44:49 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: > > add exports Overall this looks good to me. Left couple of inline comments. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java line 38: > 36: /** > 37: * Returns true when the Labeled must compute the actual content > width in computePrefWidth(). > 38: * @return whether computePrefWidth() must compute the actual > content width Do you think re-wording the sentence to explain when this function returns true/false would be better here? modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java line 58: > 56: /** > 57: * Returns true when the Labeled must compute the actual content > width in computePrefWidth(). > 58: * @return whether computePrefWidth() must compute the actual content > width Same comment as above modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java line 42: > 40: /** > 41: * Tests textTruncated property of Labeled, using Label, TableCell, and > TreeTableCell controls > 42: * (the last two contain conditional code that redirects the execution of > computePrefWidth() Missing `)`? - PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1991164711 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1559103685 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1559108375 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1559148976
Re: RFR: 8092102: Labeled: truncated property [v8]
On Thu, 28 Mar 2024 21:44:49 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: > > add exports unit tests added, please re-review. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2026206381
Re: RFR: 8092102: Labeled: truncated property [v8]
> 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: add exports - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/45704b92..f98ff907 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=07 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=06-07 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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
