Community
Participate
Working Groups
Many observable types in the databinding library overload hashCode and equals methods with variants that return true iff the content of the observables match. This is incorrect for observables since two observables with the same content are not interchangeable. This bad implementation of hashCode and equals has a ripple effect on the rest of the library: - Observables can't be used in normal sets, maps, collections, or any other utility that invokes hashcode or equals. - Databinding has worked around this internally often doing comparisons using "==" rather than ".equals". However, this creates problems for generic types that could either be used by a databinding class or primitive. For example, WritableValue has a check for redundant setter invocations that uses "==", but this means WritableValues can easily become part of an infinite cycle that repeatedly sets the same string with different identity to the same observable. - Databinding has also worked around this by implementing custom collections: IdentityMap, IdentityObservableSet, IdentitySet, and IdentityWrapper. Most of these classes can be deleted and replaced with normal sets and maps once this is fixed. - This also blocks implementation of the graph-based change propagation algorithm I'm working on, which relies on correct hashCode and equals methods. Changes requested: - Delete the hashCode and equals overrides for all IObservable implementations. - Check all occurrences of "==" in databinding classes - particularly all the cases that are invoking it on a generic parameter type. Change them to Objects.equals. - Delete the Identity* classes and replace their usages with traditional collection classes (verify each occurrence to confirm that it's appropriate -- I suspect in all cases it will be).
This seems to cause issues with the ISideEffect API. The setValue on an observable calls back to the UI and that makes the cursor jumps. Workaround: sideEffectFactory.create(observeSummary::getValue, txtSummaryTarget::setValue); sideEffectFactory.create(txtSummaryTarget::getValue, summary -> { todo.ifPresent(t -> { t.setSummary(summary); if (dirtyable != null) { dirtyable.setDirty(dirty); } }); }); @Simon, Stefan, can fix be fixed? I think we should downport that to 4.6.1
Simon, what's your bugfix queue look like at the moment? Any chance you could look at this?
New Gerrit change created: https://git.eclipse.org/r/83959
Seems to me that the issue is not serious enough to get in an RC, but would still be worth being considerated for a maintenance release.
Removing target milestone for all bugs that are not major or above.
> Removing target milestone for all bugs that are not major or above. Of course I meant "major or below". Sorry for the noise!
Jens, is this something for you?
(In reply to Lars Vogel from comment #7) > Jens, is this something for you? It look interesting to me, but unfortunately I won't have time any time soon.
Having though about this for a bit, I have come to the conclusion that it is not possible to change the semantics of `equals` in the desired way without breaking a massive amount of old code. For example, I just wrote a unit test that looked like this: assertEquals(List.of(1, 2, 3), someObservableList); All such code would break. I don't see any way such breakage could be avoided, and I don't think it would be tolerable.
(In reply to Jens Lideström from comment #9) > Having though about this for a bit, I have come to the conclusion that it is > not possible to change the semantics of `equals` in the desired way without > breaking a massive amount of old code. > > For example, I just wrote a unit test that looked like this: > > assertEquals(List.of(1, 2, 3), someObservableList); > > All such code would break. > > I don't see any way such breakage could be avoided, and I don't think it > would be tolerable. So this bug should be marked as wontfix?
(In reply to Lars Vogel from comment #10) > So this bug should be marked as wontfix? Yes, I think so. * Doing this for the observable collections would change their specification and silently break a lot of client code. * The observable value classes in the framework already do not override equals, so there is no change to be done for them. The following important observable value classes do *not* override equals: AbstractObservableValue WritableValue SimplePropertyObservableValue ConstantObservableValue ComputedValue The following value do override equals, but delegates it to the decorated value: DecoratingObservableValue There might be other updates to how equals and == are used in framework that might be worth considering. But I don't think this bug report is about such other updates.
Thanks Jens. I'm closing it and abandoning the related Gerrit change.
