Skip to content
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

feat(winforms): location support #3025

Open
wants to merge 69 commits into
base: main
Choose a base branch
from
Open

Conversation

Mause
Copy link
Contributor

@Mause Mause commented Dec 8, 2024

This PR adds support for location access on Windows via System.Device.Location.

There are still some rough edges here, as the only way to request location access is to try and access it, which isn't amazing.
Additionally there's no distinction between foreground and background access.

The code coverage isn't there all the way yet either - there are two missing lines, which I'm looking at fixing, but figured this was functional enough to get some reviews.

Also I've added some code to upload a raw coverage report, as the testbeds don't seem to have any mechanism in place to do that.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@Mause
Copy link
Contributor Author

Mause commented Dec 8, 2024

I think it would probably be worth waiting for #2999 to be merged before looking to merge this

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted about waiting until #2999 lands; but this looks pretty solid.

My only big question stems from the what the watcher API is being used. AIUI, this effectively turns on the watcher as soon as the user asks for permission, and essentially never turns it off. I can see how that works, but it slightly concerns me from a battery usage perspective (Ok, that's not a huge concern for desktop, but it's not nothing).

The solution/workaround depends a little on whether Start() and Stop() are blocking methods; but it seems like it should be possible to use onPermissionStatusChanged, checks of the actual permission state, and possibly deferring call to set_result() to inside the callback so that a Stop() is called for every "transient" check, unless continuous tracking is turned on (in which case, Stop() is called in stop_tracking(). Essentially, there would always be handlers installed; but it would be a question of application state whether the Toga is notified of a position update.

Does that makes sense?

@@ -90,6 +90,8 @@ async def test_grant_background_permission(app, location_probe):

async def test_deny_background_permission(app, location_probe):
"""A user can deny background permission to use location."""
skip_on_platforms("windows")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach we take for this kind of skip is to make the probe do the skip/xfail when an attempt is made to use the feature. In this case, we'd make request_background_location() raise an XFAIL - we use skip to indicate a feature hasn't been implemented yet, and XFAIL to indicate "this won't ever be implemented because the platform fundamentally doesn't support this idea".


def request_permission(self, future: AsyncResult[bool]) -> None:
self.watcher.Start(False) # TODO: where can we call stop?
future.set_result(self.has_permission())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Start() a blocking method? A method that never returns False? My concern here is whether has_permission() is given a chance to return False on the basis of user input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this overload of Start is blocking yes, but the docs aren't super clear, and it doesn't seem possible to clear the flag on the python exe to test locally properly

@Mause
Copy link
Contributor Author

Mause commented Dec 10, 2024

Noted about waiting until #2999 lands; but this looks pretty solid.

My only big question stems from the what the watcher API is being used. AIUI, this effectively turns on the watcher as soon as the user asks for permission, and essentially never turns it off. I can see how that works, but it slightly concerns me from a battery usage perspective (Ok, that's not a huge concern for desktop, but it's not nothing).

The solution/workaround depends a little on whether Start() and Stop() are blocking methods; but it seems like it should be possible to use onPermissionStatusChanged, checks of the actual permission state, and possibly deferring call to set_result() to inside the callback so that a Stop() is called for every "transient" check, unless continuous tracking is turned on (in which case, Stop() is called in stop_tracking(). Essentially, there would always be handlers installed; but it would be a question of application state whether the Toga is notified of a position update.

Does that makes sense?

That does somewhat make sense yes, aside from the fact that onPermissionStatusChanged doesn't actually exist 😉

I think the issue with that would be a sacrifice of location quality, as it takes a while for the watcher to pin down the location, in my case, picking the Perth CBD (i assume via IP geocoding), before (I assume) using WIFI for a more accurate location. If we're okay with the current location result being very coarse unless the watcher is turned on fully, then happy to pursue that path

@Mause
Copy link
Contributor Author

Mause commented Dec 10, 2024

I guess another alternative to improve the location quality, would be to await a decent quality location being returned (by tweaking the config on the watcher itself maybe)

@freakboy3742
Copy link
Member

That does somewhat make sense yes, aside from the fact that onPermissionStatusChanged doesn't actually exist 😉

I can see how that could be a possible impediment to progress... 😅

So... why is it so lovingly documented if it doesn't exist? Is this a gap in what Python.net can expose? A .NET version thing?

I think the issue with that would be a sacrifice of location quality, as it takes a while for the watcher to pin down the location
...
I guess another alternative to improve the location quality, would be to await a decent quality location being returned (by tweaking the config on the watcher itself maybe)

There's no problem with the API taking time to return a response - that's why it's an async API. Would that mean wrapping any "transient" call with a Start()/Stop() pair is a viable solution?

@Mause
Copy link
Contributor Author

Mause commented Dec 11, 2024

That does somewhat make sense yes, aside from the fact that onPermissionStatusChanged doesn't actually exist 😉

I can see how that could be a possible impediment to progress... 😅

So... why is it so lovingly documented if it doesn't exist? Is this a gap in what Python.net can expose? A .NET version thing?

Well, it isn't documented? I think you misread "position" as "permission"?

I think the issue with that would be a sacrifice of location quality, as it takes a while for the watcher to pin down the location
...
I guess another alternative to improve the location quality, would be to await a decent quality location being returned (by tweaking the config on the watcher itself maybe)

There's no problem with the API taking time to return a response - that's why it's an async API. Would that mean wrapping any "transient" call with a Start()/Stop() pair is a viable solution?

I believe so yes? I'll look into it and test it out 😁

@Mause
Copy link
Contributor Author

Mause commented Dec 11, 2024

@freakboy3742
Copy link
Member

I can see how that could be a possible impediment to progress... 😅
So... why is it so lovingly documented if it doesn't exist? Is this a gap in what Python.net can expose? A .NET version thing?

Well, it isn't documented? I think you misread "position" as "permission"?

I definitely got the method name wrong in the link I provided; but the link itself works, and the GeoPositionStatus enum looks like it refers to permissions - or at the very least, that it might be able to provide feedback for the "disabled" case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants