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

Make Glimpse check if things are enabled in DataBoundControlAdapter (fix for #779) #789

Merged
merged 2 commits into from Apr 29, 2014

Conversation

Projects
None yet
3 participants
@grahammendick

grahammendick commented Apr 29, 2014

When Glimpse is enabled and then disabled the DataBoundControlAdapter is still registered but GlimpseConfiguration.GetConfiguredTimerStrategy()() returns null. Defensive checks added so event listeners only added when Glimpse enabled.
@CGijbels Couldn't use GlimpseConfiguration.RuntimePolicyStrategy() because couldn't access Glimpse context from the ControlAdapter (it's not like a normal Inspector). Also I tried moving the Adapter registration into Setup but the DataBoundControlAdapter wasn't registered successfully. Try it yourself and you'll see that the DataBound information doesn't appear in the tab.

Added check for non-null TimerStrategy()
When Glimpse is disabled GetConfiguredTimerStrategy() returns null so
added defensive checks
Show outdated Hide outdated source/Glimpse.WebForms/Inspector/DataBoundControlAdapter.cs
@@ -157,7 +160,7 @@ public DataBoundControlAdapter()
private bool AdapterStateCopied { get; set; }
private static readonly MethodInfo BeginRenderInfo = typeof(System.Web.UI.Adapters.PageAdapter).GetMethod("BeginRender", BindingFlags.Instance | BindingFlags.NonPublic);
private static readonly MethodInfo BeginRenderInfo = typeof(ControlAdapter).GetMethod("BeginRender", BindingFlags.Instance | BindingFlags.NonPublic);

This comment has been minimized.

@grahammendick

grahammendick Apr 29, 2014

Changed PageAdapter to ControlAdapter (not necessary for the fix but it's clearer)

@grahammendick

grahammendick Apr 29, 2014

Changed PageAdapter to ControlAdapter (not necessary for the fix but it's clearer)

Show outdated Hide outdated source/Glimpse.WebForms/Inspector/DataBoundControlAdapter.cs
@@ -73,10 +73,13 @@ private TimeSpan Offset
protected override void OnInit(EventArgs e)
{
if (GlimpseConfiguration.GetConfiguredTimerStrategy()() != null)

This comment has been minimized.

@grahammendick

grahammendick Apr 29, 2014

Defensive check

@grahammendick

This comment has been minimized.

@avanderhoorn

avanderhoorn Apr 29, 2014

Member

Looks good. But I think the tabs/spaces are off here. See here.

@avanderhoorn

avanderhoorn Apr 29, 2014

Member

Looks good. But I think the tabs/spaces are off here. See here.

Show outdated Hide outdated source/Glimpse.WebForms/Inspector/DataBoundControlAdapter.cs
protected override void OnInit(EventArgs e)
{
CopyAccessState();
if (GlimpseConfiguration.GetConfiguredTimerStrategy()() != null)

This comment has been minimized.

@grahammendick

grahammendick Apr 29, 2014

Defensive check

@grahammendick
@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Apr 29, 2014

Member

This looks good to me and if its tested, etc, I'm happy for it to come it. Let me know.

Member

avanderhoorn commented Apr 29, 2014

This looks good to me and if its tested, etc, I'm happy for it to come it. Let me know.

@grahammendick

This comment has been minimized.

Show comment
Hide comment
@grahammendick

grahammendick Apr 29, 2014

Sorry about the tabs, it's fixed now

grahammendick commented Apr 29, 2014

Sorry about the tabs, it's fixed now

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Apr 29, 2014

Collaborator

@grahammendick thanks for the fix. Maybe you already had the issue with the adapter not registering during setup while developing the feature and that that is why it is now part of the GetData method, so no worries there 😉

Regarding

Couldn't use GlimpseConfiguration.RuntimePolicyStrategy() because couldn't access Glimpse context from the ControlAdapter

I see what you mean, as it is an instance property and not statically accessible (but that has been fixed in v2 😉)

Just wondering why you also changed the reflection parts aka from System.Web.UI.Adapters.PageAdapter to ControlAdapter ?
Was it related to the issue or something that had to be done and piggybacked on this fix? (Not that it matters, I'm just curious 😉)

Collaborator

CGijbels commented Apr 29, 2014

@grahammendick thanks for the fix. Maybe you already had the issue with the adapter not registering during setup while developing the feature and that that is why it is now part of the GetData method, so no worries there 😉

Regarding

Couldn't use GlimpseConfiguration.RuntimePolicyStrategy() because couldn't access Glimpse context from the ControlAdapter

I see what you mean, as it is an instance property and not statically accessible (but that has been fixed in v2 😉)

Just wondering why you also changed the reflection parts aka from System.Web.UI.Adapters.PageAdapter to ControlAdapter ?
Was it related to the issue or something that had to be done and piggybacked on this fix? (Not that it matters, I'm just curious 😉)

@grahammendick

This comment has been minimized.

Show comment
Hide comment
@grahammendick

grahammendick Apr 29, 2014

@CGijbels I just piggybacked off the work @avanderhoorn had already done for the PageAdapter so that's why the code was in the GetData.

The reflection change was fixing my earlier sloppy paste from the PageAdapter. It worked fine because PageAdapter inherits from ControlAdapter but it was misleading because the DataBoundControlAdapter didn't inherit from PageAdapter.

grahammendick commented Apr 29, 2014

@CGijbels I just piggybacked off the work @avanderhoorn had already done for the PageAdapter so that's why the code was in the GetData.

The reflection change was fixing my earlier sloppy paste from the PageAdapter. It worked fine because PageAdapter inherits from ControlAdapter but it was misleading because the DataBoundControlAdapter didn't inherit from PageAdapter.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Apr 29, 2014

Member

We all good for this to go?

Member

avanderhoorn commented Apr 29, 2014

We all good for this to go?

@avanderhoorn avanderhoorn added this to the vNext milestone Apr 29, 2014

@avanderhoorn avanderhoorn self-assigned this Apr 29, 2014

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Apr 29, 2014

Collaborator

:shipit:

Collaborator

CGijbels commented Apr 29, 2014

:shipit:

avanderhoorn added a commit that referenced this pull request Apr 29, 2014

Merge pull request #789 from grahammendick/Issue-#779
Issue #779 Fix - Check Glimpse is enabled in DataBoundControlAdapter

@avanderhoorn avanderhoorn merged commit 8f05d51 into Glimpse:master Apr 29, 2014

@avanderhoorn avanderhoorn changed the title from Issue #779 Fix - Check Glimpse is enabled in DataBoundControlAdapter to Check Glimpse is enabled in DataBoundControlAdapter (fix for #779) Apr 29, 2014

@avanderhoorn avanderhoorn changed the title from Check Glimpse is enabled in DataBoundControlAdapter (fix for #779) to Make Glimpse check if things are enabled in DataBoundControlAdapter (fix for #779) Apr 29, 2014

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