Re: RFR: 8092102: Labeled: truncated property [v15]
> 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: comments - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/7d267043..cce514ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=14 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=13-14 Stats: 34 lines in 2 files changed: 33 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
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
Re: RFR: 8092102: Labeled: truncated property [v13]
> 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: using LabeledSkinBase.updateDisplayedText - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/219926cb..7a0dedd4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=12 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=11-12 Stats: 183 lines in 7 files changed: 75 ins; 62 del; 46 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
Re: RFR: 8092102: Labeled: truncated property [v11]
On Fri, 3 May 2024 13:19:17 GMT, Kevin Rushforth wrote: > Would it be possible to set the value of the truncated property at the point > where truncation occurs? yes, `LabeledSkinBase.updateDisplayedText()` determines whether the text is truncated or not. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1589390518
Re: RFR: 8092102: Labeled: truncated property [v12]
> 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: shim - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/6ea4c698..219926cb Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=11 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=10-11 Stats: 36 lines in 4 files changed: 9 ins; 21 del; 6 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
Re: RFR: 8092102: Labeled: truncated property [v11]
On Thu, 2 May 2024 22:10:24 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-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
Here is a test showing the failure to detect truncated text.
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.layout.HBox;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.stage.Stage;
public class TruncatedTest extends Application {
private static final String clickMe = "Click me";
private static final String clickMeAgain = "Click me again, please";
@Override public void start(Stage stage) {
stage.setTitle("Truncated Button Text (fails)");
var root = new HBox(10);
Scene scene = new Scene(root, 600, 450);
var button = new Button(clickMe);
button.setPrefWidth(100);
button.setOnAction(e -> {
button.setText(clickMe.equals(button.getText()) ? clickMeAgain :
clickMe);
//System.err.println("truncated: " + button.isTextTruncated());
});
root.getChildren().add(button);
var label = new Label();
label.textProperty().bind(button.textTruncatedProperty().asString());
root.getChildren().add(label);
stage.setScene(scene);
stage.show();
}
public static void main(String[] args) {
Application.launch(args);
}
}
-
PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2093024810
Re: RFR: 8092102: Labeled: truncated property [v11]
On Thu, 2 May 2024 22:10:24 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-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
Re: RFR: 8092102: Labeled: truncated property [v11]
On Thu, 2 May 2024 22:10:24 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-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 The latest code changes look good. I need to do some more testing tomorrow. I have what I think is a simple test program that isn't behaving like I expected. I'll double-check and let you know. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2091883561
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 18:21:52 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - missing )
>> - review comments
>> - Merge branch 'master' into 8092102.truncated
>> - add exports
>> - added unit tests
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - test
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - Merge branch 'master' into 8092102.truncated
>> - labeled helper
>> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
> line 45:
>
>> 43: * in their skins to different code paths.
>> 44: */
>> 45: public class LabeledTruncatedTest {
>
> It might be worth adding a test for `Button`.
test added
> modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
> line 93:
>
>> 91: firePulse();
>> 92:
>> 93: assertFalse(control.isTextTruncated());
>
> Can you add a test for the case where we are wrapping and the preferred
> height is exceeded?
test added
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588474995
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588474917
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 18:34:04 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line >> 114: >> >>> 112: * by itself looks rather weird. >>> 113: */ >>> 114: private static boolean useActualContentWidth; >> >> Even given your comment about why it's safe to use a global variable, >> wouldn't it be cleaner to make it an instance variable? I suppose it might >> be OK to keep it global, but only if you can show that there are no issues >> with reentrancy. > > re-entrancy should not be an issue: the flag is used in the context of a > single method which is always being called from an fx application thread. > > I do not want to increase the memory footprint by making it an instance > variable. on second thought, you are right. we cannot guarantee that someone won't stick a TableView as a graphic into another TableView cell. Switched to use an ephemeral entry in `Node.getProperties()`, same as `Properties.DEFER_TO_PARENT_PREF_WIDTH`. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588476651
Re: RFR: 8092102: Labeled: truncated property [v11]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/062d47ac..6ea4c698 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=10 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=09-10 Stats: 54 lines in 5 files changed: 29 ins; 14 del; 11 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
Re: RFR: 8092102: Labeled: truncated property [v10]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - csr - Merge branch 'master' into 8092102.truncated - missing ) - review comments - Merge branch 'master' into 8092102.truncated - add exports - added unit tests - Merge remote-tracking branch 'origin/master' into 8092102.truncated - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - ... and 7 more: https://git.openjdk.org/jfx/compare/aa9907a5...062d47ac - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=09 Stats: 309 lines in 8 files changed: 279 ins; 19 del; 11 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
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 17:37:52 GMT, Kevin Rushforth wrote: >> Andy Goryachev has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 15 commits: >> >> - missing ) >> - review comments >> - Merge branch 'master' into 8092102.truncated >> - add exports >> - added unit tests >> - Merge remote-tracking branch 'origin/master' into 8092102.truncated >> - test >> - Merge remote-tracking branch 'origin/master' into 8092102.truncated >> - Merge branch 'master' into 8092102.truncated >> - labeled helper >> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e > > modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line > 114: > >> 112: * by itself looks rather weird. >> 113: */ >> 114: private static boolean useActualContentWidth; > > Even given your comment about why it's safe to use a global variable, > wouldn't it be cleaner to make it an instance variable? I suppose it might be > OK to keep it global, but only if you can show that there are no issues with > reentrancy. re-entrancy should not be an issue: the flag is used in the context of a single method which is always being called from an fx application thread. I do not want to increase the memory footprint by making it an instance variable. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588132322
Re: RFR: 8092102: Labeled: truncated property [v9]
On Thu, 2 May 2024 18:07:12 GMT, Kevin Rushforth wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - missing )
>> - review comments
>> - Merge branch 'master' into 8092102.truncated
>> - add exports
>> - added unit tests
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - test
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - Merge branch 'master' into 8092102.truncated
>> - labeled helper
>> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java
> line 33:
>
>> 31: * Labeled Helper.
>> 32: */
>> 33: public class LabeledHelper {
>
> You might consider making `LabeledHelper` a subclass of `ControlHelper`,
> which is the usual pattern for helper classes of nodes, although it may not
> matter here (and could be done later if there was a need).
Thinking on it further, let's not do this unless / until there is a need (there
likely won't be).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588118640
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 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-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 with a new target base due to a
> merge or a rebase. The pull request now contains 15 commits:
>
> - missing )
> - review comments
> - Merge branch 'master' into 8092102.truncated
> - add exports
> - added unit tests
> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
> - test
> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
> - Merge branch 'master' into 8092102.truncated
> - labeled helper
> - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e
The API changes look good. I left a couple comments on the API docs.
I also left a couple questions / comments on the fix and test. I haven't tested
it yet.
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java
line 33:
> 31: * Labeled Helper.
> 32: */
> 33: public class LabeledHelper {
You might consider making `LabeledHelper` a subclass of `ControlHelper`, which
is the usual pattern for helper classes of nodes, although it may not matter
here (and could be done later if there was a need).
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java
line 62:
> 60: * @return true to use the actual content width, false to delegate to
> the parent
> 61: */
> 62: public static boolean isUseActualContentWidth() {
If you make the `useActualContentWidth` flag an instance variable of `Labeled`
(see my comment in that class), then you would need to add the `Labeled` as an
argument to this method (here and in the accessor method).
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
114:
> 112: * by itself looks rather weird.
> 113: */
> 114: private static boolean useActualContentWidth;
Even given your comment about why it's safe to use a global variable, wouldn't
it be cleaner to make it an instance variable? I suppose it might be OK to keep
it global, but only if you can show that there are no issues with reentrancy.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
848:
> 846: /**
> 847: * Indicates whether the text has been truncated
> 848: * when it cannot fit into the available width.
"when" --> "because"
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
850:
> 848: * when it cannot fit into the available width.
> 849: *
> 850: * When truncated, the {@link #ellipsisStringProperty() ellipsis
> string}
I recommend either using `@linkplain` or changing the text to `ellipsisString`,
matching the name of the property.
modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
line 45:
> 43: * in their skins to different code paths.
> 44: */
> 45: public class LabeledTruncatedTest {
It might be worth adding a test for `Button`.
modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java
line 93:
> 91: firePulse();
> 92:
> 93: assertFalse(control.isTextTruncated());
Can you add a test for the case where we are wrapping and the preferred height
is exceeded?
-
PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2036421013
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588087048
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588090900
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588033608
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588065486
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1588067300
PR Review Comment:
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 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-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 with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e Looks good. I tested a few scenarios with TableView, Button, Label. It behaved as expected - Marked as reviewed by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2035862805
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 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-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 with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e Testing looks good. Works as expected. The change looks good. Please allow me a little more time to take a more closer look. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2085127845
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 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-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 with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e @azuev-java please review - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2083402516
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 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-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 with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e @aghaisas could you take a look at this PR please? - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2070351050
Re: RFR: 8092102: Labeled: truncated property [v9]
On Wed, 10 Apr 2024 21:25:10 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-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 with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - missing ) > - review comments > - Merge branch 'master' into 8092102.truncated > - add exports > - added unit tests > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e LGTM - Marked as reviewed by kpk (Committer). PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-2000588287
Re: RFR: 8092102: Labeled: truncated property [v8]
On Wed, 10 Apr 2024 21:28:16 GMT, Andy Goryachev wrote: >> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java >> line 38: >> >>> 36: /** >>> 37: * Returns true when the Labeled must compute the actual >>> content width in computePrefWidth(). >>> 38: * @return whether computePrefWidth() must compute the actual >>> content width >> >> Do you think re-wording the sentence to explain when this function returns >> true/false would be better here? > > is the new version clearer? Yes. Thanks for updating. - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1565508320
Re: RFR: 8092102: Labeled: truncated property [v8]
On Wed, 10 Apr 2024 09:03:33 GMT, Karthik P K wrote: >> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> add exports > > modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java > line 38: > >> 36: /** >> 37: * Returns true when the Labeled must compute the actual content >> width in computePrefWidth(). >> 38: * @return whether computePrefWidth() must compute the actual >> content width > > Do you think re-wording the sentence to explain when this function returns > true/false would be better here? is the new version clearer? - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1560075257
Re: RFR: 8092102: Labeled: truncated property [v9]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - missing ) - review comments - Merge branch 'master' into 8092102.truncated - add exports - added unit tests - Merge remote-tracking branch 'origin/master' into 8092102.truncated - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - Merge branch 'master' into 8092102.truncated - labeled helper - ... and 5 more: https://git.openjdk.org/jfx/compare/0eb4d719...aa28eb4e - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=08 Stats: 309 lines in 8 files changed: 279 ins; 19 del; 11 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
Re: RFR: 8092102: Labeled: truncated property [v8]
On Thu, 28 Mar 2024 21:44:49 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-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: > > add exports Overall this looks good to me. Left couple of inline comments. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java line 38: > 36: /** > 37: * Returns true when the Labeled must compute the actual content > width in computePrefWidth(). > 38: * @return whether computePrefWidth() must compute the actual > content width Do you think re-wording the sentence to explain when this function returns true/false would be better here? modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/LabeledHelper.java line 58: > 56: /** > 57: * Returns true when the Labeled must compute the actual content > width in computePrefWidth(). > 58: * @return whether computePrefWidth() must compute the actual content > width Same comment as above modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java line 42: > 40: /** > 41: * Tests textTruncated property of Labeled, using Label, TableCell, and > TreeTableCell controls > 42: * (the last two contain conditional code that redirects the execution of > computePrefWidth() Missing `)`? - PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1991164711 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1559103685 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1559108375 PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1559148976
Re: RFR: 8092102: Labeled: truncated property [v8]
On Thu, 28 Mar 2024 21:44:49 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-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: > > add exports unit tests added, please re-review. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-2026206381
Re: RFR: 8092102: Labeled: truncated property [v8]
> 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: add exports - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/45704b92..f98ff907 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=07 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=06-07 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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
Re: RFR: 8092102: Labeled: truncated property [v7]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - added unit tests - Merge remote-tracking branch 'origin/master' into 8092102.truncated - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - Merge branch 'master' into 8092102.truncated - labeled helper - handle tree/table view cells - Merge remote-tracking branch 'origin/master' into 8092102.truncated - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - ... and 1 more: https://git.openjdk.org/jfx/compare/7a4d2976...45704b92 - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=06 Stats: 306 lines in 7 files changed: 276 ins; 19 del; 11 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
Re: RFR: 8092102: Labeled: truncated property [v6]
On Wed, 27 Mar 2024 21:54:37 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-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 with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - test > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - Merge branch 'master' into 8092102.truncated > - labeled helper > - handle tree/table view cells > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - review comments > - Merge remote-tracking branch 'origin/master' into 8092102.truncated > - 8092102 Labeled: truncated property modules/javafx.controls/src/test/java/test/javafx/scene/control/LabeledTruncatedTest.java line 153: > 151: > 152: // FIX fails > 153: //assertEquals(Boolean.TRUE, truncated.get()); still WIP, I can't figure out why the table cells have width 0 - PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1542092066
Re: RFR: 8092102: Labeled: truncated property [v6]
> 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 with a new target base due to a merge or a rebase. The pull request now contains ten commits: - test - Merge remote-tracking branch 'origin/master' into 8092102.truncated - Merge branch 'master' into 8092102.truncated - labeled helper - handle tree/table view cells - Merge remote-tracking branch 'origin/master' into 8092102.truncated - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - 8092102 Labeled: truncated property - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=05 Stats: 357 lines in 5 files changed: 329 ins; 19 del; 9 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
Re: RFR: 8092102: Labeled: truncated property [v5]
> 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 with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge branch 'master' into 8092102.truncated - labeled helper - handle tree/table view cells - Merge remote-tracking branch 'origin/master' into 8092102.truncated - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - 8092102 Labeled: truncated property - Changes: https://git.openjdk.org/jfx/pull/1389/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1389=04 Stats: 189 lines in 4 files changed: 161 ins; 19 del; 9 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
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
Re: RFR: 8092102: Labeled: truncated property [v3]
> 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? > > **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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - handle tree/table view cells - Merge remote-tracking branch 'origin/master' into 8092102.truncated - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - 8092102 Labeled: truncated property - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/8a5cb17e..72f170b6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=01-02 Stats: 85 lines in 5 files changed: 72 ins; 4 del; 9 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
Re: RFR: 8092102: Labeled: truncated property [v2]
On Tue, 5 Mar 2024 14:16:59 GMT, Nir Lisker wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains three additional
>> commits since the last revision:
>>
>> - review comments
>> - Merge remote-tracking branch 'origin/master' into 8092102.truncated
>> - 8092102 Labeled: truncated property
>
> modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
> 850:
>
>> 848: protected boolean computeValue() {
>> 849: if (isWrapText()) {
>> 850: return false;
>
> Are you sure that allowing text to wrap necessarily means it won't be
> truncated? What if the max height doesn't allow another line?
Good point. So the label will have its text truncated (by inserting the
ellipsis string) when wrapText is on and the size is constrained by setting
maxHeight.
Interestingly, it will not truncate the text (again, ellipsis string) if the
label is resized by the layout:
Using the latest MonkeyTester to test
https://github.com/andy-goryachev-oracle/MonkeyTest
uncomment the listener in LabelPage
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1513493586
Re: RFR: 8092102: Labeled: truncated property [v2]
> 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? > > **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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - review comments - Merge remote-tracking branch 'origin/master' into 8092102.truncated - 8092102 Labeled: truncated property - Changes: - all: https://git.openjdk.org/jfx/pull/1389/files - new: https://git.openjdk.org/jfx/pull/1389/files/7cbd46c7..8a5cb17e Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1389=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1389=00-01 Stats: 1319 lines in 36 files changed: 1172 ins; 79 del; 68 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
Re: RFR: 8092102: Labeled: truncated property
On Tue, 5 Mar 2024 14:59:20 GMT, Nir Lisker wrote: > This enhancement should be discussed in the mailing list. I've never needed > this myself, but it already had an old JBS ticket, so I can see that others > did and for a legitimate reason (adding tooltips). I don't mind if it's added > or not. Yes, this warrants a mailing list discussion, especially given the following comment in the PR Description: > **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? I also don't mind one way or the other. If it is felt to be a useful convenience, especially to make it easier to optionally enable Tooltip generation, it seems OK to me. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-1979157887
Re: RFR: 8092102: Labeled: truncated property
On Mon, 4 Mar 2024 21:04:28 GMT, Andy Goryachev wrote:
> Adds Labeled.truncated 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
> - text
> - width
> - wrapText
>
> For some reason, line 859 generates a javadoc "co comment" warning, despite
> the javadoc comment present at the property declaration in line 832.
>
> 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?
Presuming we go forward with this, I left a couple comments on the API.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
834:
> 832: private ObservableBooleanValue truncated;
> 833:
> 834: public final ObservableBooleanValue truncatedProperty() {
The return type should be `ReadOnlyBooleanProperty`. That almost certainly
explains the other problem you are seeing, since javadoc does not recognize
this as a boolean property.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
859:
> 857: }
> 858: // FIX why does this method emit "warning: no comment" javadoc
> warning?
> 859: public final boolean isTruncated() {
See above.
-
PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1915597929
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1511958192
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1511958471
Re: RFR: 8092102: Labeled: truncated property
On Tue, 5 Mar 2024 09:52:37 GMT, Karthik P K wrote: > Do you think change in `labelPaddingProperty`or `graphicProperty` should also > trigger check good question! a change in these properties gets handled correctly - no need to add explicit binding. - PR Comment: https://git.openjdk.org/jfx/pull/1389#issuecomment-1979134552
Re: RFR: 8092102: Labeled: truncated property
On Mon, 4 Mar 2024 21:04:28 GMT, Andy Goryachev wrote:
> Adds Labeled.truncated 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
> - text
> - width
> - wrapText
>
> For some reason, line 859 generates a javadoc "co comment" warning, despite
> the javadoc comment present at the property declaration in line 832.
>
> 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?
This enhancement should be discussed in the mailing list. I've never needed
this myself, but it already had an old JBS ticket, so I can see that others did
and for a legitimate reason (adding tooltips). I don't mind if it's added or
not.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
823:
> 821: /**
> 822: * Truncated read-only property indicates whether the text has been
> truncated
> 823: * when it cannot fit into the available width.
No need to repeat the name and type in the first sentence as it's used in the
summary table. Most phrasings opt (inconsistently) for: "Indicates whether..."
or just "Whether..." (I prefer the former).
I'm also not sure about the sole role of the width. Can the text not be
truncated for a reason other than not fitting in the width, like not fitting in
the height?
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
830:
> 828: *
> 829: * @since 23
> 830: * @defaultValue false
I don't think that a default value is applicable here since it's completely
dependent on other properties and can't be set. It's possible to call the `get`
of this property for the first time and get `true`. The value here is the value
used in the get method:
return property == null ? DEFAULT_VALUE : property.get();
at least in all the cases I've seen.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
832:
> 830: * @defaultValue false
> 831: */
> 832: private ObservableBooleanValue truncated;
The property name should probably be `textTruncated` to be in line with
`textOverrun`, `textFill`, `wrapText`...
Otherwise it could look like the labeled is truncated.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
834:
> 832: private ObservableBooleanValue truncated;
> 833:
> 834: public final ObservableBooleanValue truncatedProperty() {
`ObservableBooleanValue` is not a property, so it's odd to use it as the return
type of one. Usually `ReadOnlyBooleanProperty` is used. This implementation
through binding is also unique for read-only properties. A look in `Node` and
`Labeled` show a different way of implementing read-only properties.
modules/javafx.controls/src/main/java/javafx/scene/control/Labeled.java line
850:
> 848: protected boolean computeValue() {
> 849: if (isWrapText()) {
> 850: return false;
Are you sure that allowing text to wrap necessarily means it won't be
truncated? What if the max height doesn't allow another line?
-
PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1917119989
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512963900
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512912954
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512968169
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512956759
PR Review Comment: https://git.openjdk.org/jfx/pull/1389#discussion_r1512898960
Re: RFR: 8092102: Labeled: truncated property
On Mon, 4 Mar 2024 21:04:28 GMT, Andy Goryachev wrote: > Adds Labeled.truncated 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 > - text > - width > - wrapText > > For some reason, line 859 generates a javadoc "co comment" warning, despite > the javadoc comment present at the property declaration in line 832. > > 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? Overall change looks good to me. Do you think change in `labelPaddingProperty`or `graphicProperty` should also trigger check in newly added truncated property? I believe the properties which are already bound should take care of this. Just wanted to confirm here. - PR Review: https://git.openjdk.org/jfx/pull/1389#pullrequestreview-1916415972
