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
RFR: 8331616: ChangeListener is not triggered when the InvalidationListener is removed
This PR provides a fix for the linked issue. The issue was that when an invalidation listener is removed, and the `ExpressionHelper` type changes from `Generic` to `SingleChange` that it would copy the current value of the `Generic` instance before it was updated (this is because invalidation listeners trigger before change listeners and the current value would only be updated **after** invalidation listeners notifications were completed). The code now will update the current value before sending out any notifications **if** there are change listeners present to head off this problem. Added a few test cases to verify this. Note: the PR which replaces `ExpressionHelper` does not have this problem: https://github.com/openjdk/jfx/pull/1081 - Commit messages: - Fix problem in ExpressionHelper that could copy the wrong current value Changes: https://git.openjdk.org/jfx/pull/1448/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1448=00 Issue: https://bugs.openjdk.org/browse/JDK-8331616 Stats: 69 lines in 2 files changed: 66 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1448.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1448/head:pull/1448 PR: https://git.openjdk.org/jfx/pull/1448
Re: RFR: 8290310: ChangeListener events are incorrect or misleading when a nested change occurs [v12]
> This provides and uses a new implementation of `ExpressionHelper`, called > `ListenerManager` with improved semantics. > > # Behavior > > |Listener...|ExpressionHelper|ListenerManager| > |---|---|---| > |Invocation Order|In order they were registered, invalidation listeners > always before change listeners|(unchanged)| > |Removal during Notification|All listeners present when notification started > are notified, but excluded for any nested changes|Listeners are removed > immediately regardless of nesting| > |Addition during Notification|Only listeners present when notification > started are notified, but included for any nested changes|New listeners are > never called during the current notification regardless of nesting| > > ## Nested notifications: > > | |ExpressionHelper|ListenerManager| > |---|---|---| > |Type|Depth first (call stack increases for each nested level)|(same)| > |# of Calls|Listeners * Depth (using incorrect old values)|Collapses nested > changes, skipping non-changes| > |Vetoing Possible?|No|Yes| > |Old Value correctness|Only for listeners called before listeners making > nested changes|Always| > > # Performance > > |Listener|ExpressionHelper|ListenerManager| > |---|---|---| > |Addition|Array based, append in empty slot, resize as needed|(same)| > |Removal|Array based, shift array, resize as needed|(same)| > |Addition during notification|Array is copied, removing collected > WeakListeners in the process|Appended when notification finishes| > |Removal during notification|As above|Entry is `null`ed (to avoid moving > elements in array that is being iterated)| > |Notification completion with changes|-|Null entries (and collected > WeakListeners) are removed| > |Notifying Invalidation Listeners|1 ns each|(same)| > |Notifying Change Listeners|1 ns each (*)|2-3 ns each| > > (*) a simple for loop is close to optimal, but unfortunately does not provide > correct old values > > # Memory Use > > Does not include alignment, and assumes a 32-bit VM or one that is using > compressed oops. > > |Listener|ExpressionHelper|ListenerManager|OldValueCaching ListenerManager| > |---|---|---|---| > |No Listeners|none|none|none| > |Single InvalidationListener|16 bytes overhead|none|none| > |Single ChangeListener|20 bytes overhead|none|16 bytes overhead| > |Multiple listeners|57 + 4 per listener (excluding unused slots)|57 + 4 per > listener (excluding unused slots)|61 + 4 per listener (excluding unused > slots)| > > # About nested changes > > Nested changes are simply changes that are made to a property that is > currently in the process of notifying its listeners. This... John Hendrikx has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 27 commits: - Merge remote-tracking branch 'upstream/master' into feature/nested-emission-with-correct-old-values - Fix generic warnings - Fix merge - Merge remote-tracking branch 'upstream/master' into feature/nested-emission-with-correct-old-values - Prevent removal of weak listeners during unlock - Use an overridable method to store latest value - Merge the recursive notification loop code Made loop in ListenerList slightly more generic to allow merging the logic in OldValueCachingListenerList and ListenerList; performance impact seems minimal - Small bug fix in OldValueCachingListenerList - Added a test case to detect and avoid this problem - Improve doc - Move listener call code to ListListenerBase - ... and 17 more: https://git.openjdk.org/jfx/compare/54005125...ffa9b299 - Changes: https://git.openjdk.org/jfx/pull/1081/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1081=11 Stats: 4358 lines in 39 files changed: 4195 ins; 7 del; 156 mod Patch: https://git.openjdk.org/jfx/pull/1081.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1081/head:pull/1081 PR: https://git.openjdk.org/jfx/pull/1081
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: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v3]
On Wed, 13 Mar 2024 09:13:20 GMT, Florian Kirmaier wrote: >> Florian Kirmaier has updated the pull request incrementally with one >> additional commit since the last revision: >> >> reverted accidental change in the .idea folder > > *push* > It's still a important bug. > If someone can guide me on how to write tests for this, i would put in the > effort. > Maybe we could make snapshot based tests? That should work on all platforms, > right? > (Like testing whether a specific RGB value appears) > Has something in this direction been done somewhere? I added a test that shows that the @FlorianKirmaier's fix works. You can start it with: `gradlew -PFULL_TEST=true :systemTests:test --tests NGNodeDirtyFlagTest`. To avoid such errors in the future, I would still suggest the refactoring, which I wrote about in my last comment. @kevinrushforth and @arapte please review. - PR Comment: https://git.openjdk.org/jfx/pull/1310#issuecomment-2092749767
Re: RFR: 8322619: Parts of SG no longer update during rendering - overlapping - culling - dirty [v5]
> In some situations, a part of the SG is no longer rendered. > I created a test program that showcases this problem. > > Explanation: > > This can happen, when a part of the SG, is covered by another Node. > In this part, one node is totally covered, and the other node is visible. > > When the totally covered Node is changed, then it is marked dirty and it's > parent, recursively until an already dirty node is found. > Due to the Culling, this totally covered Node is not rendered - with the > effect that the tree is never marked as Clean. > > In this state, a Node is Dirty but not It's parent. Based on my CodeReview, > this is an invalid state which should never happen. > > In this invalid state, when the other Node is changed, which is visible, then > the dirty state is no longer propagated upwards - because the recursive > "NGNode.markTreeDirty" algorithm encounters a dirty node early. > > This has the effect, that any SG changes in the visible Node are no longer > rendered. Sometimes the situation repairs itself. > > Useful parameters for further investigations: > -Djavafx.pulseLogger=true > -Dprism.printrendergraph=true > -Djavafx.pulseLogger.threshold=0 > > PR: > This PR ensures the dirty flag is set to false of the tree when the culling > is used. > It doesn't seem to break any existing tests - but I'm not sure whether this > is the right way to fix it. > It would be great to have some feedback on this solution - maybe guiding me > to a better solution. > > I could write a test, that just does the same thing as the test application, > but checks every frame that these nodes are not dirty - but maybe there is a > better way to test this. Florian Kirmaier has updated the pull request incrementally with two additional commits since the last revision: - JDK-8322619: Add test - Revert "JDK-8322619: Clear dirty flag on the node and all its children if they are skipped due to visible==false or opacity==0" This reverts commit 5f9f1574515c078c1fd0dccd476325090a0b284d. - Changes: - all: https://git.openjdk.org/jfx/pull/1310/files - new: https://git.openjdk.org/jfx/pull/1310/files/5f9f1574..e3163f30 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1310=04 - incr: https://webrevs.openjdk.org/?repo=jfx=1310=03-04 Stats: 204 lines in 2 files changed: 190 ins; 8 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1310.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1310/head:pull/1310 PR: https://git.openjdk.org/jfx/pull/1310
Integrated: 8271865: SortedList::getViewIndex behaves not correctly for some index values
On Sun, 24 Mar 2024 15:11:29 GMT, drmarmac wrote: > This PR adds the missing checks, as well as code documentation that an > IndexOutOfBoundsException may be thrown. This pull request has now been integrated. Changeset: 54005125 Author:drmarmac <6900949+drmar...@users.noreply.github.com> Committer: Lukasz Kostyra URL: https://git.openjdk.org/jfx/commit/54005125a3f45489d3071eafd7f219ae621ff7c7 Stats: 54 lines in 5 files changed: 38 ins; 2 del; 14 mod 8271865: SortedList::getViewIndex behaves not correctly for some index values Reviewed-by: lkostyra, kcr - PR: https://git.openjdk.org/jfx/pull/1432
Re: RFR: 8271865: SortedList::getViewIndex behaves not correctly for some index values [v3]
On Thu, 2 May 2024 13:12:09 GMT, drmarmac wrote: >> This PR adds the missing checks, as well as code documentation that an >> IndexOutOfBoundsException may be thrown. > > drmarmac 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: > > - Merge remote-tracking branch 'refs/remotes/origin/master' into > fixes/transformation-list-oob > - use `@code` syntax > - JavaDoc updates > - TransformationList.getSourceIndex/getViewIndex should throw IOOBE > - Add failing test LGTM - Marked as reviewed by lkostyra (Committer). PR Review: https://git.openjdk.org/jfx/pull/1432#pullrequestreview-2037632992
