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.

Sign up for GitHub

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

Jump to bottom

Support Nullable Bindable Properties in TypedBinding.cs #371

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Support Nullable Bindable Properties in TypedBinding.cs #371

wants to merge 3 commits into from

Conversation

JoeVoo
Copy link

@JoeVoo JoeVoo commented Feb 19, 2025 •
edited by TheCodeTraveler
Loading

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

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

  • null values must be allowed here

@JoeVoo
Update TypedBinding.cs
d01a987 
Remove the null value Exception
bijington
bijington reviewed Feb 19, 2025
View reviewed changes
src/CommunityToolkit.Maui.Markup/TypedBinding.cs
@@ -248,7 +248,7 @@ void ApplyCore(TSource? sourceObject, BindableObject target, BindableProperty pr
var needsSetter = (mode == BindingMode.TwoWay && fromTarget) || mode == BindingMode.OneWayToSource;
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
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?

TheCodeTraveler
TheCodeTraveler requested changes Feb 19, 2025
View reviewed changes
Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment •
edited
Loading

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
Author

JoeVoo commented Feb 20, 2025

@dotnet-policy-service agree

@JoeVoo
Copy link
Author

JoeVoo commented Feb 20, 2025 •
edited
Loading

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
Author

JoeVoo commented Feb 20, 2025 •
edited
Loading

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

@bijington
Copy link
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
@TheCodeTraveler
Merge branch 'main' into patch-1
1a0612b
@JoeVoo
Update TypedBinding.cs
d9ccbfa 
newerly i got an error message on the TryConvert line with ref value is null. I managed this with the added lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bijington bijington bijington left review comments

@TheCodeTraveler TheCodeTraveler TheCodeTraveler requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Set nullable BindableProperty to null throws Exception
3 participants
@JoeVoo @bijington @TheCodeTraveler