Avalonia icon indicating copy to clipboard operation
Avalonia copied to clipboard

UpdateSourceTrigger

Open pr8x opened this issue 2 years ago • 9 comments

What does the pull request do?

Implement WPF's UpdateSourceTrigger (https://docs.microsoft.com/en-us/dotnet/api/system.windows.data.binding.updatesourcetrigger?view=windowsdesktop-6.0)

TODO:

  • Documentation
  • ~~I need a proper interface for getting focus loss event inside Avalonia.Base. Any ideas? There could be some IUpdateSourceTriggerLostFocusProvider or something. Right now I am just checking for "IsFocused" property (by name) which is not a good solution for obvious reasons.~~

What is the current behavior?

It's not supported.

Checklist

  • [X] Added unit tests (if possible)?
  • [ ] Added XML documentation to any related classes?
  • [ ] Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation

Breaking changes

Most important ones: InstancedBinding.ctor() InstancedBinding.TwoWay() InstancedBinding.OneWayToSource()

These could be defaulted to UpdateSourceTrigger.Default though.

Fixed issues

Fixes https://github.com/AvaloniaUI/Avalonia/issues/3754

pr8x avatar Mar 13 '22 14:03 pr8x

You can test this PR using the following package version. 0.10.999-cibuild0019276-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

avaloniaui-team avatar Mar 13 '22 14:03 avaloniaui-team

You can test this PR using the following package version. 0.10.999-cibuild0019289-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

avaloniaui-team avatar Mar 14 '22 17:03 avaloniaui-team

Maybe you can add an example to BindingDemo? (side note: styles are broken in there, can probably switch to fluent theme).

MarchingCube avatar Mar 14 '22 23:03 MarchingCube

I need a proper interface for getting focus loss event inside Avalonia.Base. Any ideas?

To be honest, I think we need to wait until https://github.com/AvaloniaUI/Avalonia/pull/5831 is merged in order to do this properly, because this feature breaks the layering we currently have.

grokys avatar Apr 13 '22 13:04 grokys

Decided to remove UpdateSource and UpdateTarget for now. I feel like exposing them on the InstancedBinding is not really helpful in many cases and they have some overhead even when they're not used. I wonder if it's really worth having them.


Anyways, ran some benchmarks and it seems that this PR improves bindings a bit:

Master

Method Mean Error StdDev Gen 0 Gen 1 Allocated
TwoWayBinding_Via_Binding 3.984 μs 0.0778 μs 0.0895 μs 0.3395 0.0076 3 KB
UpdateTwoWayBinding_Via_Binding 118.075 μs 0.3807 μs 0.3375 μs 4.1504 - 34 KB

This PR

Method Mean Error StdDev Gen 0 Gen 1 Allocated
TwoWayBinding_Via_Binding 3.177 μs 0.0447 μs 0.0396 μs 0.3014 0.0076 2 KB
UpdateTwoWayBinding_Via_Binding 109.579 μs 0.9755 μs 0.9125 μs 4.0283 - 34 KB

pr8x avatar Jun 05 '22 13:06 pr8x

@pr8x wanted to take a look at this, but seems a bunch of binding unit tests are failing?

grokys avatar Aug 04 '22 08:08 grokys

You can test this PR using the following package version. 0.10.999-cibuild0022898-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

avaloniaui-team avatar Aug 05 '22 12:08 avaloniaui-team

@grokys UTs fixed.

pr8x avatar Aug 05 '22 12:08 pr8x

@pr8x Our product is being ported from WPF and needs UpdateSourceTrigger.Explicit, UpdateSource, and UpdateTarget.

We can work around the lack of these options these by creating proxy properties and assigning values or bindings to them at times of our choosing, but this is an ugly hack that can cause confusion among control consumers.

Have you seen the method AvaloniaObject.CoerceValue? This sort of does the same thing as UpdateTarget, but a) only for StyledProperty properties and b) by recalculating the effective property value, not a specific binding.

Between the AvaloniaObject._directBindings and ValueStore._values fields, it seem to me that direct access to bindings applied to each property should be possible, allowing something equivalent to this WPF call:

myControl.GetBindingExpression(Control.MyProperty).UpdateTarget();

Use case for UpdateSource

We have a map pin that the user can drag around the screen. We need other UI elements to immediately respond to its position changing, but we only want to update the position value in our viewmodel (via a two-way binding on the pin) when the user lets go of the mouse.

Use case for UpdateTarget

Our TimePicker template includes text boxes which the user can type into. If the control is in 12-hour mode and set to 11pm, then when the user types "23" into the hour field and commits the value, we need the box to change to display "11". But because the underlying TimeSpan value has not changed, no event is raised, the binding is not re-evaluated, and the box continues to display "23". We need to manually re-run the text box binding.

TomEdwardsEnscape avatar Sep 03 '22 12:09 TomEdwardsEnscape

Hi guys, do you have news about this merge ? I'm trying to find workaround like using BindingOperations.Apply but I'm not sure is the right way to force an update on a binding

Thanks in advance.

Tenjim avatar Sep 29 '22 10:09 Tenjim

Also would like to see an update on the status here. @pr8x Are you waiting for core maintainer feedback?

It would be great to get this in for 11.0.

robloo avatar Nov 04 '22 00:11 robloo

I'm kinda 👎 on merging this PR right now for a couple of reasons:

  1. We to do a major refactor of the binding system very soon anyway, and would like to include this feature anyway. The way it's implemented here feels rather bolted onto an architecture that wasn't really designed for it
  2. There's only 1 unit test. I'm 99% sure that one unit test isn't going to cover all of the edge-cases

In the absence of more unit tests, I'd be ok with merging if it'd had extensive testing. I think I asked you a while ago @pr8x if you'd tested this in your product and you hadn't yet. Have you had chance to test it now?

grokys avatar Nov 04 '22 10:11 grokys

@grokys I did test it in one scenario and it was working fine.

At this point this PR was stagnant waiting for review that we might as well close it and wait for the refactoring of the bindings. Current binding system seems messy anyway relying way too much on observables/LINQ selectors.

pr8x avatar Nov 07 '22 11:11 pr8x

Ok, lets close this and I'll get started on refactoring the binding system soon.

grokys avatar Nov 09 '22 08:11 grokys

Hello, Any update on the possibility to have this feature (and refactoring of the binding system) in new 11 previews? The workaround using a Behavior for lost focus seems not to work anymore.

mightypanda avatar Feb 19 '23 18:02 mightypanda