Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8290040: Provide simplified deterministic way to manage listeners #830

Closed
wants to merge 18 commits into from
+400 −2
Closed

8290040: Provide simplified deterministic way to manage listeners #830

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4c8453a
Fix bug where ObjectBinding returns null when revalidated immediately
hjohn Jul 14, 2022
3485656
Change explanatory comment to block comment
hjohn Jul 15, 2022
a70161c
Add conditional bindings to ObservableValue
hjohn Jul 15, 2022
fd19fc3
Rename showing property to shown as it is already used in subclasses
hjohn Jul 15, 2022
07e9d88
Update javadoc of "when" with better phrasing
hjohn Sep 1, 2022
13c6e39
Small wording fixes
hjohn Sep 4, 2022
2353852
Add missing new line
hjohn Sep 22, 2022
44d2ced
Fix review comments
hjohn Sep 29, 2022
4fa45c7
Merge remote-tracking branch 'origin/master' into
hjohn Sep 29, 2022
ffba692
Improve documentation of shown property
hjohn Oct 26, 2022
ea54699
Fix comment in test
hjohn Oct 26, 2022
46f206a
Fix javadoc error
hjohn Oct 26, 2022
5c22fdd
Adjust Node
hjohn Dec 1, 2022
67c277c
Improve wording in javadoc and comments
hjohn Dec 2, 2022
94b50b5
Remove changes to javafx.graphics Node
hjohn Dec 11, 2022
75ccca6
Remove example referencing Node#shownProperty
hjohn Dec 11, 2022
24872f0
Merge branch 'openjdk:master' into feature/conditional-bindings
hjohn Dec 12, 2022
7318af2
Fix formatting in test
hjohn Dec 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
View file
Original file line number Diff line number Diff line change
@@ -277,7 +277,7 @@ default <U> ObservableValue<U> flatMap(Function<? super T, ? extends ObservableV
* <pre>{@code
* ObservableValue<Boolean> condition = new SimpleBooleanProperty(true);
* ObservableValue<String> globalProperty = new SimpleStringProperty("A");
* ObservableValue<String> whenProperty = property.when(isShowing);
* ObservableValue<String> whenProperty = property.when(condition);
*
* // observe whenProperty, which will in turn observe globalProperty
* whenProperty.addChangeListener((ov, old, current) -> System.out.println(current));
@@ -299,8 +299,8 @@ default <U> ObservableValue<U> flatMap(Function<? super T, ? extends ObservableV
* Label label = ... ;
* ObservableValue<String> globalProperty = new SimpleStringProperty("A");
*
* // bind label's text to a global property only when it is showing:
* label.textProperty().bind(globalProperty.when(label::isShowingProperty));
* // bind label's text to a global property only when it is shown:
* label.textProperty().bind(globalProperty.when(label::isShownProperty));
* }</pre>
* @param condition a boolean {@code ObservableValue}, cannot be {@code null}
* @return an {@code ObservableValue} that holds this value whenever the given
14 changes: 7 additions & 7 deletions modules/javafx.graphics/src/main/java/javafx/scene/Node.java
View file
Original file line number Diff line number Diff line change
@@ -1410,25 +1410,25 @@ public String getName() {
* @defaultValue false
* @since 20
*/
private ReadOnlyBooleanProperty showing;
private ReadOnlyBooleanProperty shown;

public final boolean isShowing() {
public final boolean isShown() {
Scene s = getScene();
if (s == null) return false;
Window w = s.getWindow();
return w != null && w.isShowing();
}

public final ReadOnlyBooleanProperty showingProperty() {
if (showing == null) {
public final ReadOnlyBooleanProperty shownProperty() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property probably should have been named showing to align it with Window.showing, but the "good name" is already taken by ComboBox and other controls. Just out of interest, have you considered alternatives to shown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look too far for alternative names after I discovered showing would not be possible. The name comes from isTreeShowing which is used for a similar purpose (inside Node) and from conditionOnShowing in ReactFX.

The name needs to imply that visibility has no effect on it (ie, setVisible(false) won't toggle it). Neither does it check if the node isn't covered or off screen.

In theory you could use a more general name (like active as in "part of an active currently showing scene graph"). isActive seems to even be available...

A name like "used" or "inUse" may also work (as in "indicates the node is currently used as part of a currently showing scene graph".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That reminds me... parent is described as:

The parent of this {@code Node}. If this {@code Node} has not been added to a scene graph, then parent will be null.

Which I think is incorrect; parent can easily be non-null while not being part of a scene graph.

Copy link
Collaborator

@nlisker nlisker Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

displayed/displaying?

if (shown == null) {
ObservableValue<Boolean> ov = sceneProperty()
.flatMap(Scene::windowProperty)
.flatMap(Window::showingProperty);

showing = new ReadOnlyBooleanDelegate(Node.this, "showing", ov);
shown = new ReadOnlyBooleanDelegate(Node.this, "shown", ov);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shownProperty() might be used quite a lot, have you considered using a specialized implementation that doesn't need several intermediate observable values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is already optimized enough by being lazily instantiated (ie. it won't be created if never used). As a second layer, these are fluent bindings which only register listeners when observed.

Creating a specialized implementation should IMHO replicate the lazy behavior (to avoid too many listeners on window/scene when the property isn't actually observed) which is non-trivial and would require an extensive test. This risks creating bugs now (or later) of which there have been quite a few with similar custom properties -- it's easy to accidentally cache an "old" value or forgetting to clear a cached "current" value when invalidated.

In short, I think optimized custom implementations have been and still are a source of bugs, with no evidence that these optimizations are worth while..

}

return showing;
return shown;
}

// Candidate to make publicly available or to add as a convience method to ObservableValue
@@ -8470,7 +8470,7 @@ void markDirtyLayoutBranch() {
}

final boolean isTreeShowing() {
return isTreeVisible() && isShowing();
return isTreeVisible() && isShown();
}

private void updateTreeVisible(boolean parentChanged) {
48 changes: 24 additions & 24 deletions modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java
View file
Original file line number Diff line number Diff line change
@@ -1265,51 +1265,51 @@ public void testIsShowingProperty() {
Scene s = new Scene(g);
Stage st = new Stage();

assertFalse(g.isShowing());
assertFalse(c.isShowing());
assertFalse(g.showingProperty().get());
assertFalse(c.showingProperty().get());
assertFalse(g.isShown());
assertFalse(c.isShown());
assertFalse(g.shownProperty().get());
assertFalse(c.shownProperty().get());

st.show();
st.setScene(s);
SceneShim.scenePulseListener_pulse(s);

assertTrue(g.isShowing());
assertTrue(c.isShowing());
assertTrue(g.showingProperty().get());
assertTrue(c.showingProperty().get());
assertTrue(g.isShown());
assertTrue(c.isShown());
assertTrue(g.shownProperty().get());
assertTrue(c.shownProperty().get());

g.setVisible(false); // irrelevant change for isShowing
SceneShim.scenePulseListener_pulse(s);

assertTrue(g.isShowing());
assertTrue(c.isShowing());
assertTrue(g.showingProperty().get());
assertTrue(c.showingProperty().get());
assertTrue(g.isShown());
assertTrue(c.isShown());
assertTrue(g.shownProperty().get());
assertTrue(c.shownProperty().get());

s.setRoot(new Group());
SceneShim.scenePulseListener_pulse(s);

assertFalse(g.isShowing());
assertFalse(c.isShowing());
assertFalse(g.showingProperty().get());
assertFalse(c.showingProperty().get());
assertFalse(g.isShown());
assertFalse(c.isShown());
assertFalse(g.shownProperty().get());
assertFalse(c.shownProperty().get());

s.setRoot(g);
SceneShim.scenePulseListener_pulse(s);

assertTrue(g.isShowing());
assertTrue(c.isShowing());
assertTrue(g.showingProperty().get());
assertTrue(c.showingProperty().get());
assertTrue(g.isShown());
assertTrue(c.isShown());
assertTrue(g.shownProperty().get());
assertTrue(c.shownProperty().get());

st.hide();
SceneShim.scenePulseListener_pulse(s);

assertFalse(g.isShowing());
assertFalse(c.isShowing());
assertFalse(g.showingProperty().get());
assertFalse(c.showingProperty().get());
assertFalse(g.isShown());
assertFalse(c.isShown());
assertFalse(g.shownProperty().get());
assertFalse(c.shownProperty().get());
}

@Test