From a09cdbdeea2c5edfc50d0dc8d5d26fc299175ab1 Mon Sep 17 00:00:00 2001
From: Joel Mason <jobba1@hotmail.com>
Date: Mon, 15 May 2017 21:14:39 +1000
Subject: [PATCH] Fixes flatten when current_node is created after the flatten

Flatten now uses bind! which handles this case well
---
 doc/dev notes.md |  4 ++--
 src/operators.jl | 34 ++++++++++++++++++++--------------
 test/flatten.jl  | 31 +++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/doc/dev notes.md b/doc/dev notes.md
index ff08172..33c60a6 100644
--- a/doc/dev notes.md	
+++ b/doc/dev notes.md	
@@ -11,12 +11,12 @@ Action updates the node whose `actions` Vector it's in with `set_value!` (pre-up
 1. merge (multiple parents)
 1. previous (caches the previous update)
 1. droprepeats (calls deactivate! when value(input) == prev_value)
-1. flatten (wire_flatten and set_flatten_val both get run whenever either the `current_node` or the `input` sigsig update. Would it be better to just add those actions to the input and current_node update respectively?)
+1. flatten (wire_flatten gets run whenever `input` sigsig updates. Uses `bind!` to update the flatten if the signal that is the `input` sigsig's current value (`current_node`) has its value updated).
 
 Action on an auxilliary node connected to the input that pushes to it, this allows the action to run, even if the node is a non-input, but gets pushed to
 1. throttle/debounce (the action is on a foreach on the input node, which sets up a timer to push to the throttle output node)
 1. delay (the action is on a foreach on the input node, which just pushes to the delay node)
-1. bind! (action is a `map` on the src node, which calls set_value! on the dest node, and returns nothing). , e.g. from test/basics.jl "non-input bind":
+1. bind! (action is a `map` on the src node, which calls set_value! on the dest node, and returns nothing). e.g. from test/basics.jl "non-input bind":
 ```
 s = Signal(1; name="sig 1")
 m = map(x->2x, s; name="m")
diff --git a/src/operators.jl b/src/operators.jl
index 3bf9af6..f5e921e 100644
--- a/src/operators.jl
+++ b/src/operators.jl
@@ -265,7 +265,7 @@ value of the current signal. The `typ` keyword argument specifies
 the type of the flattened signal. It is `Any` by default.
 """
 function flatten(input::Signal; typ=Any, name=auto_name!("flatten", input))
-    n = Signal(typ, input.value.value, (input, input.value); name=name)
+    n = Signal(typ, input.value.value, (input,); name=name)
     connect_flatten(n, input)
     n
 end
@@ -273,32 +273,35 @@ end
 
 """
 `connect_flatten(output, input)`
-`output` is the flatten node, `input` is the Signal{Signal} ("sigsig") node
-Descendents of this flatten node need to know to update on changes to
-the input sigsig (allroots(input)), or changes to the value of the
-current sig (roots == allroots(current_node))
+
+`output` is the flatten node, `input` is the Signal{Signal} ("sigsig") node. The
+flatten needs to update on changes to the input sigsig, or changes to the value
+of the current sig (`current_node`). The former is achieved through a foreach `wire_flatten`
+attached to the input sigsig. The latter is achieved through binding the flatten
+to `current_node`.
 """
 function connect_flatten(output, input)
     # input is a Signal{Signal} (aka sigsig), current_node is the signal/node
-    # that is the input's current value. wire_flatten sets the flatten's
-    # parents, to (input, input.value), when the sigsig gets a new signal as its
-    # value. This ensures that both set_flatten_val and wire_flatten will be run
-    # (and flatten output node's value will update) when either the current_node
-    # updates, or when the input sigsig updates.
+    # that is the input's current value. wire_flatten will run when the sigsig gets a new signal as its
+    # value. This ensures that set_flatten_val will be run (and flatten output
+    # node's value will update) when either the current_node updates, or when
+    # the input sigsig updates.
     current_node = input.value
     wire_flatten() = begin
         # If the sigsig's value has changed update output's parents so it will
         # only update when the new current_node updates, and no longer
         # update when the previous signal updates.
         if current_node != input.value
+            unbind!(output, current_node, false)
             current_node = input.value
-            output.parents = (input, current_node)
+            bind!(output, current_node, false)
+            # the bind will have run downstream actions - avoid doubling up
+            deactivate!(output)
         end
     end
 
-    set_flatten_val() = set_value!(output, current_node.value)
     add_action!(wire_flatten, output)
-    add_action!(set_flatten_val, output) # this must come after wire_flatten
+    bind!(output, current_node, false)
 end
 
 const _bindings = Dict() # XXX GC Issue? can't use WeakKeyDict with Pairs...
@@ -345,7 +348,10 @@ function bind!(dest::Signal, src::Signal, twoway=true)
             # run_push then resume processing the original push by reactivating
             # the previously active nodes.
             active_nodes = pause_push()
-            run_push(dest, src.value, onerror_rethrow, false) # false for dont_remove_dead - messes with active_nodes
+            # `true` below is for dont_remove_dead nodes - messes with active_nodes
+            # TODO - check that - not sure it actually does, this may be a relic
+            # of an earlier implementation which used the node's id's
+            run_push(dest, src.value, onerror_rethrow, true)
             foreach(activate!, active_nodes)
         end
         nothing
diff --git a/test/flatten.jl b/test/flatten.jl
index 7d28028..1512e39 100644
--- a/test/flatten.jl
+++ b/test/flatten.jl
@@ -88,4 +88,35 @@ facts("Flatten") do
         @fact queue_size() --> 0
     end
 
+    context("Sigsig's value created after SigSig") do
+        # This is why we need bind! in flatten implementation rather than just
+        # setting the flatten's parents to (input, current_node) every time
+        # input updates. That won't work if the current_node was created after
+        # the flatten (e.g. in the below after pushing a new value to `a`),
+        # because updates to the current_node happen further down the `nodes`
+        # list than the flatten, so the flatten doesn't get updated.
+        a = Signal(number())
+        local b
+        c = foreach(a) do av
+                b = Signal(av)
+                foreach(identity, b)
+            end
+        d = flatten(c)
+        b_orig = b
+
+        @fact d.value --> a.value
+
+        push!(b, number())
+        step()
+        @fact d.value --> b.value
+
+        push!(a, number())
+        step()
+        @fact d.value --> a.value
+        @fact b_orig --> not(b)
+
+        push!(b, number())
+        step()
+        @fact d.value + 1 --> b.value + 1
+    end
 end