[Android] Always set a non-null Device.Info #504

Merged
merged 1 commit into from Nov 8, 2016

Conversation

Projects
None yet
6 participants
@alanmcgovern
Contributor

alanmcgovern commented Nov 3, 2016

When we run under the context of layoutlib the Context object
that is created will not implement IDeviceInfoProvider. All this
means is that we will not get change notifications when the
orientation changes, but that's ok! This won't happen anyway.

If we instantiate the Device.Info object then everywhere else in
the code will be able to get access to the screen properties.

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=44893

@alanmcgovern

This comment has been minimized.

Show comment
Hide comment
@alanmcgovern

alanmcgovern Nov 3, 2016

Contributor

I believe this is a better fix than PR #503 so I'll close that. If this patch is acceptable can we merge it back to the current/next release?

Contributor

alanmcgovern commented Nov 3, 2016

I believe this is a better fix than PR #503 so I'll close that. If this patch is acceptable can we merge it back to the current/next release?

readonly Size _pixelScreenSize;
readonly double _scalingFactor;
Orientation _previousOrientation = Orientation.Undefined;
- public AndroidDeviceInfo(IDeviceInfoProvider formsActivity)
+ public AndroidDeviceInfo(Context formsActivity)

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 3, 2016

Contributor

What happens if you remove the parameter and just use Context everywhere inside this class? I'm not sure what kind of purpose IDeviceInfoProvider serves.

@adrianknight89

adrianknight89 Nov 3, 2016

Contributor

What happens if you remove the parameter and just use Context everywhere inside this class? I'm not sure what kind of purpose IDeviceInfoProvider serves.

This comment has been minimized.

@alanmcgovern

alanmcgovern Nov 3, 2016

Contributor

It has an extra event which is not available on a pure Context object. The ConfigurationChanged event is provided by the interface, not the underlying Context object

@alanmcgovern

alanmcgovern Nov 3, 2016

Contributor

It has an extra event which is not available on a pure Context object. The ConfigurationChanged event is provided by the interface, not the underlying Context object

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 3, 2016

Contributor

What if you cast Context to an Activity? Shouldn't you see the event then?

@adrianknight89

adrianknight89 Nov 3, 2016

Contributor

What if you cast Context to an Activity? Shouldn't you see the event then?

This comment has been minimized.

@alanmcgovern

alanmcgovern Nov 3, 2016

Contributor

There's no guarantee it is an Activity. The information we need exists on Context and we are guaranteed to have a Context, so we should take that information.

If we are lucky and have an Activity then we can definitely cast to that and get more info, or if it's an IDeviceInfoProvider we could cast to that. However we cannot rely on that always being true.

@alanmcgovern

alanmcgovern Nov 3, 2016

Contributor

There's no guarantee it is an Activity. The information we need exists on Context and we are guaranteed to have a Context, so we should take that information.

If we are lucky and have an Activity then we can definitely cast to that and get more info, or if it's an IDeviceInfoProvider we could cast to that. However we cannot rely on that always being true.

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 3, 2016

Contributor

If there is no guarantee that it's an Activity, then _formsActivity should probably be renamed to context. Right now, it implies that it's an Activity.

@adrianknight89

adrianknight89 Nov 3, 2016

Contributor

If there is no guarantee that it's an Activity, then _formsActivity should probably be renamed to context. Right now, it implies that it's an Activity.

This comment has been minimized.

@hartez

hartez Nov 7, 2016

Member

I agree with @adrianknight89 on the naming; _formsActivity should be renamed _context. The bad naming is a pre-existing condition here, so I'm not going to reject the PR if it doesn't get changed, but it'd be nice.

@hartez

hartez Nov 7, 2016

Member

I agree with @adrianknight89 on the naming; _formsActivity should be renamed _context. The bad naming is a pre-existing condition here, so I'm not going to reject the PR if it doesn't get changed, but it'd be nice.

@adrianknight89 adrianknight89 referenced this pull request Nov 3, 2016

Closed

[Core] Make screen info publicly available #484

3 of 4 tasks complete
readonly Size _pixelScreenSize;
readonly double _scalingFactor;
Orientation _previousOrientation = Orientation.Undefined;
- public AndroidDeviceInfo(IDeviceInfoProvider formsActivity)
+ public AndroidDeviceInfo(Context formsActivity)

This comment has been minimized.

@hartez

hartez Nov 7, 2016

Member

I agree with @adrianknight89 on the naming; _formsActivity should be renamed _context. The bad naming is a pre-existing condition here, so I'm not going to reject the PR if it doesn't get changed, but it'd be nice.

@hartez

hartez Nov 7, 2016

Member

I agree with @adrianknight89 on the naming; _formsActivity should be renamed _context. The bad naming is a pre-existing condition here, so I'm not going to reject the PR if it doesn't get changed, but it'd be nice.

Xamarin.Forms.Platform.Android/Forms.cs
@@ -224,7 +224,8 @@ public override double ScalingFactor
protected override void Dispose(bool disposing)
{
- _formsActivity.ConfigurationChanged -= ConfigurationChanged;
+ if (_formsActivity is IDeviceInfoProvider)

This comment has been minimized.

@hartez

hartez Nov 7, 2016

Member

This if block needs to be surrounded by if (disposing){} so it doesn't run in the destructor.

@hartez

hartez Nov 7, 2016

Member

This if block needs to be surrounded by if (disposing){} so it doesn't run in the destructor.

@@ -224,7 +224,8 @@ public override double ScalingFactor
protected override void Dispose(bool disposing)
{

This comment has been minimized.

@hartez

hartez Nov 7, 2016

Member

This needs to properly handle double disposal (i.e., it needs a 'disposed' flag). For an example, see

protected override void Dispose(bool disposing)

@hartez

hartez Nov 7, 2016

Member

This needs to properly handle double disposal (i.e., it needs a 'disposed' flag). For an example, see

protected override void Dispose(bool disposing)

This comment has been minimized.

@alanmcgovern

alanmcgovern Nov 8, 2016

Contributor

Fixed

@alanmcgovern

alanmcgovern Nov 8, 2016

Contributor

Fixed

[Android] Always set a non-null Device.Info
When we run under the context of layoutlib the `Context` object
that is created will not implement IDeviceInfoProvider. All this
means is that we will not get change notifications when the
orientation changes, but that's ok! This won't happen anyway.

If we instantiate the Device.Info object then everywhere else in
the code will be able to get access to the screen properties.

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=44893

Am i supposed to dismiss it when i address the issues?

@StephaneDelcroix StephaneDelcroix merged commit d43b8a2 into master Nov 8, 2016

@rmarinho rmarinho deleted the always-set-DeviceInfo branch Jun 22, 2017

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

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