SimpleVar notifies Observers with wrong value

[Grannath]

It seems I've found a nasty bug in 2.0-M5. When setValue(newVal) is called on SimpleVar, it updates the field value on SimpleVar, then calls invalidate() from its super class ValBase. This in turn notifies all Observers using its own field value. But the later only gets updated on getValue(), which was never called.

Maybe I'm missing something here. I don't get that AccuMap thing (yet). I'll try to come up with a test case later.

[TomasMikula]

Yes, please, try to come up with a test case.

[TomasMikula]

Probably the piece you are missing is that notifyObservers on Val should be called with the old value of Val. Each observable entity in ReactFX has one canonical observer that accepts a canonical description of the update (i.e. no special handling of InvalidationListener and ChangeListener). For Val, the canonical observer is the consumer of the old value—this is the Consumer<? super T> in

ValBase<T> extends ObservableBase<Consumer<? super T>, T>

A canonical description of the change is the old value (the second type argument to ObservableBase above—T).

Then both InvalidationListener and ChangeListener can be recovered from this kind of observer. The InvalidationListener just ignores the old value. The ChangeListener will need to call getValue() to obtain the new value.

[Grannath]

Morning,

I cannot commit from work, but I can at least paste something in here. For me, the following test case fails on 2.0-M5.

public class VarTest
{
    @Test
    public void checkSimpleVar()
    {
        final String firstVal = "foo";
        final String secondVal = "bar";

        final Var<String> var = Var.newSimpleVar(firstVal);

        // Check that only invalidation is fired when setting the same value again.
        final Subscription sub1 = Subscription.multi(
                var.observe(it -> fail("Observer was notified without change.")),
                var.observeInvalidations(it -> assertEquals("Invalidation Observer was notified with wrong Observable.", var, it)),
                var.observeChanges((obs, oldVal, newVal) -> fail("Change Observer was notified without change.")));
        var.setValue(firstVal);
        sub1.unsubscribe();

        // check that everyone receives the right values on a real change
        final Subscription sub2 = Subscription.multi(
                var.observe(it -> assertEquals("Observer was notified with wrong value.", secondVal, it)),
                var.observeInvalidations(it -> assertEquals("Invalidation Observer was notified with wrong Observable.", var, it)),
                var.observeChanges((obs, oldVal, newVal) ->
                {
                    assertEquals("Change Observer was notified with wrong Observable.", var, obs);
                    assertEquals("Change Observer was notified with wrong old value.", firstVal, oldVal);
                    assertEquals("Change Observer was notified with wrong new value.", secondVal, newVal);
                }));
        var.setValue(secondVal);
        sub2.unsubscribe();
    }
}

And it fails pretty hard. The Observer (from Var.observe()) gets the old value instead of the new. Even better, the Invalidation Observer (from Var.observeInvalidations()) gets notified with the old value as well instead of the Observable.

Interestingly, the later method works the first time, when no actual change occured. I have no idea yet how exactly this happens.

[TomasMikula]

Excuse my brevity, it is very late night where I am, but everything seems to work as it should. The observer registered with observe is expected to receive the old value (see my previous comment). The observeInvalidations method does not take an InvalidationListener as you might expect (see it's signature), but a consumer of the old value as well. In those assertions, you are comparing a Var to String, no wonder it fails.

[Grannath]

Oh, interesting, I read that as an implementation detail, sorry. So, what's the point then to have observeInvalidations() and observe() if they both behave basically the same? Also, it's easy to confuse this due to EventStream.observe(), which behaves very differently from Val.observe(). And of course there is the name clash between JavaFX and ReactFX on observing invalidations.

Well, you go to sleep, I will think of an idea to make the API a little clearer. ;)

[Grannath]

OK, my thoughts so far.

First of, I've found the documentation on Observable. I don't see it in my IDE when calling observe() on some other class, that's how I missed it. You did a good job on describing the situation there.

I see the point to have one class or interface that does the observer management. But the two most prominent examples for Observables are Val/Var and EventStream, which are two very different concepts. Having them share the interface Observable is basically asking for trouble. There cannot possibly be a contract on the interface on what the oberver receives. That is not what I would expect from an interface, which should define the behaviour as far as possible so that I can treat two different implementations the same.

It might be beneficial to use two different interfaces to describe streams and observable values (in this case meaning just the concept, not any implementation). That is where the main "rift" is, between stateful and stateless. Different types of observable values can probably agree on some common type of listener, and so can different streams. That would mean some code duplication, but currently I think it might be worth it.

Of course, there is the Observable from JavaFX, which has the absolute minimal common denominator for a listener. The parameter is pretty much useless unless you reuse the observer. But it allows to have an actual contract on the interface methods on implementation behaviour. This would work on both Val/Var and EventStream, but would also make the listener much less powerful.

As for the rest, having observe() and observeInvalidations() makes me expect a difference between them. Especially when considering my point above. observeChanges() suddenly conformes to JavaFX again. Not that I'm against that out of principle. But it's sets a precedent it probably shouldn't. Also, just giving the old and new value is much shorter.

Wow, this sounds more like a rant than I meant to, sorry for that. I like working with ReactFX, so I get a bit passionate.

In short, I think the best version for Val/Var would be observeInvalidations(Consumer<? super T>) to receive old values and observeChanges(BiConsumer<? super T, ? super T>) with old and new values, nothing else. Maybe an observeValues(Consumer<? super T>) to receive new values, but I'm not sure if that is a real benefit. All of this is of course very much a breaking change in the API, after several milestone releases.

Because EventStreams don't hold values, having a simple observe() method is fine. There really cannot be any confusion on the argument in this case.

Don't get me wrong, I'm not actually suggesting to do all this. But at the very least, I hope it gives a few hints to writing the documentation.

[TomasMikula]

The methods on Observable are meant to be used mainly by implementations, not directly by the user. Too bad an interface method cannot be protected, but maybe one day they will. (Java 9 will allow private (and thus final) methods on interfaces. Maybe someday there will be support for protected abstract methods as well.)

In short, I think the best version for Val/Var would be observeInvalidations(Consumer<? super T>) to receive old values and observeChanges(BiConsumer<? super T, ? super T>) with old and new values, nothing else.

Apart from the observeChanges signature, which, as you noted, somewhat surprisingly accepts a ChangeListener from JavaFX, this is basically the idea. Except add to that list the observe method that I don't know how to hide from the public, and the methods required to conform to javafx.beans.value.ObservableValue, and you get somewhere close to where we are.

I think I would accept a pull request changing the argument of observeChanges to the BiConsumer you propose.

[TomasMikula]

That is where the main "rift" is, between stateful and stateless.

There is a common denominator—notification about an event. That in some cases the notification describes a state change, doesn't matter.

Different types of observable values can probably agree on some common type of listener, and so can different streams.

Even if they don't agree on the type of listener, the listener management turns out to be the same (though it took some effort to get there, to have a common base for all of EventStream, Val and LiveList).

I think the only problem is the inability to hide the Observable interface from the end user, but I guess I can live with that.