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

Custom converter specified on binding is called only once at startup and not with the every OnPropertyChanged event #16137

Closed
viktorspacil opened this issue Jun 26, 2024 · 7 comments · Fixed by #16150

Comments

@viktorspacil
Copy link

viktorspacil commented Jun 26, 2024

Describe the bug

Custom converter specified on binding is called only once at startup and not with the every OnPropertyChanged event as is expected. This is a new behavior related to 11.1.0-rc1, everything worked just fine with 11.0.10/11.0.11. Minimal sample to simulate this behavior is attached.
TestApp.zip

To Reproduce

  1. Start the app
  2. TextBlock content on the second line should change with the every click on button "Click me" (toggling between suffixes -cs/cz and -en/en) but this is not happening

Expected behavior

Text on the second line should change with the every click on button "Click me" because OnPropertyChanged event is fired for the binded property.

Avalonia version

11.1.0-rc1

OS

No response

Additional context

No response

@stevemonaco
Copy link
Contributor

stevemonaco commented Jun 26, 2024

You should consider writing an IMultiValueConverter and move the _toggle state into the ViewModel. IMO, converters are best left as pure functions without local state.

I can confirm the regression from 11.0.11 with the repro though (rc1 and nightly), so it should be fixed.

Also, I see a use of BindingNotification which might not work until RC2 or stable.

@grokys
Copy link
Member

grokys commented Jun 26, 2024

As @stevemonaco said, this is a bit of an edge case as you're raising PropertyChanged on a property which hasn't changed and then relying on a stateful IValueConverter to mutate the state differently each time. The issue is that we now check for a changed bound value earlier than before and so the converter isn't being called.

I cross-checked with WPF and it also does call the converter even if the value hasn't changed so I'm in two minds about whether we should support this use-case. On one hand we try to match WPF's behavior where it makes sense, but on the other hand it feels like this is an anti-pattern in that it's generally bad practise to rely on side-effects in bindings. It also feels like a misuse of PropertyChanged in that if that event is raised on a property that hasn't changed, a listener should be free to ignore the event.

@robloo
Copy link
Contributor

robloo commented Jun 26, 2024

I've run across a related issue to this that I never filed an issue for. The situation is this:

  1. Have a custom IsNullOrEmptyConverter bound to an ObservableCollection property. That converter is used to show "list empty, please add an item" type messages. So ListBlock.IsVisible="{Binding ListProperty, Converter={StaticResource IsNullOrEmptyConverter}}" in pseudocode.
  2. In the view model, manually force the converter to update visibility by calling OnPropertyChanged("ListProperty") which should force re-calculation of the converter and hide the message. In Avalonia it does not.

This is a pattern I've used since WPF as it saves one additional property on the view model that adds no new information. For now I've had to work-around this with a "IsListEmpty" property.

I really wish the binding systems were smart enough to raise property change notification on the ListProperty when it's a collection that has INotifyCollectionChanged but we aren't there yet. So I've been using this method.

Overall though, I think there always needs to be a way for apps to force re-evaluation of bindings. That's needed in edge cases we've discussed separately anyway.

@viktorspacil
Copy link
Author

As @stevemonaco said, this is a bit of an edge case as you're raising PropertyChanged on a property which hasn't changed and then relying on a stateful IValueConverter to mutate the state differently each time. The issue is that we now check for a changed bound value earlier than before and so the converter isn't being called.

I cross-checked with WPF and it also does call the converter even if the value hasn't changed so I'm in two minds about whether we should support this use-case. On one hand we try to match WPF's behavior where it makes sense, but on the other hand it feels like this is an anti-pattern in that it's generally bad practise to rely on side-effects in bindings. It also feels like a misuse of PropertyChanged in that if that event is raised on a property that hasn't changed, a listener should be free to ignore the event.

I provided the minimal sample to simulate this behavior. In real use I need to raise the property change for the property (flags) even if its value does not change because I need to translate the flags value into another language in runtime. I do not figure out another way how to do that.

@stevemonaco
Copy link
Contributor

stevemonaco commented Jun 26, 2024

@robloo

ObservableCollection<T>.Count can be binded to as it raises PropertyChanged for Count. Granted, that's a lot of notifications and converter calls since ObservableCollection<T> doesn't well-support range operations.

I think stateful converters are bad practice, but I also think the ViewModel or some component in-between should be responsible for restricting notifications, if necessary.

@viktorspacil

To clarify my earlier answer: Use a IMultiValueConverter and move the _toggle value to the ViewModel. You bind both TestEnum and Toggle to the converter. Then when you mutate either one from the VM, the converter will be reevaluated and the binding target updated.

@robloo
Copy link
Contributor

robloo commented Jun 26, 2024

@stevemonaco

ObservableCollection.Count can be binded to as it raises PropertyChanged for Count. Granted, that's a lot of notifications and converter calls since ObservableCollection doesn't well-support range operations.

lol, I've done it using the collection itself for so many years I never thought to check that. I never knew INotifyPropertyChanged fired for Count. Always learning, thanks!

I think stateful converters are bad practice, but I also think the ViewModel or some component in-between should be responsible for restricting notifications, if necessary.

I would say an IsNullOrEmptyConverter isn't stateful. It's a transform of the raw information and adds nothing to it or maintains any state itself. Transforming information is exactly what converters are for. Simply speaking we want to minimize duplicate information (properties) in the view model. I'm sure there is some information theory rationale here.


Anyway, I still think we shouldn't try to outsmart application developers. Just as in WPF, if the view model wants an update it should get an update. I'm sure I can find other cases where it's needed if I looked a while.

@grokys
Copy link
Member

grokys commented Jun 27, 2024

I've been thinking about this, and as much as I dislike some of the patterns this is used for, Avalonia 11.0 supports it and WPF supports it so Avalonia 11.1 should also support it.

grokys added a commit that referenced this issue Jun 27, 2024
grokys added a commit that referenced this issue Jun 27, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 28, 2024
#16150)

* Added failing test for #16137.

* Raise node change even if value hasn't changed.

Fixes #16137.
grokys added a commit that referenced this issue Jun 28, 2024
#16150)

* Added failing test for #16137.

* Raise node change even if value hasn't changed.

Fixes #16137.
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