SodiumFRP / sodium

Sodium - Functional Reactive Programming (FRP) Library for multiple languages
http://sodium.nz/
Other
848 stars 138 forks source link

busy should be false when initiation and completion events fire simultaneously. #116

Closed ownwaterloo closed 6 years ago

ownwaterloo commented 6 years ago

lookup is a asynchronous operation and events from sIn and sOut can't be simultaneous. So the current decision about merge is fine when we think about the whole Lookup application. But when we think about IsBusy locally, it is better to not assume events aren't simultaneous, right? busy should be false in the end of transaction when events are simultaneous (i.e., drop sIn).

ownwaterloo commented 6 years ago

It seems that the new IsBusy which drop sIn could be used in more situations. But I was failed to find out a real example. Here's my attempt.

First, we could use a fake lookup when test the application, i.e.:

public static final
Lambda1<Stream<String>,Stream<Optional<String>>> lookup = sWord ->
  sWord.map( s -> "Sodium".equals(s)
           ? Optional.of("a FRP library")
           : Optional.empty() );

The application won't work properly (i.e., keep busy) if sIn takes precedence. So the new IsBusy is useful for this case.

On the other hand, can we use a synchronous lookup to fake an asynchronous one in general? If the answer is no, the new IsBusy is not so attractive.

Next, I tried to implement a offline dictionary application. It initializes a predefined dictionary on startup:

public static final Map<String,String> predefined =
  Collections.unmodifiableMap(new HashMap<String,String>() {{
    put("Sodium", "a FRP library");
    put("Reactive Banana", "another FRP library");
    // more ...
  }});

public static final
Lambda1<Stream<String>, Stream<Optional<String>>> lookup = sWord ->
  sWord.map(s -> Optional.ofNullable(predefined.get(s)));

sIn and sOut are simultaneous now, and only the new IsBusy will work.

But for this application, results are immediately available and busy is always false. We could implement it without lookup, sOut and busy in the first place:

STextField word = new STextField("", 25);
// always true
Cell<Boolean> enabled = new Cell<>(true);
SButton button = new SButton("look up", enabled);
Stream<String> sWord = button.sClicked.snapshot(word.text);
// without new IsBusy<>(lookup, sWord)
Stream<String> sDefinition = sWord.map(s -> predefined.getOrDefault(s, "ERROR!"));
Cell<String> output = sDefinition.hold("");
STextArea outputArea = new STextArea(output, enabled);

Both the new IsBusy and the old one are not useful for this application.

After that, I tried to implement another dictionary application which will query the online dictionary only when the local dictionary doesn't have the word.

public static final
Lambda1<Stream<String>, Stream<Optional<String>>> lookup = sWord -> {
    StreamSink<Optional<String>> sDefinition = new StreamSink<>();
    Listener l = sWord.listenWeak(wrd -> {
        if (predefined.containsKey(wrd)) {
            sDefinition.send(Optional.of(predefined.get(wrd)));
            return;
        }
        new Thread() {
        // query the online dictionary
        }
    });
    return sDefinition.addCleanup(l);
};

But we shouldn't call send inside listen, right? We could call send inside another thread or inside Transaction.post:

        if (predefined.containsKey(wrd)) {
            Transaction.post(() -> sDefinition.send(Optional.of(predefined.get(wrd))));
            return;
        }

In both cases, sIn and sOut aren't simultaneous. So the new IsBusy makes no difference here.

In summary:

What do you think?

clinuxrulz commented 6 years ago

Looks good to me. IsBusy is more useful when it works for both async and sync.

If a side effect is known to be light weight, then there is no problem with sync.

the-real-blackh commented 6 years ago

Great - thank you.