On Thu, 2 May 2024 22:10:24 GMT, Andy Goryachev <ango...@openjdk.org> 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

I found at least one case where this fails to detect truncation. There might be 
others. I will attach a test case that shows the failure.

Additionally, I left a comment about the modifications to Region. Since they 
are only needed for testing, you can instead add getWidth / getHeight to 
RegionShim, allowing you to revert all changes to Region (and RegionHelper).

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

> 855:                         }
> 856: 
> 857:                         return (getWidth() < prefWidth(getHeight()));

This is not sufficient to detect all cases where truncation can occur. The 
assumption that pref width is the full width needed to contain the text does 
not always hold true. For example, if an app explicitly sets the pref width of 
a button or label, its width will be set to its pref width. If the text does 
not fit, it will be truncated, but you won't detect it.

I'm not sure you can detect truncation just by looking at the various 
properties on the Labeled control. Would it be possible to set the value of the 
truncated property at the point where truncation occurs? That would guarantee a 
correct answer.

modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
 line 87:

> 85: 
> 86:     private void test(Labeled control) {
> 87:         RegionHelper.setWidth(control, 1000);

This can (and should) be done using a shim method rather than adding a helper 
method. If you add setWidth / setHeight to RegionShim you don't need to modify 
Region.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/layout/RegionHelper.java
 line 115:

> 113:     }
> 114: 
> 115:     public static void setWidth(Node node, double width) {

If you instead add this method to RegionShim, you won't need to modify Region 
at all, and its impact will be limited to the test classes.

modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java line 186:

> 184: 
> 185:             @Override
> 186:             public void setWidth(Node node, double width) {

If you add the needed methods to RegionShim, you can revert all changes to this 
file.

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

Changes requested by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2038130006
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589192349
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589195274
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589196444
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589197018

Reply via email to