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. |
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 :-) |
|
All good! I pushed that fix for the hardware app as #3045, but let me know if you'd prefer me to roll it into this PR. |
|
Thanks again for the review @freakboy3742. The documentation changes we discussed are up in the latest commits. Earlier today when doing some research on libaperture (which is now defunct for GTK3), I discovered GeoClueless, which makes testing location tracking updates a cinch. I reference it in the module docs as well as the manual static-provider alternative. I also realised I'd been capitalising GeoClue incorrectly in the prose documentation. While the PyGObject binding uses |
docs/spelling_wordlist
Outdated
| GeoClue | ||
| Geoclue |
There was a problem hiding this comment.
+1 to using the "official" capitalisation; do we need to preserve the Geoclue form?
There was a problem hiding this comment.
I left it in because for some reason I thought it would raise errors on the references to the Geoclue.Simple class, but that was an incorrect understanding on my part. Removed in 0de43b9
| GeoClue clients. That is, you can effectively test tracking location updates by | ||
| modifying the coordinates in the file, and that will propagate to any listening | ||
| process the same way a "real" location update would. | ||
| """ |
There was a problem hiding this comment.
This is great - and nice find with geoclueless - that looks like a really handy tool.
Fixes #2990
Adds a
Locationservice implementation for Toga GTK using the standard Linux geolocation library and system service, GeoClue.See the new
location.pymodule's docstring for instructions on testing with GeoClue. Aside from the unit tests, use the hardware example app along with one of the approaches from the module docstring to see this in action.PR Checklist: