On Mon, 4 Mar 2024 21:04:28 GMT, Andy Goryachev <ango...@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
