Add Location implementation to Toga GTK #2999
Conversation
| def wait_for_client(*args): | ||
| result.set_result(self.can_get_location) | ||
| self.disconnect(listener_handle) | ||
|
|
||
| listener_handle = self.connect("notify::state", wait_for_client) |
There was a problem hiding this comment.
Here's the "magic" that inheriting from GObject.Object enables 🎉. I tried to think of other ways to do this, but they were all nasty and over-complicated. At the end of the day, requesting permission needs to be aware of the asynchronous nature of the Geoclue client startup, and have a way to schedule itself until after the client is available if this is called early.
There was a problem hiding this comment.
Looks elegant to me. I might question whether it would be worth making the "notification" object an attribute of the impl, rather than actually being the impl - but the notification mechanism will ultimately be the same, so maybe the extra object isn't worth it if the impl can do double duty.
The other option would be to subclass the GeoClue client class to add this "state" property, so everything can be done on a single "native" object.
There was a problem hiding this comment.
A couple of notes and questions answered inline, but this looks like it's on the right path.
By way of testing - the hardware example takes the location APIs for a walk; updating the briefcase configuration to include the extra permissions and system libraries would be helpful (with a possible need to add the same hints to the Briefcase Linux templates)
| def wait_for_client(*args): | ||
| result.set_result(self.can_get_location) | ||
| self.disconnect(listener_handle) | ||
|
|
||
| listener_handle = self.connect("notify::state", wait_for_client) |
There was a problem hiding this comment.
Looks elegant to me. I might question whether it would be worth making the "notification" object an attribute of the impl, rather than actually being the impl - but the notification mechanism will ultimately be the same, so maybe the extra object isn't worth it if the impl can do double duty.
The other option would be to subclass the GeoClue client class to add this "state" property, so everything can be done on a single "native" object.
|
Thanks for the review @freakboy3742. I've got most of the testbed tests passing. I'm still scratching my head a bit on how to understand the distinction between allow and grant request, and how to represent that in the internal state of Toga GTK's I'm also curious to try subclassing First task though is definitely to sort out the permissions stuff 🙂. If you have any tips or elucidations that might help me understand "allow" vs "grant", I'd greatly appreciate it. |
Might need to add this back if this distinction is useful for e.g. permissions, but it would be better to derive from Xdp.Portal.running_under_sandbox
|
I've opened an issue in the geoclue repository regarding the error ambiguity. I was looking through the XDG Location portal code and realised that it does actually report distinct errors for different permission failures (including one I wasn't aware existed, system-wide location settings), but the geoclue simple API is obscuring that... potentially unintentionally. Out of curiosity, it's got me playing around with making the XDG Location portal request directly to see if the real permission error shows up. I suppose one workaround would be to check for sandboxed conditions and only use |
|
Phosh did not reveal anything too immediately useful, aside from the existence of the GNOME setting In non-sandboxed environments, that setting can be checked. In sandboxed environments, we could indeed check for the backgrounding permission. However, I'd prefer to do that in a separate PR, because whether the app has backgrounding permissions generally is of broader concern than to just the location. For sandboxed apps, the check should be handled in the Toga GTK With that in mind, I think to continue moving this PR forward, I will do the following:
Additionally, I'll open an issue to implement background permission checking for GTK via the XDG Portal Background API. |
|
It looks like the CI failures you're seeing are because the CI environment doesn't have the underlying library (and because the import is then failing, the library is being set to None, which is breaking the monkeypatch trying to set an attribute that doesn't exist). The fix is to modify the CI configuration to add the libgeoclue dependency into the test environment. For bonus points, you could also add protection to the test so when the probe is created, if |
|
Thanks for the tip, Russell.
Interesting idea, but I'm not sure about using xfail for this, at least not in CI. If the test is configured to "expectedly fail" if Geoclue isn't present, it could obscure CI configuration issues, and do so in a way no one would be likely to notice. I'd be worried that, for example, a dependency configuration change could accidentally leave out Geoclue, in which case the tests just wouldn't run. I do like the idea to make sure the tests run with the expected dependencies, and to prevent potentially annoying dependency issues for devs running the tests locally with an obscure failure. In 282da75 I've configured the tests to fail if Geoclue isn't available and the execution environment is CI, and to xfail otherwise with a clear explanation. |
|
Because of the various edge cases in the GTK Location implementation, I need to find a way to expand the coverage in test. Some ideas for how to do this, but please let me know if there's a better way:
It feels like this would already be a case somewhere in the code, so I'm wondering if there's an existing way this is handled? I looked around the tests briefly but didn't see anything other than the utility to skip tests on certain platforms. |
That was the intention - but what you've done here is even better. Thanks!
This is the one approach we avoid - so far, there's always been a "high level" version of a test that can exist on every platform.
I'm not sure I fully follow what you're suggesting here. Each platform has a LocationProbe implementation (so the Linux probe and the macOS probe are different); are you suggesting having multiple LocationProbes for Linux?
The general approach is to define a set of tests that exercises all behaviours that are known to exist; and if a specific backend doesn't implement a behavior, use the probe implementation on that backend to raise the skip or xfail, as necessary. Looking at the coverage misses, most of them seem to relate to failure modes - cases where a location couldn't be read, or permission didn't exist, or similar. Assuming those branches are actually reachable in practice (i.e., they're not covering cases that can't be reached because the public interface prevents that case), then the usual approach would be to work out how to describe that test case in abstract terms (e.g., "test_location_request_before_ready" or "test_location_request_permission_cancelled_mid_request"). You then implement that test using calls to the probe, and calls to the public API. This will almost always involve a probe configuration or assertion that is specific to that test - and if it doesn't, make one up that is test specific. If the backend doesn't demonstrate the behavior (e.g., macOS location services can't be in an "initialising" state), then you make the implementation of that probe method an xfail on that probe backend. This means that any new test you add is either an xfail because that behavior can't happen on the platform in question; or it's a pass that duplicates coverage of some API paths because that backend doesn't make a distinction about the different way the code can execute. An example here is the NumberInput tests. There's an abstract idea of incrementing and decrementing the value in a NumberInput; but on iOS, there's no UI manifestation of that feature, so the call to In some cases, where there's a clear feature difference, we'll also take the approach of adding a boolean flag on the probe, and then having two different testing paths. The probe implementations of OptionContainer are an example here. OptionContainer has some radically different interpretations on mobile and desktop (and even some differences on desktop), so there's a couple of flags that control how the testbed runs - but they're not "if iOS"; they're "if more_option_is_stateful", or "if max_tabs is None". Hopefully that make some kind of sense; if you've got an edge case that doesn't seem to fit into that pattern, let me know which one and I'll see if I can make some suggestions. |
|
Point taken and in general I agree, and can certainly work out how to make that happen. The trouble is that due to the differences in how permissions are handled in and out of sandboxed environments, there are branches of the GTK The idea behind creating multiple If I created multiple location probes for toga gtk's testbed, one would be for sandboxed execution and another for non-sandboxed where gsettings exist. |
Ah - now I understand what you were aiming at - and yes, that approach definitely makes some sense. My question is how to make that structure fit into the overall test harness (and make the same test run twice on a single platform). If you're able to parameterise each location test so that you can use one of a list of probes for any given platform, I guess that would work. iOS, Android and macOS have a single "probe configuration", and Linux has two. However, we really want that parameterisation to be defined on the probe (or, at least, on something platform specific, so there isn't a "if Linux: probe_list = ..." block in the tests. That's definitely pushing the limits of my experience with pytest fixtures - but maybe I'm missing an obvious option. Alternatively, you could have 2 complete sets of tests - one for sandboxed operation using the sandbox probe, and one for non-sandboxed operation using the non-sandboxed probe - with the "non-sandboxed" tests being skipped on iOS, Android and macOS (since there's no non-sandboxed option, and thus no non-sandboxed probe there). I'm assuming the "non sandbox" tests will need to exercise significantly different failure modes, so this might be the easiest approach. It doesn't bother me if there's half a dozen tests that don't run on most platforms because they don't have a "non-sandboxed" deployment option. Yet another option would be to run the testbed a second time in a sandboxed setup - i.e., explicitly do a full test suite run under Flatpak. That wouldn't be my preferred option, but I wouldn't rule it out if there's no other way to do this elegantly. Or if you've got another suggestion, I'm open to ideas. |
This matches Geoclue's own expectations regarding permissions: https://gitlab.freedesktop.org/geoclue/geoclue/-/issues/111 TL;DR: Location permissions essentially do not meaningfully exist outside of sandboxed applications on Linux. Furthermore, attempting to respect GNOME's settings only creates confusion for users, especially if GSettings is available on the system but unused by the application user's DE (e.g., KDE). In that case, the setting will be disabled (as that is the default), and the user will have no insight into where or why the location permissions are being denied to the application.
Forgot to install pre-commit hooks in a new clone
|
I've written about it in this commit's message (82f1bed), but it's worth calling out that I've ripped out all the gsettings stuff. This Geoclue issue has a good overview of why it shouldn't be used in non-sandboxed apps anyway, but furthermore, the setting defaults to false, so if it's available but the user's DE doesn't expose it to them (i.e., anything other than GNOME), then Toga apps would never by allowed to get locations. Instead, users should rely on sandboxing for managing location access. Applications running on the host have no reliable protections. |
Fix and simplify the error handling and permission checks to ensure that they match what is actually possible given the known edge cases. This also includes actually fixing the permission error check, which was not correctly implemented. An issue only noticed because of coverage failing on those lines!
|
Now that the Linux tests are all passing, I've gone and broken the rest of the platforms 😀. I need to update their location probes for handling the new test cases I added to the testbed. That's on for tomorrow... and then hopefully this PR will finally be ready for a real review! |
|
I couldn't see any reason why this particular MapView test would fail on Windows if all the other Windows MapView tests passed. I've merged main into this branch and pushed again in hopes it's just a fluke 🤞. If not, I'll need some advice on debugging the issue... or maybe spin up a Windows VM 😅 |
That test is one that is known to fail intermittently (#2632). For some reason, Github's CI has a networking configuration that seems to straight up fail with alarming regularity. If you're able to shed any light on that, suggestions are welcome - but otherwise, we're living with it for now as an annoying wart. |
|
Gotcha, I should have spent more than a few seconds searching issues for clues 😀. Unfortunately, I have no light to shed. We ran into some weird flakiness in Openverse's CI with GitHub runners, specifically with ports randomly being occupied, which we "fixed" by just retrying until the ports were available 😬. That did indeed make the issue of annoying failures in CI go away, not pretty though, and a waste of time in cases when the failure is legitimate. For this case with the MapView, I'd wonder if there's a way to cut the network out of the question for these tests altogether. It's curious that this test in particular is the only flaky one, and also only on Windows? In any case, this PR is ready to review. I've left a couple of comments to give some additional context, but I look forward to feedback on this! |
It's not that surprising - we see the analogous failure (but less often) on GTK; and Winforms and GTK are the two backends that use an explicit WebView contacting the OSM servers.
👍 I'll try to take a look this afternoon - tomorrow at the latest. |
There was a problem hiding this comment.
I've flagged a couple of testing details, which are as much an issue of "local style guide" as anything else; and one question about handling of permission changes, but otherwise, this looks like a really solid implementation - nice work.
I'm not having much luck with manual testing, though (Ubuntu 22.04, non-sandboxed, running in a Parallels VM). On my first attempt, the initial permission request took ~20s to return, and it always failed. I found the Ubuntu setting to enable location permissions - but then the initial permission request never returns.
Am I missing something obvious here? I can't rule out that it's my Parallels VM that is the issue - there are some network requests that a little longer they should on a native machine. What sort of delays/timings do you see?
| will already be available (see :meth:`~.Location.has_background_permission()`), | ||
| and the upstream location API never has to call this method. | ||
| """ | ||
| raise NotImplementedError("Background permissions are non-existent on Linux") |
There was a problem hiding this comment.
I'm inclined to say this should immediately return True, rather than an error. You don't need to ask for permission, but in a cross-platform app, you will need to request background permission to support those platforms that need the permission; having that API raise an exception in a case where it doesn't matter could be jarring.
The core API will prevent asking for background permission until foreground permission has been approved; so I think we're safe to just return True.
There was a problem hiding this comment.
Sure thing. It'll have to have a no cover pragma on it, though, because has_background_permission is just a call to has_permission in this implementation. When the Toga user calls request_background_permission, the core Location class will bypass the actual request because has_background_permission will already be true (and must be, because you have to request foreground permission before background). In other words, there's no way to get this method to run via the core Location api. I can add that in a comment explaining why it can't be covered, though.
Here's the relevant code: https://github.com/sarayourfriend/toga/blob/add/gtk-geoclue-geolocation/core/src/toga/hardware/location.py#L119-L132
There was a problem hiding this comment.
Sure - as long as the code has a brief explanatory note for why the no-cover is justified, that's fine.
| @pytest.fixture | ||
| def supports_background_permission(location_probe): | ||
| return getattr(location_probe, "supports_background_permission", True) |
There was a problem hiding this comment.
I think this fixture might become unnecessary if request_background_permission returns True. If the attribute is still required, preference is to define it on all backends so it's explicit that background permission is available.
There was a problem hiding this comment.
The difficulty is that has_background_permission returns the result of has_permission. This is an issue for the test_deny_background_permission test, which this fixture bypasses and explicitly requires foreground permission to be granted while background is denied. The Linux location permission model just doesn't have that case.
The situation in those two assertions is impossible for the GTK Location backend.
There was a problem hiding this comment.
Ah - got it. So - the fixture won't be needed (if the attribute exists on every probe backend); but I can see that there may be no way around a programmatic "skip-if-probe-doesn't-have-background-permission" check.
| ) | ||
|
|
||
| def has_permission(self): | ||
| return bool(self.permission_result) |
There was a problem hiding this comment.
What happens if permission is revoked after being initially allowed? Is that even a possibility? If it is, it looks like the listener handle is removed once an "initialised" state is received.
There was a problem hiding this comment.
If you run an example in Flatpak and use Flatseal, you can deny permissions while the app is running and already has location permission... but the XDG Location Portal doesn't send any signal to that fact, and location updates still propagate. If the app restarts, then permission does go away. I haven't found any way to revoke permissions while the app is running and after it's already requested permissions.
In a non-sandboxed environment, the state can end up in FAILED if the system service closes, which is handled by the listener attached to the client:
A "client" is only available in non-sandboxed environments, as mentioned in the Geoclue documentation for the method linked to in the section I've linked above.
In that case, tracking will cease, and calls to current_location and start_tracking will fail with a RuntimeError.
This isn't a permissions issue, though. It could be the Geoclue agent crashing, for example, and you can reproduce it by running the app outside a sandbox and running systemctl stop geoclue. That is why the code I linked above sets the state to FAILED rather than DENIED, and doesn't reset the permission result.
You're probably running into https://gitlab.gnome.org/GNOME/gnome-maps/-/issues/700, unless you've already addressed that by modifying the Geoclue configuration to work around it. If you check journalctl you would see some errors from geoclue about a You can swap out MLS for another provider, Alternatively, you can set a static geolocation. Detailed instructions for this are in Edit: to mention a benefit of the static geolocation file: you can actually simulate changes to the location by just editing the file. If you Further edit: If that still doesn't work, to see whether Toga is the issue or if it's upstream, try running GNOME Weather or Maps, or one of the Geoclue demos (e.g., where-am-i which I believe Ubuntu includes when you install |
Co-authored-by: Russell Keith-Magee <[email protected]>
Co-authored-by: Russell Keith-Magee <[email protected]>
xfail directly in the probe rather than awkwardly relying on truthy response from the method
Oh, that it were so :-) Ubuntu's 22.04 config has a whole configuration block (commented out) for using Google's geolocation service, but no configuration for position.xyz, and no mention of Ubuntu 24.04 does have a mention of the These details about geolocation capturing would be worth capturing - at the very least in a comment at the top of the GTK location file (so future developers know what is going on); but a more generic note on platform quirks would also be worthwhile. In particular, the fact you need to open the Gnome settings panel and select Privacy > Location to enable before using geolocation is something that is very different to macOS/iOS/Android, where you'll be prompted to authorize on first use. However, I'm still getting unexpected behavior - the permissions are all tracking as expected; but a manual location update isn't triggering the on_change handler. You can demonstrate this with the
That's a neat trick - worth capturing somewhere as well. |
👍 for dev docs. I'll get those in a commit soon. For user docs, I'd be inclined to have a general note along the lines of:
I'd be worried about going into too many specific details, considering behaviour differs even between desktop environments. To my knowledge, KDE (the DE I use) has no analagous location privacy setting. Maybe linking the GNOME docs is wiser, but their instructions differ from the Ubuntu ones. I also don't see any documentation regarding such a setting on Linux Mint, which surprises me, I wonder if it defaults to enabled there.
For what it's worth, the experience on KDE is identical to what you are describing. Fedora ships with Geoclue and I'm fairly certain I didn't even need to enable the systemd service. It "just worked". Kind of a mess with this one... I'm really looking forward to seeing what interesting complexity lies when I take a shot at implementing the camera service 😅. |
Completely agreed - it's not our job to provide step-by-step user guides for every possible Linux distribution. As long as we flag the issues that need to be considered, and hint in the general direction of the sorts of places a user might want to look to address those issues ("you may need to enable geolocation services; there may also be a setting in your DE settings panel..."), that's sufficient IMHO.
The service was enabled on Ubuntu... it just didn't have any working location providers, and the privacy setting was turned off with no trigger indicating that this might be a problem. 🤷
I'm not going to question your life choices when they benefit Toga as a project 😝 |
I'm not following what the exact issue is, in fact, I think the hardware example might be expecting incorrect behaviour. At the very least, I'm not seeing code in any of the The core The GTK I think the hardware example app is implemented incorrectly, in this regard. It should instead capture the result of diff --git a/examples/hardware/hardware/app.py b/examples/hardware/hardware/app.py
index 4e8790ce7..d8d53b31a 100644
--- a/examples/hardware/hardware/app.py
+++ b/examples/hardware/hardware/app.py
@@ -118,6 +118,9 @@ class ExampleHardwareApp(toga.App):
)
def location_changed(self, geo, location, altitude, **kwargs):
+ self._set_location(location)
+
+ def _set_location(self, location):
self.map_view.location = location
if self.pin is None:
@@ -131,8 +134,8 @@ class ExampleHardwareApp(toga.App):
try:
await self.location.request_permission()
- # Getting the current location will trigger the on_change handler
- await self.location.current_location()
+ location = await self.location.current_location()
+ self._set_location(location)
except NotImplementedError:
await self.main_window.dialog(Are you seeing the hardware example app function the way you described with the manual update on macOS, by chance? I really can't see what in the code would make it behave the way the example app asserts it should 🤔 |
Huh... looks like you're right. I know the hardware example did work as implemented at some point in the past - I distinctly remember needing to disable the explicit location set in the hardware example code because it was triggering the That also matches the documentation of So - it looks like your implementation is correct, and the Hardware example isn't; I guess a fix is called for there. Thanks for keeping me honest/apologies for leading you astray :-) |
Fixes #2990
This PR is still a work in process. It needs testing and documentation of some caveats for Linux location handling (some of which are discussed in the linked issue).
PR Checklist: