Enable `Action` executing state feedback to the state property. #400

andersio · 2017-05-22T07:22:33Z

Related: #309.

There are legitimate uses of isExecuting feedback to the Action state property. For example, we may link the availability of multiple actions together, so that all disables when one executes.

This PR refactors the internal of Action so that such use cases are permitted. Legitimate infinite feedback loops would still result in deadlocks.

Example:

let isIdle = MutableProperty(true)

candyCrusher = Action(enabledIf: isIdle) { ... }
fruitCrusher = Action(enabledIf: isIdle) { ... }
carrotCrusher = Action(enabledIf: isIdle) { ... }

isIdle <~ candyCrusher.isExecuting.negate()
isIdle <~ fruitCrusher.isExecuting.negate()
isIdle <~ carrotCrusher.isExecuting.negate()

Alternative/Follow-up

Could we have some kind of "action group"s that determine the availability at a coarser granularity?

mdiep · 2017-05-24T13:56:02Z

Sources/Action.swift

+
+						state.availability = .enabledExecuting
+						isExecuting.value = true
+						isEnabled.value = false


Why is isEnabled false if availability is enabledExecuting?

Action s are disabled when executing.

mdiep · 2017-05-24T13:56:31Z

Sources/Action.swift

+		let isExecuting = MutableProperty(false)
+		let isEnabled = MutableProperty(true)
+		self.isExecuting = Property(capturing: isExecuting)
+		self.isEnabled = Property(capturing: isEnabled)


Can these be derived from actionState instead of duplicating that information in separate properties?

The signal of ActionState is deliberately ignored, since Signal doesn't support recursion with value events.

actionState deadlocks when one does isEnabled <~ isExecuting and both are derived from it, i.e. the current implementation.

In 4acaf81, the read-only isEnabled is once again derived since we are permitting only state <~ isExecuting here, where state is the state of the Action that flips its availability (isEnabled).

mdiep · 2017-05-24T13:57:25Z

Sources/Action.swift

+					state.availability = .disabledExecuting
+
+				default:
+					break


This default shouldn't be needed?

There are eight combinations in total, and only four need to be acted on.

mdiep · 2017-05-24T14:00:14Z

Sources/Action.swift

-		executeClosure = { state, input in execute(state as! State.Value, input) }
+		// `isExecuting` and `isEnabled` should use their own locks, so that legitimate
+		// feedbacks would not deadlock.
+		let initial = (availability: Availability.enabledIdle, value: state.value)


Can you make this a struct instead of a typealias?

Done in 4acaf81.

mdiep · 2017-05-24T14:00:51Z

Sources/Action.swift

+		case enabledIdle
+		case enabledExecuting
+		case disabledIdle
+		case disabledExecuting


This is really modeling two independent boolean states. Can we separate these and add them to ActionState individually? I think that will simplify the code a bit as well.

The problem is that isEnabled and isExecuting are not independent. They together form a finite state machine, and IMO named cases are easier to understand than a switch with tuples of booleans.

Changed in 4acaf81 anyway.

mdiep · 2017-05-24T14:04:05Z

Sources/Action.swift

 	/// In other words, this sends every value from every unit of work that the `Action`
 	/// executes.
-	public let values: Signal<Output, NoError>
+	public private(set) lazy var values: Signal<Output, NoError> = { [unowned self] in


Can we make this lazy change in a separate PR? I'm not sold on it, and it's not essential to this PR.

Reverted in 4acaf81.

mdiep · 2017-05-31T00:44:48Z

Sources/Action.swift

+		self.isEnabled = actionState.map { $0.isEnabled }
+
+		let isExecuting = MutableProperty(false)
+		self.isExecuting = Property(capturing: isExecuting)


Can this be self.isExecuting = actionState.map { $0.isExecuting }?

The point of giving it its own backing is that isExecuting calls out to the observers outside the signal of actionState, so that when actionState is modified reactively & synchronously it doesn't deadlock.

However, it does seem violating exclusivity of access so it might need a change anyway. We might need to add guards in MutableProperty to prevent nested modifications too, or rely on the dynamic endorcement in Swift 4.

mdiep · 2017-05-31T00:45:37Z

Sources/Action.swift

+		self.isExecuting = Property(capturing: isExecuting)
+
+		// Associate the state property with the created `Action`.
+		lifetime.observeEnded { _ = state }


This comment is describing what this line does. Can you update it to describe why it needs to be associated?

mdiep · 2017-05-31T00:48:31Z

Sources/Action.swift

-		state = MutableProperty(initial)
+		self.execute = { action, input in
+			return SignalProducer { observer, lifetime in
+				func tryStart() -> State.Value? {


This is only called once. Maybe take the code out of the function?

let state = actionState.modify { state in ... } guard let state = state else { ...

It should be done after the modifier of `ActionState` returns and the `inout` reference is semantically written back, in order not to violate the exclusivity of access.

mdiep · 2017-05-31T11:54:37Z

Sources/Action.swift


-		self.isEnabled = state.map { $0.isEnabled }.skipRepeats()
-		self.isExecuting = state.map { $0.isExecuting }.skipRepeats()
+				let latestState: State.Value? = actionState.modify(didSet: didSet) { state in


This didSet seem pointless. The function is only called here, right after notifiesExecutionState is set to false—so it will always do nothing. It seem like both didSet() and notifiesExecutionState can be removed?

notifiesExecutionState is set to true when the current execution attempt succeeds and starts. didSet is invoked after the trailing closure modifying actionState.

🤦🏼‍♂️

mdiep · 2017-05-31T12:41:16Z

Sources/Action.swift


-		self.isEnabled = state.map { $0.isEnabled }.skipRepeats()
-		self.isExecuting = state.map { $0.isExecuting }.skipRepeats()
+				let latestState: State.Value? = actionState.modify(didSet: didSet) { state in


🤦🏼‍♂️

andersio added the bug label May 22, 2017

andersio force-pushed the action-recursive-enabled branch 3 times, most recently from d110566 to abc9160 Compare May 22, 2017 07:55

andersio modified the milestone: 2.0 May 22, 2017

mdiep requested changes May 24, 2017

View reviewed changes

tikitu mentioned this pull request May 25, 2017

Action composition (WIP) #407

Closed

andersio force-pushed the action-recursive-enabled branch 5 times, most recently from abbf01d to 4b6792e Compare May 26, 2017 20:00

Enable Action executing state feedback to the state property.

4acaf81

andersio force-pushed the action-recursive-enabled branch from 4b6792e to 4acaf81 Compare May 26, 2017 20:02

andersio requested a review from mdiep May 26, 2017 20:05

mdiep requested changes May 31, 2017

View reviewed changes

Corrected the isExecuting emission.

bf67909

It should be done after the modifier of `ActionState` returns and the `inout` reference is semantically written back, in order not to violate the exclusivity of access.

mdiep requested changes May 31, 2017

View reviewed changes

mdiep approved these changes May 31, 2017

View reviewed changes

mdiep merged commit 45c2bd4 into master May 31, 2017

mdiep deleted the action-recursive-enabled branch May 31, 2017 13:00

andersio mentioned this pull request Jul 10, 2017

Expand feedback support to Action.isEnabled. #481

Merged

1 task

Enable Action executing state feedback to the state property. #400

Enable Action executing state feedback to the state property. #400

Uh oh!

Conversation

andersio commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Alternative/Follow-up

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersio May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersio May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersio May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersio May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enable `Action` executing state feedback to the state property. #400

Enable `Action` executing state feedback to the state property. #400

andersio commented May 22, 2017 •

edited

Loading

andersio May 24, 2017 •

edited

Loading

andersio May 26, 2017 •

edited

Loading

andersio May 24, 2017 •

edited

Loading

andersio May 31, 2017 •

edited

Loading