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

[Core] Report binding failures #2911

Merged
merged 2 commits into from Jul 30, 2018

Conversation

6 participants
@pingzing
Contributor

pingzing commented Jun 3, 2018

Description of Change

A lot of logging happens internally, for example when a binding fails. In order to make these logs more accessible to the end-user, the Application.LogWarningsToApplicationOutput property was introduced. WIth this, logging will also be outputted to the debug console.

Bugs Fixed

API Changes

Added:

public static bool Application.LogWarningsToApplicationOutput { get; set; }

Behavioral Changes

When Application.LogWarningsToApplicationOutput is set to true, a LogListener is attached to the internal logging system which outputs the messages through a Debug.Writeline. When set to false the LogListener is removed again.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

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

@rmarinho rmarinho requested a review from StephaneDelcroix Jun 4, 2018

@StephaneDelcroix

StephaneDelcroix requested changes Jun 7, 2018 edited

Thanks for this PR. Enabling reporting on binding issues is definitely something we want. But the solution you're proposing looks a bit too complex and complicated for what it really is.

Don't get me wrong, it's nicely done, but most of the time, the users do not need that level of atomicity.

The alternate solution right now for the user is to add a LogListener, but that API is now internal.
So, in order to ship to capability, I would prefer if you could redo this by only adding a public API that will add a LogListener that connect to Debug.WriteLine.

After, we will want to update our templates so this is used by default while running the app in DEBUG mode.

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jun 9, 2018

Thanks for the extensive feedback @StephaneDelcroix!

Do you have any suggestion on where the public API might live? Is there already a good place to put this?

Add the DebugSettings class as we did now, with a public API and let that add the LogListener?
Or maybe a static method on the Binding class?

@StephaneDelcroix

This comment has been minimized.

Member

StephaneDelcroix commented Jun 11, 2018

@jfversluis:

a static property in Application class, maybe ?

so you'd have this in your App.xaml.cs code behind:

public App() {
#if DEBUG
    Application.LogWarningsToApplicationOutput |= true;
#endif
    InitializeComponent();
}

But that could be in Device as well, and feel free to pick up a better name for the property.

@jassmith thoughts ^^ ?

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jun 12, 2018

Sounds good to me, I feel Application would make more sense than Device since it applies to the application lifecycle and doesn't really tell you anything about the device.

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jun 12, 2018

@StephaneDelcroix see changes, more like this then?

@StephaneDelcroix

This comment has been minimized.

Member

StephaneDelcroix commented Jun 13, 2018

@jfversluis please update PR description to match the new implementation choices

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jun 14, 2018

As much as I want to, I didn't open the initial PR :) @pingzing ?

@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jun 14, 2018

I'd propose this, maybe you could replace it @StephaneDelcroix

Description of Change

In regard to issue 2639. A lot of logging happens internally, for example when a binding fails. In order to make these logs more accessible to the end-user, the Application.LogWarningsToApplicationOutput property was introduced. WIth this, logging will also be outputted to the debug console.

Bugs Fixed

  • N/A

API Changes

Added:

public static bool Application.LogWarningsToApplicationOutput

Behavioral Changes

When Application.LogWarningsToApplicationOutput is set to true, a LogListener is attached to the internal logging system which outputs the messages through a Debug.Writeline. When set to false the LogListener is removed again.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@jfversluis

This comment has been minimized.

Contributor

jfversluis commented Jul 11, 2018

@StephaneDelcroix what needs to happen to get this merged? 🙂

@rmarinho rmarinho merged commit 1a71035 into xamarin:master Jul 30, 2018

4 checks passed

VSTS: Xamarin Forms (PR Builds) PR-2911 - (1816035) succeeded
Details
VSTS: Xamarin Forms OSX PR-2911 - (1816055) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-2911 - (1816038) succeeded
Details
license/cla All CLA requirements met.
Details

vNext+1 (master) automation moved this from In Review to Done Jul 30, 2018

@jfversluis jfversluis deleted the jfversluis:feature/issue-2639 branch Jul 31, 2018

PureWeen added a commit that referenced this pull request 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 added this to the 3.3.0 milestone Sep 20, 2018

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

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