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

Add a geolocation service #2462

Merged
merged 24 commits into from
Apr 22, 2024
Merged

Add a geolocation service #2462

merged 24 commits into from
Apr 22, 2024

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Mar 22, 2024

Adds a geolocation service API.

Allows tracking of current location and altitude, ongoing notifications (including background notifications) of changes, and permission handling.

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

@freakboy3742 freakboy3742 marked this pull request as ready for review March 28, 2024 06:25
@freakboy3742
Copy link
Member Author

freakboy3742 commented Mar 28, 2024

@mhsmith This is fully implemented and has 100% coverage... but I'm not sure it's correct on Android. I've only been able to test in the emulator, which has an interface for setting geolocation points; but using that UI doesn't seem to have any impact on the running app:

[Moved to #2525]

I've also noticed what looks like a bug in the map zoom - it should be setting the map zoom to 16 when it creates a pin, but this doesn't seem to be taking effect. I found a similar issue with macOS and iOS - there, the way that zoom was implemented was interacting with setting the map location, so rapid sequential changes would cause only the last change to take effect. I've fixed the issue on iOS and macOS, but the cause won't be the same on Android (at least, not in our code) because "zoom" and "map centre" are two separate APIs. I don't know if this might indicate a bug with OSM itself.

@freakboy3742
Copy link
Member Author

I've also noticed what looks like a bug in the map zoom - it should be setting the map zoom to 16 when it creates a pin, but this doesn't seem to be taking effect.

I've found a fix/workaround for this problem - the issue is that both actions trigger animations, so if you set the zoom while the location is animating, the zoom animation is lost. The fix is to flush any animations to their final state before adding a new animation - there's no analog of the iOS/macOS approach, because there's no public listener that can be used to monitor for the end of an animation. This isn't ideal, because you won't see the effect of the animation for the first part of any location/zoom or zoom/location changes - but you'll still see an animation, so maybe that doesn't matter so much. I've logged this as upstream as osmdroid/osmdroid#1989.

@freakboy3742
Copy link
Member Author

I don't know what's going on with CI. The most recent commit only added code to the Android backend, I'm not sure how that is now causing failures on iOS. There seems to be a strong correlation between how long it takes to start the simulator, and the production of failures... but that might be a red herring.

@freakboy3742
Copy link
Member Author

The CI issue appears to be unrelated to geolocation; see #2476.

docs/reference/api/hardware/index.rst Show resolved Hide resolved
docs/spelling_wordlist Show resolved Hide resolved
docs/reference/api/hardware/geolocation.rst Outdated Show resolved Hide resolved
docs/reference/api/hardware/geolocation.rst Outdated Show resolved Hide resolved
docs/reference/api/hardware/geolocation.rst Outdated Show resolved Hide resolved
docs/reference/api/hardware/geolocation.rst Outdated Show resolved Hide resolved
core/src/toga/hardware/geolocation.py Outdated Show resolved Hide resolved
core/src/toga/hardware/geolocation.py Outdated Show resolved Hide resolved
changes/2462.feature.rst Outdated Show resolved Hide resolved
Comment on lines 183 to 184
If an :any:`on_change` handler is installed, requesting the current location
will also cause that handler to be invoked.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this? We don't call the on_change handler on a TextInput when the code reads its value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it's an odd choice; but it's somewhat forced by an annoying implementation quirk.

On macOS and iOS, you don't explicitly ask for the current location - you can either get the currently cached value (which may not exist), or you request a single location update. There's no "request ID" or other identifier that you can associate with a current location request, so there's no way to differentiate the "source" of an update as being a once-off request, or an ongoing tracking request.

This isn't an issue on Android, as the handler for the two types of update are different; but it's easy to make the behavior consistent.

The options here (as I see them) are:

  1. Generate a change event when current_location is satisfied
  2. Don't generate a change event if any current_location is being satisfied by an update
  3. Modify the API to a passive "request a location update" that expects the user to have a change handler in place to receive the result.

(3) is closer aligned to the macOS/iOS implementation, but seems a bit restrictive as it doesn't let you explicitly request a location and then perform logic on the return value that is different from "I've got a new location" logic.

(2) would potentially work for macOS/iOS - but it means that if you manually request an update, and you've got location tracking enabled, you could potentially miss a location update of interest. I guess having location tracking enabled means you're likely to get an update in another couple of seconds; but if the user stops moving just as they request and update, that might not happen. It's an edge case, but not an inconceivable one.

I ended up going with (1), on the basis that it's not untrue - if you request a current location, you do have an updated location; and it also allows for you to use the API as if it were (3)-style API (i.e., just install an on_change handler to update a map when any change occurs).

I think I can slightly optimise this by modifying the implementation on macOS/iOS so that a change event is only generated if tracking is enabled. That means we'd only see the on_change update if you're actually tracking location, in which case it becomes one more update callback than you were expecting, but you won't get an on_change notification when you're not location tracking. This would let us remove the explicit on_change that has been added to ensure consistency; and then modify the documentation you've referenced here to say that requesting the current location may cause the handler to be invoked, making the macOS/iOS behavior an implementation quirk, rather than anything guaranteed.

@mhsmith
Copy link
Member

mhsmith commented Apr 11, 2024

I can't reproduce this now, but several times on the "Android 14.0 (Google APIs)" emulator I saw the map remain blank, and I could only fix it by restarting the process. In the logs it printed this message hundreds of times:

[Moved to #2495]

Co-authored-by: Malcolm Smith <smith@chaquo.com>
@freakboy3742
Copy link
Member Author

Looks like something in freezegun has an incompatibility with Python 3.13.0a6. The fix has been made in freezegun's main branch; the 3.13 tests will fail until a formal release with that change is available.

@freakboy3742
Copy link
Member Author

I can't reproduce this now, but several times on the "Android 14.0 (Google APIs)" emulator I saw the map remain blank, and I could only fix it by restarting the process. In the logs it printed this message hundreds of times:
...

I've logged this as #2495.

@freakboy3742
Copy link
Member Author

freakboy3742 commented Apr 12, 2024

See #2498 for a fix for the Py3.13 issue.

core/src/toga/hardware/location.py Outdated Show resolved Hide resolved
core/src/toga/hardware/location.py Outdated Show resolved Hide resolved
@mhsmith mhsmith merged commit a9f9716 into beeware:main Apr 22, 2024
34 checks passed
@freakboy3742 freakboy3742 deleted the geolocation branch April 22, 2024 22:14
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