Re: RFR: 8092102: Labeled: truncated property [v4]
On Fri, 15 Mar 2024 09:43:26 GMT, Marius Hanl wrote: >> 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). thanks, tests are coming. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1538284627
Re: RFR: 8092102: Labeled: truncated property [v4]
On Tue, 12 Mar 2024 15:50:21 GMT, Andy Goryachev 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
Re: RFR: 8092102: Labeled: truncated property [v4]
On Fri, 15 Mar 2024 09:41:25 GMT, Marius Hanl 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
Re: RFR: 8092102: Labeled: truncated property [v4]
On Mon, 11 Mar 2024 22:03:49 GMT, Marius Hanl wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> labeled helper
>
> 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?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1521720911
Re: RFR: 8092102: Labeled: truncated property [v4]
On Fri, 8 Mar 2024 21:08:25 GMT, Andy Goryachev 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-8091891](https://bugs.openjdk.org/browse/JDK-8091891) TreeView: There
>> is no tooltip available on truncated node
>> * [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:
>
> labeled helper
Changes requested by mhanl (Committer).
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.
-
PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1929489141
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1520481720
Re: RFR: 8092102: Labeled: truncated property [v4]
> 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-8091891](https://bugs.openjdk.org/browse/JDK-8091891) TreeView: There > is no tooltip available on truncated node > * [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: labeled helper - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/72f170b6..cf9a2e0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=02-03 Stats: 138 lines in 5 files changed: 86 ins; 45 del; 7 mod Patch: https://git.openjdk.org/jfx/pull/1389.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1389/head:pull/1389 PR: https://git.openjdk.org/jfx/pull/1389
