On Tue, 12 Mar 2024 15:50:21 GMT, Andy Goryachev <[email protected]> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java
>> line 133:
>>
>>> 131: protected double computePrefWidth(double height, double topInset,
>>> double rightInset, double bottomInset,
>>> 132: double leftInset) {
>>> 133: if (LabeledHelper.isUseContentWidth() ||
>>> isDeferToParentForPrefWidth) {
>>
>> I'm not conviced that this is the correct solution here.
>> First of all I would keep this PR simple and ONLY add the truncated Property.
>> Second, I would rather want to see if this can be changed without adding
>> some flags which once again will not help application developers that may
>> want to extend something like this.
>>
>> IMO, the separation of concern is wrong. A Cell should always give back the
>> prefWidth, other callers should think if they want to use the pref width or
>> the table column width.
>>
>> In order to move this forward, I would like to see only the necessary
>> changes, which is the truncated property. Some tests for it would be good as
>> well.
>
> Thank you for the feedback!
>
> The property is being added to Labeled, which happens to be a base class for
> the cells' skins. The truncated property therefore must work in all the
> subclasses, even those where computePrefWidth is hacked via
> `Properties.DEFER_TO_PARENT_PREF_WIDTH`.
>
>> Second, I would rather want to see if this can be changed without adding
>> some flags which once again will not help application developers that may
>> want to extend something like this.
>
> This is a much more difficult task: First, it needs a careful discussion on
> which APIs to add, if any (there is a possibility that some people might say
> that no new APIs are needed as one can always create an instance of Text or
> Label and use it to get the sizing). There might be a need to undo the hacks
> made by the founding father such as `Properties.DEFER_TO_PARENT_PREF_WIDTH`.
> I would say all this is out of scope for this PR, as it achieves its goal
> without introducing the new APIs.
>
>> IMO, the separation of concern is wrong. A Cell should always give back the
>> prefWidth, other callers should think if they want to use the pref width or
>> the table column width.
>
> Why cells are controls is slightly beyond me, but who am I to ask? I would
> say there should be one control in this picture, and that is TableView (or
> any other cell-based control) whose job is to size and lay out its various
> parts. The current design is different, and it is out of scope for this PR
> to change the design.
>
>> In order to move this forward, I would like to see only the necessary changes
>
> And that 's exactly what you see here - only the necessary changes (that
> unfortunately include undoing the earlier hacks in cells).
>
> As for tests - I certainly agree, but that would be a manual test. The
> behavior can be verified with the examples in JDK-8205211, I just felt the
> value of a manual test is rather low. What I would rather see is an
> automated test that actually verifies the new property behavior down to a
> pixel. Any ideas how to make one?
Okay.
Tests: Well, there are some properties we can test here:
- wrapText = true -> height should be used -> check truncated
- width changed -> check truncated
- text changed -> check truncated
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1526007604
