Skip to content

Support Nullable Bindable Properties in TypedBinding.cs#371

Closed
JoeVoo wants to merge 3 commits intoCommunityToolkit:mainfrom
JoeVoo:patch-1
Closed

Support Nullable Bindable Properties in TypedBinding.cs#371
JoeVoo wants to merge 3 commits intoCommunityToolkit:mainfrom
JoeVoo:patch-1

Conversation

@JoeVoo
Copy link
Copy Markdown

@JoeVoo JoeVoo commented Feb 19, 2025

Remove the null value Exception

  • Bug fix

Description of Change

Remove the exception fired on null value that is not needed
var value = GetTargetValue(target.GetValue(property), typeof(TProperty)) ?? throw new InvalidOperationException("Unable to find target value");

Linked Issues

PR Checklist

Additional information

  • null values must be allowed here

Remove the null value Exception
if (needsSetter && sourceObject is not null)
{
var value = GetTargetValue(target.GetValue(property), typeof(TProperty)) ?? throw new InvalidOperationException("Unable to find target value");
var value = GetTargetValue(target.GetValue(property), typeof(TProperty));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should check whether TProperty is nullable in this scenario. Thoughts @TheCodeTraveler?

Copy link
Copy Markdown
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JoeVoo! Could you please add unit tests that demonstrate the bug you are fixing and that verify the fix in this PR? It'd be great if you could also update the sample app to include a demonstration of how users were impacted; we use the sample app as a regression test against new PRs to ensure they don't break any existing behavior.

I've re-added the PR Checklist from our Pull Request Template to this PR Description that contains this checklist.

@JoeVoo
Copy link
Copy Markdown
Author

JoeVoo commented Feb 20, 2025

@dotnet-policy-service agree

@JoeVoo
Copy link
Copy Markdown
Author

JoeVoo commented Feb 20, 2025

Nullable reference types are enabled in the Unit Tests. I think this happens only in a 'nullable ref type disabled' project. But i am no test specialist. I try to avoid using enable.
My control that got the exception is a generic localized enum picker that's selectedValue can be bound and set null.

public partial class EnumPicker<T> : Picker where T : struct, Enum 
...
       public static readonly BindableProperty SelectedValueProperty =
           BindableProperty.Create(
           nameof(SelectedValue),
           typeof(T?),
           typeof(EnumPicker<T>),
           null,
           BindingMode.TwoWay,
           null,
           propertyChanged: SelectedValue_Changed);

typeof(T?) and EnumOfT is not nice, but the only way i found to set it null

@JoeVoo
Copy link
Copy Markdown
Author

JoeVoo commented Feb 20, 2025

@bijington , what is your scenario to get the exception?

@bijington
Copy link
Copy Markdown
Contributor

I am not experiencing this issue but I would be happy to try and contribute a unit test which would highlight the issue and fix if that helps?

@TheCodeTraveler TheCodeTraveler changed the title Update TypedBinding.cs Support Nullable Bindable Properties in TypedBinding.cs Feb 20, 2025
TheCodeTraveler and others added 2 commits February 21, 2025 09:50
newerly i got an error message on the TryConvert line with ref value is null. I managed this with the added lines.
@TheCodeTraveler
Copy link
Copy Markdown
Collaborator

Closing in favor of #381; same solution with unit tests included

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Set nullable BindableProperty to null throws Exception

3 participants