RHBA-415: After rename, designer is not marked as dirty more #842
Conversation
| @@ -49,7 +49,7 @@ public void fireChangeTitleEvent() { | |||
| if (dtPath == null) { | |||
| return; | |||
| } | |||
| if (dtPath.equals(path)) { | |||
| if (path.equals(dtPath)) { | |||
There was a problem hiding this comment.
Out of interest, why did you need to change the order of the equality check?
Both parameters were checked for null before the comparison.
There was a problem hiding this comment.
@manstis yeah, this change is tricky. Here we want to use the .equals method from the PathImpl (path), not from the ObservablePath (dtPath).
(I covered it with tests to avoid some regression, if we change the line 52 back to the if (dtPath.equals(path)), the testFireChangeTitleEvent_LockInfoUpdateForActiveDecisionTable breaks) :-)
There was a problem hiding this comment.
@karreiro Sounds like an equals(..) implementation is broken somewhere :-(
https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals(java.lang.Object)
"It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true."
I wonder whether it is better to fix the offending implementation?
There was a problem hiding this comment.
@manstis Definitely, the implementation of the ObservablePathImpl#equals is not symmetric. Sometimes it uses the o.original, sometime it uses the o (here) :-(
My first approach was to change it. However, the current implementation is widely used and the instability that I created does worth. So, instead of that, I preferred to make a specific/small fix with less impact.
I definitely agree that ObservablePathImpl#equals needs to be refactored, but I think we could raise another JIRA for that.
See https://issues.jboss.org/browse/RHBA-415
Before:
After:
Part of an ensemble: