Skip to content

Fix property change when undefined value is explicitly set to NaN#273

Open
dyrpsf wants to merge 2 commits intosbmlteam:masterfrom
dyrpsf:property-change-nan
Open

Fix property change when undefined value is explicitly set to NaN#273
dyrpsf wants to merge 2 commits intosbmlteam:masterfrom
dyrpsf:property-change-nan

Conversation

@dyrpsf
Copy link
Contributor

@dyrpsf dyrpsf commented Jan 9, 2026

Summary

This PR addresses sbmlteam/jsbml#255.

When a previously undefined double value (internally represented as NaN) is explicitly
set to NaN, no TreeNodeChangeListener event is fired. This happens because
firePropertyChange compares the old and new values using equals(), and for
Double.NaN, oldValue.equals(newValue) returns true, so no change is detected.

To fix this, the check in AbstractTreeNode.firePropertyChange is extended to also
consider reference inequality:

if ((oldValue == null) && (newValue != null)) {
  changeType = 0; // element added
} else if ((oldValue != null) && (newValue == null)) {
  changeType = 1; // element removed
} else if ((oldValue != null) &&
           (!oldValue.equals(newValue) || oldValue != newValue)) {
  changeType = 2; // real property change
}

This ensures that a property change event is fired even when equals() considers
the two values equal (e.g. NaN vs NaN), but the underlying objects differ.

Verification
Built JSBML and ran the core tests using:

mvn test -pl core -am

All jsbml-core tests pass, so the change does not introduce regressions
in the existing test suite.

draeger

This comment was marked as duplicate.

Copy link
Member

@draeger draeger left a comment

Choose a reason for hiding this comment

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

Are there any test cases for the behavior of property-change events? If not, it would be good to create some. It is also possible to just create an issue for this task, so we can close this PR sooner.

@dyrpsf
Copy link
Contributor Author

dyrpsf commented Mar 12, 2026

I’ve added a regression test (PropertyChangeNaNTest) that attaches a SimpleTreeNodeChangeListener to a Parameter, then calls setValue(Double.NaN) when the internal value is already NaN, and verifies that a property-change event is fired.

mvn -pl core test passes on this branch.

@dyrpsf dyrpsf requested a review from draeger March 12, 2026 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No property change happens when a value is explicitly set to NaN that was undefined before

2 participants