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

[Fix] WPF-BindingLeak: Bind to property of non INotifyPropertyChanged without using BindingMode.OneTime #2997

Merged
merged 6 commits into from Jun 28, 2017

Conversation

Projects
None yet
3 participants
@xxMUROxx
Contributor

xxMUROxx commented Jun 27, 2017

No description provided.

xxMUROxx and others added some commits Jun 23, 2017

Rename IgnoreThicknessSideType to ThicknessSideType and use it in Bor…
…derThicknessToStrokeThicknessConverter
Removed newly created BorderThicknessToStrokeThicknessConverter and m…
…oved the written code to existing ThicknessToDoubleConverter.

Removed ThicknessToDoubleConverter resource entries where the static resource is not referenced.

@punker76 punker76 added this to the 1.6.0 milestone Jun 28, 2017

@punker76 punker76 merged commit ceb5654 into MahApps:develop Jun 28, 2017

1 check passed

continuous-integration/teamcity Finished TeamCity Build MahApps.Metro PullRequest :: MahApps.Metro PullRequests : Tests passed: 64
Details
@punker76

This comment has been minimized.

Member

punker76 commented Jun 28, 2017

@xxMUROxx 👍

@thoemmi

This comment has been minimized.

Collaborator

thoemmi commented Jun 28, 2017

@xxMUROxx

I have one objection: ThicknessToDoubleConverter.ConvertBack was changed to throw an exception instead of returning DependencyProperty.UnsetValue. What's the reason for that change? The documentation for IValueConverter.ConvertBack says (emphasis mine):

The data binding engine does not catch exceptions that are thrown by a user-supplied converter. Any exception that is thrown by the ConvertBack method, or any uncaught exceptions that are thrown by methods that the ConvertBack method calls, are treated as run-time errors. Handle anticipated problems by returning DependencyProperty.UnsetValue.

A return value of DependencyProperty.UnsetValue indicates that the converter produced no value and that the binding uses the FallbackValue, if available, or the default value instead.

A return value of Binding.DoNothing indicates that the binding does not transfer the value or use the FallbackValue or default value.

/cc @punker76

@punker76

This comment has been minimized.

Member

punker76 commented Jun 28, 2017

@thoemmi Ah, yeah, your're right for this. I'll change it. thx for watching :-D

punker76 added a commit that referenced this pull request Jun 28, 2017

#2997
  WPF-BindingLeak: Bind to property of non INotifyPropertyChanged without using BindingMode.OneTime

  Use DependencyProperty.UnsetValue, cause:

The [documentation for `IValueConverter.ConvertBack`] https://msdn.microsoft.com/en-us/library/system.windows.data.ivalueconverter.convertback(v=vs.110).aspx) says (emphasis mine):

The data binding engine does not catch exceptions that are thrown by a user-supplied converter. **Any exception that is thrown by the ConvertBack method, or any uncaught exceptions that are thrown by methods that the ConvertBack method calls, are treated as run-time errors. Handle anticipated problems by returning `DependencyProperty.UnsetValue`**.

A return value of `DependencyProperty.UnsetValue` indicates that the converter produced no value and that the binding uses the FallbackValue, if available, or the default value instead.

A return value of `Binding.DoNothing` indicates that the binding does not transfer the value or use the `FallbackValue` or default value.
@xxMUROxx

This comment has been minimized.

Contributor

xxMUROxx commented Jun 29, 2017

@thoemmi thanks for the notification. It was not intended to change this piece of code.

@xxMUROxx xxMUROxx deleted the xxMUROxx:hotfix/WPF-BindingLeak branch Jun 29, 2017

@punker76 punker76 added the Bug label Feb 11, 2018

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