Wrap view store binding to avoid reading stale state

[iampatbrown]

Attempt to fix #1800

The current method creates a binding with a location type of ObservableObjectLocation. By wrapping the binding the location type becomes FunctionalLocation. Bindings that use ObservableObjectLocation seem to have their getter called while evaluating if a views inputs have changed. This doesn't seem to be the case with FunctionalLocation. Which kind of makes sense.

I've confirmed this works for #1783 and #1798 as well as checked animations work as expected.

@stephencelis @mbrandonw @tgrapperon maybe you can stress test this a bit to see if there are any unwanted effects. I imagine there is going to be some sort of optimisation that can be applied to bindings that use ObservableObjectLocation, but don't really think that's relevant here.

[tgrapperon]

"Yo Dawg, I heard you like bindings, so we put bindings into your bindings so you can bind while you bind!"

Great find! I think that most of the problems are caused by the fact that the ObservedObject we were using is not installed on the graph. So while it was fixing some animations issues, it likely wasn't participating as it should to the invalidation process. I would guess that function backed ones are likely eagerly evaluated, whereas ObservedObject based ones are maybe coalesced up to the next render pass. I haven't checked yet if a simple functional binding that would call objectWillChange() in its setter wouldn't have helped with animations (maybe it was like that at some point).

I'll try to give them a spin soon to check if I'm spotting issues.

[barabashd]

Nice catch @iampatbrown! I'm curious about the 'transaction($1)' purpose here. Could you explain it, please? Because removing this part still fixes both issues.

[iampatbrown]

Nice catch @iampatbrown! I'm curious about the 'transaction($1)' purpose here. Could you explain it, please? Because removing this part still fixes both issues.

@barabashd .transaction($1) allows any animations or transactions explicitly set on the binding to work correctly. eg:

Toggle("Toggle", isOn: $isOn.animation(.easeIn))

Using withTransaction(transaction) { base.wrappedValue = newValue } as an alternative has slightly different behaviour. It causes explicit animations/transactions to take precedence over other calls to withAnimation/withTransaction.

[iampatbrown]

I think that most of the problems are caused by the fact that the ObservedObject we were using is not installed on the graph. So while it was fixing some animations issues, it likely wasn't participating as it should to the invalidation process.

@tgrapperon It's been a while since stumbling across the ObservedObject approach, can't remember having much success without it though. My guess was that something happened under the hood that tracked state changes and forwarded explicit animations/transactions correctly.

I haven't checked yet if a simple functional binding that would call objectWillChange() in its setter wouldn't have helped with animations (maybe it was like that at some point).

Pretty sure the main thing was forwarding explicit binding animations/transactions. Accessing the current Transaction to merge/override any explicit binding transaction could also work, though it's not really supported.

[stephencelis]

Seems to work great! We'll get a couple integration tests in place in another PR.