Geolocation: Add #nullable for Essentials Geolocation code #13371

vividos · 2023-02-15T21:34:00Z

Description of Change

This PR is a follow-up to #9572 and adds #nullable enable to the remaining Essentials' Geolocation classes.

Issues Fixed

No issue, but follow-up code changes left over from #9572.

ghost · 2023-02-15T21:34:09Z

Hey there @vividos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

vividos · 2023-02-15T21:35:48Z

src/Essentials/src/Geolocation/Geolocation.android.cs


-		static LocationManager LocationManager =>
+		static LocationManager? LocationManager =>


I'm not sure if LocationManager can ever be null on a sane Android environment, but there might be some weird custom ROMs; I guess the best is to throw FeatureNotSupportedException then...

@jfversluis there are still my three questions...

vividos · 2023-02-15T21:39:58Z

src/Essentials/src/Geolocation/Geolocation.android.cs

 		{
+			if (provider == null)


as ILocationListener has the provider parameter marked with string?, provider may theoretically be null, but from reading the Android documentation, it makes no sense to be null. Therefore I just return here.

@jfversluis the second...

vividos · 2023-02-15T21:41:37Z

src/Essentials/src/Geolocation/Geolocation.ios.macos.cs

@@ -183,7 +185,7 @@ public void StopListeningForeground()
 				listener.ErrorHandler = null;
 			}

-			listeningManager.Delegate = null;
+			listeningManager.WeakDelegate = null;


I'm not sure setting the WeakDelegate is the proper thing to do here, but since the Delegate now is non-nullable, it can't be set to null anymore.

@jfversluis the third...

vividos · 2023-02-15T21:55:45Z

@jfversluis Here's the rest of the changes I mentioned. I added some comments to the changes that we should discuss.

src/Essentials/src/Geolocation/Geolocation.android.cs

src/Essentials/src/Geolocation/Geolocation.ios.macos.cs

vividos · 2023-02-16T07:41:01Z

@mandel-macaque Thanks for the review comments! I addressed all apart from two; see my above comments. Also I have three original comments attached to the code, which we also should discuss.

jfversluis · 2023-02-16T08:52:31Z

/azp run

azure-pipelines · 2023-02-16T08:53:01Z

Azure Pipelines successfully started running 2 pipeline(s).

jfversluis · 2023-02-16T09:15:31Z

Amazing @vividos thank you! If you're looking for something after this... We might want to enable nullable on the project level and then add #nullable disable to the files that we still need to cover and remove #nullable enable for the ones that are already done.

Besides the other feedback there is these build errors:

/Users/builder/azdo/_work/2/s/src/Essentials/src/Geolocation/Geolocation.tizen.cs(16,25): error CS8613: Nullability of reference types in return type of 'Task GeolocationImplementation.GetLastKnownLocationAsync()' doesn't match implicitly implemented member 'Task<Location?> IGeolocation.GetLastKnownLocationAsync()'. [/Users/builder/azdo/_work/2/s/src/Essentials/src/Essentials.csproj::TargetFramework=net7.0-tizen]
/Users/builder/azdo/_work/2/s/src/Essentials/src/Geolocation/Geolocation.tizen.cs(18,31): error CS8613: Nullability of reference types in return type of 'Task GeolocationImplementation.GetLocationAsync(GeolocationRequest request, CancellationToken cancellationToken)' doesn't match implicitly implemented member 'Task<Location?> IGeolocation.GetLocationAsync(GeolocationRequest request, CancellationToken cancelToken)'. [/Users/builder/azdo/_work/2/s/src/Essentials/src/Essentials.csproj::TargetFramework=net7.0-tizen]

vividos · 2023-02-16T12:01:30Z

I'll check out the Tizen build error.
All of my pull requests have the purpose to make a (distant) future migration of my Forms projects to MAUI easier 😉

jfversluis · 2023-02-16T12:57:32Z

Hey I don't blame you! I can only be happy that you not only help yourself out, but also others in the process. So thank you for doing this!

mandel-macaque

Agree on the ones left as they where, there is a missing comment to be address (probably github hide it by mistake).

src/Essentials/src/Geolocation/Geolocation.android.cs

vividos · 2023-02-17T07:42:00Z

fixed the Tizen errors and still waiting opinions on my three comments from Feb. 15th.

azure-pipelines · 2023-02-17T12:04:11Z

Azure Pipelines successfully started running 2 pipeline(s).

mandel-macaque

@vividos I took the liberty to do a commit fixing some forgotten == null and != null.

jfversluis · 2023-02-17T18:26:05Z

/azp run

vividos · 2023-02-17T18:26:25Z

There's still the three code comments...

azure-pipelines · 2023-02-17T18:26:26Z

Azure Pipelines successfully started running 2 pipeline(s).

jfversluis

I think the rest of your comments are valid, so LGTM! Thank you!

vividos · 2023-02-20T19:43:54Z

Ah sorry I didn't see you comment. Sorry for the noise!

) * enabled nullable checks for GeolocationRequest class * enabled nullable checks for more Geolocation source files * enabled nullable checks for Geolocation Android and iOS implementation * changed null checks as suggested in review * fixed nullable errors for Tizen * Fix some forgotte == null * Fix some forgotte != null --------- Co-authored-by: Manuel de la Pena <mandel@microsoft.com>

vividos added 3 commits February 15, 2023 21:44

enabled nullable checks for GeolocationRequest class

120ce41

enabled nullable checks for more Geolocation source files

e38f534

enabled nullable checks for Geolocation Android and iOS implementation

70a8e18

ghost added the community ✨ Community Contribution label Feb 15, 2023

vividos commented Feb 15, 2023

View reviewed changes

vividos changed the title ~~Geolocation: Add #nullable~~ Geolocation: Add #nullable for Essentials Geolocation code Feb 15, 2023

mandel-macaque requested changes Feb 15, 2023

View reviewed changes

changed null checks as suggested in review

877c808

jsuarezruiz added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Feb 16, 2023

jfversluis requested a review from mandel-macaque February 16, 2023 08:52

jfversluis assigned mandel-macaque Feb 16, 2023

mandel-macaque requested changes Feb 16, 2023

View reviewed changes

src/Essentials/src/Geolocation/Geolocation.android.cs Outdated Show resolved Hide resolved

fixed nullable errors for Tizen

2bc9e6d

mandel-macaque added 2 commits February 17, 2023 11:10

Fix some forgotte == null

cdf8702

Fix some forgotte != null

c6387da

mandel-macaque approved these changes Feb 17, 2023

View reviewed changes

jfversluis approved these changes Feb 20, 2023

View reviewed changes

jfversluis merged commit 8b87c11 into dotnet:main Feb 20, 2023

github-actions bot locked and limited conversation to collaborators Dec 13, 2023

samhouts added the fixed-in-8.0.0-preview.2.7871 Look for this fix in 8.0.0-preview.2.7871! label Aug 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geolocation: Add #nullable for Essentials Geolocation code #13371

Geolocation: Add #nullable for Essentials Geolocation code #13371

vividos commented Feb 15, 2023

ghost commented Feb 15, 2023

vividos Feb 15, 2023

vividos Feb 20, 2023 •

edited

Loading

vividos Feb 15, 2023

vividos Feb 20, 2023

vividos Feb 15, 2023

vividos Feb 20, 2023

vividos commented Feb 15, 2023

vividos commented Feb 16, 2023

jfversluis commented Feb 16, 2023 •

edited

Loading

azure-pipelines bot commented Feb 16, 2023

jfversluis commented Feb 16, 2023 •

edited

Loading

vividos commented Feb 16, 2023

jfversluis commented Feb 16, 2023

mandel-macaque left a comment

vividos commented Feb 17, 2023

azure-pipelines bot commented Feb 17, 2023

mandel-macaque left a comment

jfversluis commented Feb 17, 2023

vividos commented Feb 17, 2023

azure-pipelines bot commented Feb 17, 2023

jfversluis left a comment

vividos commented Feb 20, 2023


		static LocationManager LocationManager =>
		static LocationManager? LocationManager =>

Geolocation: Add #nullable for Essentials Geolocation code #13371

Geolocation: Add #nullable for Essentials Geolocation code #13371

Conversation

vividos commented Feb 15, 2023

Description of Change

Issues Fixed

ghost commented Feb 15, 2023

vividos Feb 15, 2023

Choose a reason for hiding this comment

vividos Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

vividos Feb 15, 2023

Choose a reason for hiding this comment

vividos Feb 20, 2023

Choose a reason for hiding this comment

vividos Feb 15, 2023

Choose a reason for hiding this comment

vividos Feb 20, 2023

Choose a reason for hiding this comment

vividos commented Feb 15, 2023

vividos commented Feb 16, 2023

jfversluis commented Feb 16, 2023 • edited Loading

azure-pipelines bot commented Feb 16, 2023

jfversluis commented Feb 16, 2023 • edited Loading

vividos commented Feb 16, 2023

jfversluis commented Feb 16, 2023

mandel-macaque left a comment

Choose a reason for hiding this comment

vividos commented Feb 17, 2023

azure-pipelines bot commented Feb 17, 2023

mandel-macaque left a comment

Choose a reason for hiding this comment

jfversluis commented Feb 17, 2023

vividos commented Feb 17, 2023

azure-pipelines bot commented Feb 17, 2023

jfversluis left a comment

Choose a reason for hiding this comment

vividos commented Feb 20, 2023

vividos Feb 20, 2023 •

edited

Loading

jfversluis commented Feb 16, 2023 •

edited

Loading

jfversluis commented Feb 16, 2023 •

edited

Loading