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.

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
Closed
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Adjust Node
- Fixed javadoc
- Added comment for code that avoid eager instantiation
- Changed `isShown` to use property if it is available
  • Loading branch information
hjohn committed Dec 1, 2022
commit 5c22fdd04de21a0402fefd3ee3887ea9ee3b3daf
17 changes: 11 additions & 6 deletions modules/javafx.graphics/src/main/java/javafx/scene/Node.java
Original file line number Diff line number Diff line change
@@ -1201,6 +1201,7 @@ public final void setId(String value) {
* @defaultValue null
* @see <a href="doc-files/cssref.html">CSS Reference Guide</a>
*/
@Override
public final String getId() {
return id == null ? null : id.get();
}
@@ -1310,6 +1311,7 @@ public final void setStyle(String value) {
* an empty String is returned.
* @see <a href="doc-files/cssref.html">CSS Reference Guide</a>
*/
@Override
public final String getStyle() {
return style == null ? "" : style.get();
}
@@ -1407,7 +1409,7 @@ public String getName() {

/**
* Indicates whether or not this {@code Node} is shown. A node is considered shown if it's
* part of a {@code Scene} which is part of a {@code Window} whose
* part of a {@code Scene} that is part of a {@code Window} whose
* {@link Window#showingProperty showing property} is {@code true}. The {@link Node#visibleProperty visibility}
* of the node or its scene does not affect this property.
* <p>
@@ -1417,16 +1419,19 @@ public String getName() {
* This property can also be used to start animations when the node is shown, and to stop them
* when it is no longer shown.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence sounds like shownProperty() is quite deeply connected to animations, which it isn't. Maybe you can clarify with "For example, ..."?

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 find it reads really weird when I just make it an example (without putting a code example below it); I only added these to indicate possible use cases.

Perhaps if I rephrase it to "This property can also be useful to perform actions when the node is shown or no longer shown, like starting and stopping animations".

*
* @see ObservableValue#when(ObservableValue)
* @since 20
*/
private ReadOnlyBooleanProperty shown;

public final boolean isShown() {
Scene s = getScene();
if (s == null) return false;
Window w = s.getWindow();
return w != null && w.isShowing();
if (shown == null) { // avoid eager instantiation of property
Scene s = getScene();
if (s == null) return false;
Window w = s.getWindow();
return w != null && w.isShowing();
}

return shown.get();
}

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

Choose a reason for hiding this comment

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

displayed/displaying?