Re: RFR: 8092102: Labeled: truncated property [v14]
On Mon, 6 May 2024 14:50:00 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java >> line 221: >> >>> 219: OverrunStyle type, >>> 220: String ellipsisString, >>> 221: AtomicBoolean textTruncated >> >> I recommend setting `textTruncated` to false as the first statement >> (alternatively, add a comment that the caller is expected to initialize it >> to false). > > setting to false would be incorrect in the case of multi-line text. > > I can add a comment, though it's not a public API and there was no > comment/javadoc before. Yes, I see. In that case, it's up to you whether to add a simple comment that this should be initialized by the caller (but maybe it doesn't need to be mentioned?) - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1591138400
Re: RFR: 8092102: Labeled: truncated property [v14]
On Sat, 4 May 2024 14:05:19 GMT, Kevin Rushforth wrote: >> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> whitespace > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java > line 221: > >> 219: OverrunStyle type, >> 220: String ellipsisString, >> 221: AtomicBoolean textTruncated > > I recommend setting `textTruncated` to false as the first statement > (alternatively, add a comment that the caller is expected to initialize it to > false). setting to false would be incorrect in the case of multi-line text. I can add a comment, though it's not a public API and there was no comment/javadoc before. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1591128736
Re: RFR: 8092102: Labeled: truncated property [v14]
On Fri, 3 May 2024 21:00:26 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 is being set by the code which computes the actual text
>> string to be displayed (and which inserts the ellipsis string) in
>> `LabeledSkinBase.updateDisplayedText(double,double)`.
>>
>>
>> **Alternative**
>>
>> None exists as this requires changes to the core (Utils).
>>
>>
>> **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:
>
> whitespace
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
837:
> 835: * When truncated, the {@link #ellipsisStringProperty()
> ellipsisString}
> 836: * gets inserted in the place dictated by the
> 837: * {@link #textOverrun} property.
I just noticed that this link doesn't work. Looks like you'll need to change it
to point to the `textOverrunProperty()` method.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1591041626
Re: RFR: 8092102: Labeled: truncated property [v14]
On Sat, 4 May 2024 14:17:16 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> whitespace
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
> 855:
>
>> 853: }
>> 854:
>> 855: private ReadOnlyBooleanWrapper textTruncated() {
>
> ~Suggestion: rename this method to something like `textTruncatedImpl()` for
> clarity (as it is, the method, which is a writable property, shares the same
> name as the read-only property field, which could be confusing).~
Never mind. I missed that the types of the field and method are identical, so I
withdraw my suggestion.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1590957454
Re: RFR: 8092102: Labeled: truncated property [v14]
On Fri, 3 May 2024 21:00:26 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 is being set by the code which computes the actual text
>> string to be displayed (and which inserts the ellipsis string) in
>> `LabeledSkinBase.updateDisplayedText(double,double)`.
>>
>>
>> **Alternative**
>>
>> None exists as this requires changes to the core (Utils).
>>
>>
>> **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:
>
> whitespace
Yes, this looks like a better approach for the implementation. I did a quick
test and it now works as expected.
I left a couple comments inline.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java
line 221:
> 219: OverrunStyle type,
> 220: String ellipsisString,
> 221: AtomicBoolean textTruncated
I recommend setting `textTruncated` to false as the first statement
(alternatively, add a comment that the caller is expected to initialize it to
false).
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/skin/Utils.java
line 436:
> 434: OverrunStyle truncationStyle,
> 435: String ellipsisString,
> 436: AtomicBoolean textTruncated,
Similarly, I recommend setting it to false as the first statement. In this
case, I see that it will be set to false in the case where it makes it to the
return statement at the end of the method without being truncated, but there is
one early return where this isn't the case.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
855:
> 853: }
> 854:
> 855: private ReadOnlyBooleanWrapper textTruncated() {
Suggestion: rename this method to something like `textTruncatedImpl()` for
clarity (as it is, the method, which is a writable property, shares the same
name as the read-only property field, which could be confusing).
-
PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2039358463
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589993165
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589993485
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589994760
Re: RFR: 8092102: Labeled: truncated property [v14]
On Fri, 3 May 2024 21:00:26 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 is being set by the code which computes the actual text >> string to be displayed (and which inserts the ellipsis string) in >> `LabeledSkinBase.updateDisplayedText(double,double)`. >> >> >> **Alternative** >> >> None exists as this requires changes to the core (Utils). >> >> >> **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: > > whitespace Changed the way the new property is being set. Instead of listening for a bunch of existing properties (that's bad), hooked up directly into `LabeledSkinBase.updateDisplayedText()` which does the actual manipulation with the text. In addition, modified the Monkey Tester https://github.com/andy-goryachev-oracle/MonkeyTest to add ability to see the value of various properties in real time. For example, to verify that the new property is being updated in the context of a table, select TableView page, set Control -> Context Menu -> Show Properties Monitor, select a cell and right click to open the Property Monitor window. Find a `TextFieldTableCell` in the list and observe the value changing when resizing the column:  - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2093841963
Re: RFR: 8092102: Labeled: truncated property [v14]
> 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 is being set by the code which computes the actual text > string to be displayed (and which inserts the ellipsis string) in > `LabeledSkinBase.updateDisplayedText(double,double)`. > > > **Alternative** > > None exists as this requires changes to the core (Utils). > > > **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: whitespace - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/7a0dedd4..7d267043 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=13 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=12-13 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
