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

[Enhancement] Report Binding failures #2639

Closed
pingzing opened this Issue May 9, 2018 · 8 comments

Comments

4 participants
@pingzing
Contributor

pingzing commented May 9, 2018

Rationale

In Xamarin.Forms, when a {Binding} expression fails, it fails silently, and at runtime. The only indication that the binding has failed is that nothing appears in the UI. Debugging this can be difficult--is it a BindingContext issue? A typo in my property name? Did I accidentally define my property as a member variable? Etc.

Suggestion

Log binding failures similarly to the way that WPF and UWP do:

System.Windows.Data Error: 40 : BindingExpression path error: 'NonExistingProperty' property not found on 'object' ''Grid' (Name='pnlMain')'. BindingExpression:Path=NonExistingProperty; DataItem='Grid' (Name='pnlMain'); target element is 'TextBlock' (Name=''); target property is 'Text' (type 'String')

and print them out to the Debug log.

~EDIT~: Looking at BindingExpression.cs it looks like the logging code is there, but it never gets printed out to my Debug or Output windows. Could it be a configuration issue on my end?

Running Windows 10, Visual Studio 2017 15.6.7, reproducing this in XF 2.5.1.444934.

@pauldipietro pauldipietro added this to New in Triage May 9, 2018

@StephaneDelcroix StephaneDelcroix moved this from New to Ready For Work in Triage May 9, 2018

@Paul-Brenner-Tangoe

This comment has been minimized.

Paul-Brenner-Tangoe commented May 9, 2018

+1 This would be fantastic.

@samhouts samhouts removed this from Ready For Work in Triage May 9, 2018

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented May 30, 2018

I was triggered by this issue, looks like a good addition!

As @pingzing already mentioned, it is already written to the Trace logs.
To get them out right now, you could add this line somewhere in your app initialization code:

Xamarin.Forms.Internals.Log.Listeners.Add(new DelegateLogListener((arg1, arg2) => Debug.WriteLine(arg2)));

Of course, you could do something other than a Debug.WriteLine(). Now, these messages (and probably others) will show up in your own app output.

screenshot 2018-05-30 10 42 56

Any thoughts on how we can make this more accessible?

@pingzing

This comment has been minimized.

Contributor

pingzing commented May 30, 2018

A few things come to mind immediately:

jfversluis added a commit to jfversluis/Xamarin.Forms that referenced this issue May 30, 2018

Implemented static DebugSettings class
As per xamarin#2639 I tried to mimic the workings of UWP/WPF, starting with
the failed databinding functionality. Whenever a property is not found
this can be caught through an event.
@jfversluis

This comment has been minimized.

Contributor

jfversluis commented May 30, 2018

@pingzing could you check the referenced commit on my fork? Something like this?

It seems like the boolean to enable it is a bit redundant. Whenever the event is wired up it should just be executed? Maybe I mixed up your two solutions :)

pingzing added a commit to pingzing/Xamarin.Forms that referenced this issue May 30, 2018

@pingzing

This comment has been minimized.

Contributor

pingzing commented May 30, 2018

That's very close to what I was thinking! I made a few minor changes in my fork-of-your-fork: pingzing@bf4ba93

My changes were driven by two things: My interpretation that toggling the IsBindingTracingEnabled flag should always enable printing to the Output log, and that event handler was just there for advanced scenarios.

On that note though, I'm not sure if my simple Debug.WriteLine("") would be reliable/flexible enough?

Thoughts? Maybe too complex?

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented May 31, 2018

That makes sense. That way you don't need to do anything special to make this work besides toggling the setting. If you do have more wishes, you can hook into the event handler.

But now, how are we going to pull this together? :P

@pingzing

This comment has been minimized.

Contributor

pingzing commented May 31, 2018

Uh. Github is not well suited to this scenario =D

Maybe add me as a contributor to your fork, and we can use it as a basis for an eventual PR?

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented May 31, 2018

Done!

Note I pushed another commit that also outputs some more binding errors

@pingzing pingzing referenced this issue Jun 3, 2018

Merged

[Core] Report binding failures #2911

2 of 4 tasks complete

@samhouts samhouts added this to In Progress in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 4, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 5, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 5, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 5, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 5, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 5, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 6, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 7, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 7, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 8, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 8, 2018

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts moved this from Done to In Progress in vNext+1 (master) Jun 26, 2018

@rmarinho rmarinho added this to Done in Enhancements via automation Jul 30, 2018

rmarinho added a commit that referenced this issue Jul 30, 2018

[Core] Report binding failures (#2911) fixes #2639
* Implemented debug LogListener according to feedback

* Added check to see if log listener is already contained before adding/removing

@samhouts samhouts moved this from In Progress to Done in vNext+1 (master) Jul 30, 2018

@samhouts samhouts removed this from Done in Enhancements Jul 30, 2018

PureWeen added a commit that referenced this issue Aug 28, 2018

[Core] Report binding failures (#2911) fixes #2639
* Implemented debug LogListener according to feedback

* Added check to see if log listener is already contained before adding/removing

@samhouts samhouts removed this from Done in vNext+1 (master) Sep 21, 2018

@samhouts samhouts added this to In Progress in vCurrent (Target 3.3.0) Sep 21, 2018

@samhouts samhouts moved this from In Progress to Closed in vCurrent (Target 3.3.0) Sep 21, 2018

@samhouts samhouts moved this from Closed to Done in vCurrent (Target 3.3.0) Sep 21, 2018

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