RFR: 8092102: Labeled: truncated property [v4]
Marius Hanl
mhanl at openjdk.org
Fri Mar 15 09:45:45 UTC 2024
On Fri, 15 Mar 2024 09:41:25 GMT, Marius Hanl <mhanl at openjdk.org> wrote:
>> 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
You can check here how I manipulated the size of the stage of the `StageLoader`.
https://github.com/openjdk/jfx/pull/1396
Than we can check and set some properties on any `Labeled`, probably the `Label` is good here. And on the stage if needed (width, height).
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1526010108
More information about the openjfx-dev
mailing list