RFR: 8092102: Labeled: truncated property

[Nir Lisker]

On Mon, 4 Mar 2024 21:04:28 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> Adds Labeled.truncated 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
> - text
> - width
> - wrapText
> 
> For some reason, line 859 generates a javadoc "co comment" warning, despite the javadoc comment present at the property declaration in line 832.
> 
> 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?

This enhancement should be discussed in the mailing list. I've never needed this myself, but it already had an old JBS ticket, so I can see that others did and for a legitimate reason (adding tooltips). I don't mind if it's added or not.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 823:

> 821:     /**
> 822:      * Truncated read-only property indicates whether the text has been truncated
> 823:      * when it cannot fit into the available width.

No need to repeat the name and type in the first sentence as it's used in the summary table. Most phrasings opt (inconsistently) for: "Indicates whether..." or just "Whether..." (I prefer the former).

I'm also not sure about the sole role of the width. Can the text not be truncated for a reason other than not fitting in the width, like not fitting in the height?

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 830:

> 828:      *
> 829:      * @since 23
> 830:      * @defaultValue false

I don't think that a default value is applicable here since it's completely dependent on other properties and can't be set. It's possible to call the `get` of this property for the first time and get `true`. The value here is the value used in the get method:

return property == null ? DEFAULT_VALUE : property.get();

at least in all the cases I've seen.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 832:

> 830:      * @defaultValue false
> 831:      */
> 832:     private ObservableBooleanValue truncated;

The property name should probably be `textTruncated` to be in line with `textOverrun`, `textFill`, `wrapText`...

Otherwise it could look like the labeled is truncated.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 834:

> 832:     private ObservableBooleanValue truncated;
> 833: 
> 834:     public final ObservableBooleanValue truncatedProperty() {

`ObservableBooleanValue` is not a property, so it's odd to use it as the return type of one. Usually `ReadOnlyBooleanProperty` is used. This implementation through binding is also unique for read-only properties. A look in `Node` and `Labeled` show a different way of implementing read-only properties.

modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line 850:

> 848:                 protected boolean computeValue() {
> 849:                     if (isWrapText()) {
> 850:                         return false;

Are you sure that allowing text to wrap necessarily means it won't be truncated? What if the max height doesn't allow another line?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1917119989
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512963900
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512912954
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512968169
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512956759
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512898960