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

[BUG] Typed bindings is not working as expected. #272

Closed
2 tasks done
egvijayanand opened this issue Nov 28, 2023 · 15 comments
Closed
2 tasks done

[BUG] Typed bindings is not working as expected. #272

egvijayanand opened this issue Nov 28, 2023 · 15 comments
Labels
bug Something isn't working unverified Bug has not been verified by maintainers

Comments

@egvijayanand
Copy link

egvijayanand commented Nov 28, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Did you read the "Reporting a bug" section on Contributing file?

Current Behavior

Typed binding breaks the working behavior of the controls, whereas classic binding works in the way it is designed.

Expected Behavior

Like how it works in classic binding, only the approach changes with improved type safety.

Steps To Reproduce

  1. Clone the repository
  2. Run the sample app
  3. It hosts two controls on the Main page of the app, both are pickers, one with typed binding and the other with classic binding
  4. Both are bound to the same Index property, which means a change in one picker should reflect in the other
  5. Now change the value in the type picker and no response in the classic picker (the issue)
  6. Then change the value in the classic picker, there is a change in the type picker (actual behavior)

Link to public reproduction project repository

https://github.com/egvijayanand/markup-issue-272

Environment

- .NET MAUI C# Markup CommunityToolkit: v4.0.0
- OS: Windows 11 v10.0.22631.2715
- .NET MAUI: v8.0.3

Anything else?

No response

@egvijayanand egvijayanand added bug Something isn't working unverified Bug has not been verified by maintainers labels Nov 28, 2023
@egvijayanand

This comment was marked as outdated.

@egvijayanand
Copy link
Author

egvijayanand commented Nov 29, 2023

Here's the GitHub repository with the sample app

Kindly let me know if I missed anything, as a last measure even tried using Two-way binding, but no change in the output.

It's not only to Picker as have tried with another sample that hosts a variety of controls, nothing is working after updating to Typed bindings.

@TheCodeTraveler
Copy link
Collaborator

TheCodeTraveler commented Nov 29, 2023

Hey Vijay!

It looks like you've only implemented a One-Way Typed Binding. You've bound the getter of Picker.ItemsSourceProperty to read MainViewModel.Index, but you haven't told the binding how to set the value when it changes.

You just need to add this as the setter for your Typed Binding:

static (MainViewModel vm, int index) => vm.Index = index

One-Way Binding (Read-Only)

 new Picker()
  .Bind(Picker.ItemsSourceProperty, 
             static (MainViewModel vm) => vm.Numbers)
  .Bind(Picker.SelectedIndexProperty, 
             static (MainViewModel vm) => vm.Index, 
             mode: BindingMode.TwoWay),

Two-Way Binding (Read-Write)

 new Picker()
  .Bind(Picker.ItemsSourceProperty, 
             static (MainViewModel vm) => vm.Numbers)
  .Bind(Picker.SelectedIndexProperty, 
             static (MainViewModel vm) => vm.Index, 
             static (MainViewModel vm, int index) => vm.Index = index,
             mode: BindingMode.TwoWay),

Example App Running Fixed Code

Simulator Screen Recording - iPhone 15 Pro - 2023-11-28 at 17 23 49

@TheCodeTraveler
Copy link
Collaborator

TheCodeTraveler commented Nov 29, 2023

I'd love your help updating our docs to make this more clear!

Or point me to whatever resource where you were learning Typed Bindings and we'll see if we can update that 💯

@TheCodeTraveler TheCodeTraveler closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2023
@egvijayanand
Copy link
Author

I understand @brminnick, may be an understanding gap. Thanks.

But definitely feel it's an overhead, as most of the time we work with two-way data-binding and it writes back to the same property from where the read happens.

Is there a way to automate things so that we need to write the explicit setter only if there's logic involved?

@TheCodeTraveler
Copy link
Collaborator

But definitely feel it's an overhead, as most of the time we work with two-way data-binding and it writes back to the same property from where the read happens.

Yup - this is the biggest difference between the previous way of declaring bindings and Typed Bindings.

Is there a way to automate things so that we need to write the explicit setter only if there's logic involved?

It may be possible. It'd require a lot of forethought and unit testing to make sure we don't break any default behavior, though; I'm not sure if the TypedBinding class is meant to handle that scenario.

If you want to try it out feel free to submit a New Feature Proposal.

@egvijayanand
Copy link
Author

It may be possible. It'd require a lot of forethought and unit testing to make sure we don't break any default behavior, though; I'm not sure if the TypedBinding class is meant to handle that scenario.

Not fully sure, but seems possible. Let me try with a sample and get back on this.

@TheCodeTraveler
Copy link
Collaborator

Awesome! Yea, we'll want to test the new implementation against one-way bindings, two-way bindings, one-way-to-source bindings and one-time bindings.

There's also a DefaultBindingMode for every BindableObject, so we'll want to test it against BindableObjects that each have a different DefaultBindingMode.

https://learn.microsoft.com/en-us/dotnet/maui/fundamentals/data-binding/binding-mode?view=net-maui-8.0

Every .NET Multi-platform App UI (.NET MAUI) bindable property has a default binding mode that is set when the bindable property is created, and which is available from the DefaultBindingMode property of the BindableProperty object.

@egvijayanand
Copy link
Author

There's also a DefaultBindingMode for every BindableObject, so we'll want to test it against BindableObjects that each have a different DefaultBindingMode.

I'm a big fan of DefaultBinding (both property and mode). Last time when TypedBinding was in prototype, I asked for the possibility of having this in the context of typed too. Like Text property being the default for Label/Entry and so on and those are two-way too.

Let me check the implementation to validate the possibility.

@egvijayanand
Copy link
Author

egvijayanand commented Nov 29, 2023

For picker, SelectedIndex is default and two-way, that's why I didn't check it in deep. Thought mapping itself is good enough to do a two-way binding and that's the reason tried to set the binding mode to two-way explicitly.

@egvijayanand
Copy link
Author

egvijayanand commented Nov 29, 2023

Looks like, have cracked it.

I've already tried this. But just missed a single change while parsing the Func expression that made me to hold that for so long.

Instead of adding a generic parameter, have assumed the return value of the Func expression as an object. Now have included that one.

This will work for default properties too :-)

Will add the extension methods to the same sample, publish a newer version, and will give an update.

@egvijayanand
Copy link
Author

Have pushed the updated version. Have suffixed the Bind methods with v2 as the parameter types clash with those from the package.

The method that parses the Property name from the LINQ expression definitely requires refactoring as it was written years ago.

@brminnick Let me know your thoughts on this.

@TheCodeTraveler
Copy link
Collaborator

No way! That was quick!!

Where can I find the code?

@egvijayanand
Copy link
Author

egvijayanand commented Nov 29, 2023 via email

@egvijayanand
Copy link
Author

Further discussion here - #343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working unverified Bug has not been verified by maintainers
Projects
None yet
Development

No branches or pull requests

2 participants