Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two-way bindings gives 'Cannot save value from target back to source' in debug output #10

Closed
isaacabraham opened this issue Jun 25, 2017 · 19 comments · Fixed by #79
Closed

Comments

@isaacabraham
Copy link

On a simple two-way binding from a textbox to a string on a model, I see the following output when running: -

System.Windows.Data Error: 8 : Cannot save value from target back to source. BindingExpression:Path=Name; DataItem='ViewModelBase`2' (HashCode=64479624); target element is 'TextBox' (Name=''); target property is 'Text' (type 'String') InvalidOperationException:'System.InvalidOperationException: Property path is not valid. 'System.Dynamic.DynamicObject+MetaDynamic' does not have a public property named 'Items'.
   at CallSite.Target(Closure , CallSite , Object , Object )
   at MS.Internal.DynamicPropertyAccessorImpl.SetValue(Object component, Object value)
   at MS.Internal.Data.PropertyPathWorker.SetValue(Object item, Object value)
   at MS.Internal.Data.ClrBindingWorker.UpdateValue(Object value)
   at System.Windows.Data.BindingExpression.UpdateSource(Object value)'

This doesn't crash the app, but repeated errors like these have an effort on WPF perf when they mount up.

@2sComplement
Copy link
Collaborator

Are you using Binding.twoWay in your viewBindings?

@isaacabraham
Copy link
Author

Yes, this is a two-way binding.

@2sComplement
Copy link
Collaborator

Do you have Mode=TwoWay in your XAML binding? If so and you're still seeing this, I'd need to see a code snippet to reproduce.

@isaacabraham
Copy link
Author

Here's the repo: https://github.com/CompositionalIT/ElmishWpfSample

(Note: Mode=TwoWay is the default on XAML bindings in WPF, so it's not set explicitly.)

@2sComplement
Copy link
Collaborator

I see it now and know why it's happening. It shouldn't affect any functionality but you're right about effects on performance. I will look into a way to avoid this.

@BentTranberg
Copy link

I see this in the Elmish.Samples.Counter demo, or to be exact, in my own demo where the Elmish.Samples.Counter demo source was copied, to get a start. It happens every time the slider is moved.

Just mentioning it in case there's need for a repro.

Error message:

System.Windows.Data Error: 8 : Cannot save value from target back to source. BindingExpression:Path=StepSize; DataItem='ViewModelBase`2' (HashCode=32361769); target element is 'Slider' (Name=''); target property is 'Value' (type 'Double') InvalidOperationException:'System.InvalidOperationException: Property path is not valid. 'System.Dynamic.DynamicObject+MetaDynamic' does not have a public property named 'Items'.
   at CallSite.Target(Closure , CallSite , Object , Object )
   at MS.Internal.DynamicPropertyAccessorImpl.SetValue(Object component, Object value)
   at MS.Internal.Data.PropertyPathWorker.SetValue(Object item, Object value)
   at MS.Internal.Data.ClrBindingWorker.UpdateValue(Object value)
   at System.Windows.Data.BindingExpression.UpdateSource(Object value)'

@BentTranberg
Copy link

@isaacabraham, I have one case now where Mode=TwoWay has to be set explicitly, or else it won't work. But I have also seen that in most cases it appears to be the default.

@cmeeren

This comment has been minimized.

@cmeeren

This comment has been minimized.

@cmeeren
Copy link
Member

cmeeren commented Aug 28, 2018

After having looked into this a bit, it seems like the view performs an immediate get after the value has been set. This get will in most (all?) cases return the existing value, not the new value, because the loop hasn't had a chance to complete yet.

This leaves me with three questions:

  1. What is the actual performance impact of this error?
  2. Is the performance impact relevant outside of Debug mode?
  3. Is there anything to be done about this? I haven't come up with a good solution on the ViewModel side, and I'm not that experienced with WPF so I don't know if it can be fixed from that end (e.g. if there is some setting that disables the immediate get or simply turns of the error).

@cmeeren
Copy link
Member

cmeeren commented Aug 28, 2018

Oh, another question, perhaps the most important - can @2sComplement or anyone else answer me this: Why is the get prevented in the first place? I have tried returning true, and it doesn't seem to have an impact on the functionality, nor (in my example) visible performance when not debugging. (Printing to trace/console is costly, but in this context only relevant when debugging.)

@2sComplement
Copy link
Collaborator

If I recall correctly, allowing it resulted in some undesirable view behaviour. For example, typing in an input box would be annoying, assuming you wanted characters to appear in the correct order. With each key press the view would update the text box with the old value before updating the model.

@cmeeren
Copy link
Member

cmeeren commented Aug 28, 2018

Thanks for the quick reply! Yes, I observe the behaviour you're describing; I initially only tested with a slider. With a TextBox however, you essentially type backwards.

That leaves my first three questions.

@2sComplement
Copy link
Collaborator

Apologies for my absence in general lately, been very tied up on other projects. I couldn't prove any real impacts of this error. Nor do I know a way around it without somehow overriding the behaviour of WPF through perhaps some custom binding type, which is probably the route I would have taken to investigate a fix for this. Not convinced it's worth it though.

@JDiLenarda
Copy link
Contributor

JDiLenarda commented Aug 28, 2018

I noticed this afternoon that on aBinding.oneWay, the getter is called 3 times per update (on a silly example involving a button that increases an integer and a textbox displaying it). I was wondering if that was due to your implementation or if it was already the case before, but I didn’t check. Is it the case you’re talking about ?

Naive suggestion for the get called just after the set : would a timed out lock or mutex on TryGetMember and TrySetmember be helpful ?

I’m far from my office PC right now, I can’t watch exactly what the problem is.

@cmeeren
Copy link
Member

cmeeren commented Aug 28, 2018

@JDiLenarda it's not the same if it's a one-way binding, no.

Please take a look at the SingleCounter example in the rewrite branch, TryGetMember is only called once there. If you find out why it's called three times in your example, let us know, and let us know if it's a bug or if we should make something clear in the documentation. If you can't figure it out, I can also take a repro solution, but I'm busy this week with other projects too, so I can't promise I can look at it right away.

As for locking: I'm very wary of doing that due to general multithreaded complexity and other performance concerns following from that solution, and also because I'm not sure the reported error actually matters at all outside development/debugging scenarios. Thanks for the tip, though. If you have a concrete implementation and performance numbers to back it up, feel free to make a PR. :)

@JDiLenarda
Copy link
Contributor

JDiLenarda commented Aug 28, 2018

Now I think about it, it’s probably because the Textbox is on Mode=TwoWay by default. I’ll check tomorrow.

Edit: never mind, I was on a false lead. It is normal that the getter of Binding.oneWay is called 3 times (it is also the case on SingleCounter) : twice by UpdateModel (against the current and the new model for comparison) and once by TryGetMember, which is called only once.

@cmeeren
Copy link
Member

cmeeren commented Sep 4, 2018

Thanks for the clarification, @JDiLenarda (didn't see your edit before now).

To everyone and no-one in particular, allow me to quote my three unanswered questions lest they get lost in the thread. Perhaps @ReedCopsey knows the answer to some of these questions?

  1. What is the actual performance impact of the OP error?
  2. Is the performance impact relevant outside of Debug mode?
  3. Is there anything to be done about this? I haven't come up with a good solution on the ViewModel side, and I'm not that experienced with WPF so I don't know if it can be fixed from that end (e.g. if there is some setting that disables the immediate get or simply turns of the error message).

@cmeeren cmeeren changed the title DataBinding error on simple binding Two-way bindings give error 'Cannot save value from target back to source' Mar 25, 2019
@cmeeren cmeeren changed the title Two-way bindings give error 'Cannot save value from target back to source' Two-way bindings gives 'Cannot save value from target back to source' in debug output Mar 25, 2019
@cmeeren
Copy link
Member

cmeeren commented Apr 15, 2019

This should be fixed in 2.0.0-beta-10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants