.
Created attachment 85438 [details] [review] locator: Really take distance threshold into account This fixes several issues that led to ignoring the DistanceThreshold value most of the time: - we were ignoring the threshold value when the description from the IP geolocation service changed; - double equality comparison was done with == and not taking epsilon into account; - location threshold was interpreted in km, whereas most application authors expect it to be in meters.
Created attachment 85439 [details] [review] interface: Add a comment for DistanceThreshold Clarify that the value is in meters.
Comment on attachment 85438 [details] [review] locator: Really take distance threshold into account Review of attachment 85438 [details] [review]: ----------------------------------------------------------------- Looks good otherwise but I think we want to update location if only description changes?
Comment on attachment 85439 [details] [review] interface: Add a comment for DistanceThreshold Review of attachment 85439 [details] [review]: ----------------------------------------------------------------- ::: src/geoclue-interface.xml @@ +8,4 @@ > </interface> > <interface name="org.freedesktop.GeoClue2.Client"> > <property name="Location" type="o" access="read"/> > + <!-- Distance threshold in meters --> Nak, we need to add proper docs here. See https://developer.gnome.org/gio/2.29/gdbus-codegen.html on how to do that.
(In reply to comment #3) > Comment on attachment 85438 [details] [review] [review] > locator: Really take distance threshold into account > > Review of attachment 85438 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks good otherwise but I think we want to update location if only > description changes? Oh hm. Does the IP geolocation service always change the desc when the location changes? In any case, if we want to keep the description change check, then the current code has one more issue: g_strcmp0 (desc, new_desc) == 0 This is going to evaluate to true when both desc and new_desc are NULL, which I assume is going to be the case when get the location from GPS.
(In reply to comment #5) > (In reply to comment #3) > > Comment on attachment 85438 [details] [review] [review] [review] > > locator: Really take distance threshold into account > > > > Review of attachment 85438 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > Looks good otherwise but I think we want to update location if only > > description changes? > > Oh hm. Does the IP geolocation service always change the desc when the > location changes? Not necessarily but its irrelevant. The point is that if desc changes, we declare it a new location and disregard the fact that lat/long remains the same. I do agree that its a very theoretical problem at the moment. > In any case, if we want to keep the description change check, then the > current code has one more issue: > > g_strcmp0 (desc, new_desc) == 0 > > This is going to evaluate to true when both desc and new_desc are NULL, > which I assume is going to be the case when get the location from GPS. Why would this be a problem? If other props remain the same, we just declare it the same location and not update.
(In reply to comment #6) > Why would this be a problem? If other props remain the same, we just declare > it the same location and not update. Ahh, makes sense now. It's just that the check is currently coded the other way around: "if the descriptions are null, ignore any other prop changes". I'll go fix it up and post an updated patch.
Comment on attachment 85438 [details] [review] locator: Really take distance threshold into account Marking obsolete on Kalev's behalf.
Comment on attachment 85439 [details] [review] interface: Add a comment for DistanceThreshold Marking obsolete on Kalev's behalf.
Created attachment 85516 [details] [review] location-info: Add gclue_location_info_equal
Created attachment 85517 [details] [review] locator: Use the new gclue_location_info_equal
Created attachment 85518 [details] [review] locator: Add a local priv variable
Created attachment 85519 [details] [review] locator: Use g_clear_object
Created attachment 85520 [details] [review] locator: Fix units in distance threshold comparison Make sure we don't compare kilometers to meters.
Created attachment 85521 [details] [review] locator: Move distance threshold comparison earlier This makes sure we don't emit the LocationUpdated signal when the location change is below the threshold. In gnome-settings-daemon's datetime plugin we're setting the threshold to 50000 m (city level) and aren't interested in small changes within one city. Previously, the signal would be emitted when the user moved to another part within the same city -- due to the changed description from IP geolocation.
Created attachment 85522 [details] [review] interface: Add some documentation Document the DistanceThreshold property and LocationUpdated signal.
Comment on attachment 85516 [details] [review] location-info: Add gclue_location_info_equal Review of attachment 85516 [details] [review]: ----------------------------------------------------------------- ack
Comment on attachment 85517 [details] [review] locator: Use the new gclue_location_info_equal Review of attachment 85517 [details] [review]: ----------------------------------------------------------------- So nak but thanks for taking this approach. Thats how I should have done it. :) ::: src/gclue-locator.c @@ +168,3 @@ > distance = gclue_location_info_get_distance_from > (locator->priv->location, location); > + if (gclue_location_info_equal (locator->priv->location, location) && This won't work. gclue_location_info_equal() check for equality of coordinates so the distance check becomes meaningless. I think you gclue_location_info_equal() to compare distance and pass it the distance threshold to be able to do that.
Comment on attachment 85518 [details] [review] locator: Add a local priv variable Review of attachment 85518 [details] [review]: ----------------------------------------------------------------- ack
Comment on attachment 85519 [details] [review] locator: Use g_clear_object Review of attachment 85519 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-locator.c @@ +177,3 @@ > update: > g_debug ("Updating location"); > + g_clear_object (&priv->location); why? if we jump to 'update' directly, we know that there is nothing to unref and if we don't, we know that there is something to unref.
Comment on attachment 85520 [details] [review] locator: Fix units in distance threshold comparison Review of attachment 85520 [details] [review]: ----------------------------------------------------------------- good otherwise ::: src/gclue-locator.c @@ -161,4 @@ > GClueLocationInfo *location) > { > GClueLocatorPrivate *priv = locator->priv; > - gdouble distance; since distance is only in KMs here, i don't think it needs to be renamed.
Comment on attachment 85521 [details] [review] locator: Move distance threshold comparison earlier Review of attachment 85521 [details] [review]: ----------------------------------------------------------------- I thought we agreed that changes in description or accuracy are take as change in location. Since these changes wont really be happening in practice, I don't think you need to worry about getting redundant signals. If it becomes a real problem, we can think about have some props for apps to suppress this behavior.
Comment on attachment 85522 [details] [review] interface: Add some documentation Review of attachment 85522 [details] [review]: ----------------------------------------------------------------- ack
(In reply to comment #20) > Comment on attachment 85519 [details] [review] [review] > locator: Use g_clear_object > > Review of attachment 85519 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: src/gclue-locator.c > @@ +177,3 @@ > > update: > > g_debug ("Updating location"); > > + g_clear_object (&priv->location); > > why? if we jump to 'update' directly, we know that there is nothing to unref > and if we don't, we know that there is something to unref. It made it slightly easier for me to keep track of the control flow around the goto instructions: with g_clear_object I no longer have to care if the pointer is NULL or not. Could very well be that I'm imagining it :)
(In reply to comment #22) > Comment on attachment 85521 [details] [review] [review] > locator: Move distance threshold comparison earlier > > Review of attachment 85521 [details] [review] [review]: > ----------------------------------------------------------------- > > I thought we agreed that changes in description or accuracy are take as > change in location. Since these changes wont really be happening in > practice, I don't think you need to worry about getting redundant signals. > If it becomes a real problem, we can think about have some props for apps to > suppress this behavior. I've been testing this with the two network connections I have in my apartment. The IP geolocation service we currently use puts me in slightly different locations, 11.5 km apart. This is well below the threshold I've set but I'm still getting the LocationUpdated signals, because the description changes. Debug prints from gclue_locator_update_location: old description: Landvetter, Västra Götaland, Sweden new description: Mölndal, Västra Götaland, Sweden distance: 11.516149 km threshold: 15.000000 km This might be anecdotal evidence, but it's what led me to hacking on the gclue-locator code -- no matter how high I set the threshold, I was still getting LocationUpdated signals when switching from one wifi network to another.
(In reply to comment #25) > (In reply to comment #22) > > Comment on attachment 85521 [details] [review] [review] [review] > > locator: Move distance threshold comparison earlier > > > > Review of attachment 85521 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > I thought we agreed that changes in description or accuracy are take as > > change in location. Since these changes wont really be happening in > > practice, I don't think you need to worry about getting redundant signals. > > If it becomes a real problem, we can think about have some props for apps to > > suppress this behavior. > > I've been testing this with the two network connections I have in my > apartment. The IP geolocation service we currently use puts me in slightly > different locations, 11.5 km apart. This is well below the threshold I've > set but I'm still getting the LocationUpdated signals, because the > description changes. > > Debug prints from gclue_locator_update_location: > > old description: Landvetter, Västra Götaland, Sweden > new description: Mölndal, Västra Götaland, Sweden > distance: 11.516149 km > threshold: 15.000000 km > > This might be anecdotal evidence, but it's what led me to hacking on the > gclue-locator code -- no matter how high I set the threshold, I was still > getting LocationUpdated signals when switching from one wifi network to > another. Ah, good point. I was only thinking of description changing and other props essentially not changing case. I think your change is good then. Maybe if description or other props change, we should update our existing location object and ensure that these changes also get synced with ServiceLocation so if clients are interested, they can listen to these changes?
Ah yes, sounds like a good plan: always update the location when any of the properties changes, but inhibit the LocationUpdated signal while the distance is below the threshold. I'd need to save the original coordinates somewhere to implement this. What would be a good place, locator->priv->original_location maybe?
Created attachment 85589 [details] [review] locator: Fix units in distance threshold comparison Make sure we don't compare kilometers to meters.
Created attachment 85590 [details] [review] locator: Keep track of last notified location ... and emit the LocationUpdated signal when we cross the distance threshold compared to the previously notified location.
(In reply to comment #27) > Ah yes, sounds like a good plan: always update the location when any of the > properties changes, but inhibit the LocationUpdated signal while the > distance is below the threshold. > > I'd need to save the original coordinates somewhere to implement this. What > would be a good place, locator->priv->original_location maybe? What i meant was that locator-priv->location is kept and its props are updated. ServiceLocation should keep a ref on the LocationInfo it gets and keep its props sync to the LocationInfo props.
Comment on attachment 85589 [details] [review] locator: Fix units in distance threshold comparison Review of attachment 85589 [details] [review]: ----------------------------------------------------------------- ACK
Comment on attachment 85590 [details] [review] locator: Keep track of last notified location Review of attachment 85590 [details] [review]: ----------------------------------------------------------------- NACK. We don't need to do this as I pointed out in second last comment.
Created attachment 85677 [details] [review] service-location: Use property binding to keep locations in sync Instead of copying values over from location_info to service_location, just keep a ref to location_info and let gobject property binding keep the properties in sync.
Created attachment 85678 [details] [review] location-info: Allow setting properties after construct time
Created attachment 85679 [details] [review] location-info: Don't warn for NULL description set_description with NULL is a valid way for clearing the value.
Created attachment 85680 [details] [review] locator: Keep track of last notified location ... and only emit the LocationUpdated signal when we cross the distance threshold compared to the previously notified location.
Comment on attachment 85677 [details] [review] service-location: Use property binding to keep locations in sync Review of attachment 85677 [details] [review]: ----------------------------------------------------------------- Cool..err.. I meant ACK :)
Comment on attachment 85678 [details] [review] location-info: Allow setting properties after construct time Review of attachment 85678 [details] [review]: ----------------------------------------------------------------- ACK
Comment on attachment 85679 [details] [review] location-info: Don't warn for NULL description Review of attachment 85679 [details] [review]: ----------------------------------------------------------------- ack
Comment on attachment 85680 [details] [review] locator: Keep track of last notified location Review of attachment 85680 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-locator.c @@ -169,5 @@ > - > - threshold_km = priv->threshold / 1000.0; > - distance = gclue_location_info_get_distance_from (priv->location, > - location); > - if (gclue_location_info_equal (priv->location, location) && Seems like you are removing your own changes that are not yet merged, in this patch. You want to 'rebase -i' and remove those changes. Also that would mean this patch does more than what it says, i-e this is then going to be the patch that drops 'send LocationUpdate for any location property change' behavior. Although that change doesn't need to be in a separate patch but the commit log will need update. @@ +176,5 @@ > + "latitude", gclue_location_info_get_latitude (new_location), > + "longitude", gclue_location_info_get_longitude (new_location), > + "accuracy", gclue_location_info_get_accuracy (new_location), > + "description", gclue_location_info_get_description (new_location), > + NULL); Seems you keep the original location forever and always update its props. This would mean that if new location goes beyond threshold, apps will get notifications for prop changes on existing location and then a "LocationChanged" as well. I don't think we want that. Also since ServiceLocation keeps refs to associated location instance and (at the time of location update) have two ServiceLocation instances, we're probably going to get into trouble with keeping same location instance.
(In reply to comment #40) > Seems like you are removing your own changes that are not yet merged, in > this patch. You want to 'rebase -i' and remove those changes. Also that > would mean this patch does more than what it says, i-e this is then going to > be the patch that drops 'send LocationUpdate for any location property > change' behavior. Although that change doesn't need to be in a separate > patch but the commit log will need update. Fixed. I'll post the rebased patches here and push to the github repo as well so it's easy for you to merge in the right order if they look ok. > Seems you keep the original location forever and always update its props. > This would mean that if new location goes beyond threshold, apps will get > notifications for prop changes on existing location and then a > "LocationChanged" as well. I don't think we want that. Not so sure about that, but we can fine tune this once there's some client that's actually interested to listening to the prop changes. Right now I don't think there is any. > Also since ServiceLocation keeps refs to associated location instance and > (at the time of location update) have two ServiceLocation instances, we're > probably going to get into trouble with keeping same location instance. Ah yes, when we have: <signal name="LocationUpdated"> <arg name="old" type="o"/> <arg name="new" type="o"/> </signal> ... we need to make sure the "old" and "new" are actually different. Fixed.
Created attachment 85683 [details] [review] locator: Add a local priv variable
Created attachment 85684 [details] [review] locator: Fix units in distance threshold comparison Make sure we don't compare kilometers to meters.
Created attachment 85685 [details] [review] locator: Rework the location updating logic Previously, we would emit the LocationUpdated signal for any location property change, disregarding the DistanceThreshold value. A change in the "description" property would trigger the LocationUpdated signal, even though we were well below the threshold. This commit changes it to emit LocationUpdated only when we cross the threshold. We keep track of the previous location where LocationUpdated was last emitted. While below the threshold, we update the location properties, but don't notify the clients; the clients only get notified once the threshold is crossed.
Created attachment 85708 [details] [review] locator: Rework the location updating logic Previously, we would emit the LocationUpdated signal for any location property change, disregarding the DistanceThreshold value. A change in the "description" property would trigger the LocationUpdated signal, even though we were well below the threshold. This commit changes it to emit LocationUpdated only when we cross the threshold. We keep track of the previous location where LocationUpdated was last emitted. While below the threshold, we update the location properties, but don't notify the clients; the clients only get notified once the threshold is crossed.
(In reply to comment #41) > (In reply to comment #40) > > Seems like you are removing your own changes that are not yet merged, in > > this patch. You want to 'rebase -i' and remove those changes. Also that > > would mean this patch does more than what it says, i-e this is then going to > > be the patch that drops 'send LocationUpdate for any location property > > change' behavior. Although that change doesn't need to be in a separate > > patch but the commit log will need update. > > Fixed. I'll post the rebased patches here and push to the github repo as > well so it's easy for you to merge in the right order if they look ok. > > > > Seems you keep the original location forever and always update its props. > > This would mean that if new location goes beyond threshold, apps will get > > notifications for prop changes on existing location and then a > > "LocationChanged" as well. I don't think we want that. > > Not so sure about that, but we can fine tune this once there's some client > that's actually interested to listening to the prop changes. Right now I > don't think there is any. Nevertheless there will be dbus signals emitted for no reason then so doesn't matter if any client listens to them or not.
Comment on attachment 85683 [details] [review] locator: Add a local priv variable Review of attachment 85683 [details] [review]: ----------------------------------------------------------------- ack
Comment on attachment 85684 [details] [review] locator: Fix units in distance threshold comparison Review of attachment 85684 [details] [review]: ----------------------------------------------------------------- ack ::: src/gclue-locator.c @@ +161,5 @@ > GClueLocationInfo *location) > { > GClueLocatorPrivate *priv = locator->priv; > + gdouble accuracy, new_accuracy; > + gdouble distance; dont know why you are moving these on separate lines but no biggie.
Comment on attachment 85708 [details] [review] locator: Rework the location updating logic Review of attachment 85708 [details] [review]: ----------------------------------------------------------------- Almost there! Pretty sure next iteration can go as is :) ::: src/gclue-locator.c @@ +170,1 @@ > g_debug ("Updating location"); This debug was supposed to be for the case when we actually update location. I guess its not needed anymore. @@ +170,4 @@ > g_debug ("Updating location"); > + > + if (priv->last_notified_location != NULL) { > + threshold_km = priv->threshold / 1000.0; you might want to move threshold_km declaration in this block? @@ +178,5 @@ > + } > + > + if (notify_location || priv->location == NULL) { > + g_clear_object (&priv->location); > + priv->location = g_object_new (GCLUE_TYPE_LOCATION_INFO, NULL); why not just take a ref to new_location instead and then not set the props for this case? I think that would make more readable code.
(In reply to comment #48) > dont know why you are moving these on separate lines but no biggie. Ah thanks, that went into wrong commit with the rebase. Fixed.
(In reply to comment #49) > > g_debug ("Updating location"); > > This debug was supposed to be for the case when we actually update location. > I guess its not needed anymore. Fixed. > > + if (priv->last_notified_location != NULL) { > > + threshold_km = priv->threshold / 1000.0; > > you might want to move threshold_km declaration in this block? Sure. > > @@ +178,5 @@ > > + } > > + > > + if (notify_location || priv->location == NULL) { > > + g_clear_object (&priv->location); > > + priv->location = g_object_new (GCLUE_TYPE_LOCATION_INFO, NULL); > > why not just take a ref to new_location instead and then not set the props > for this case? I think that would make more readable code. Yes, at first I was going to do exactly what you are suggesting, but then I went with the current approach for two reasons: - In subsequent calls we're going to keep the same priv->location and modify its props. To me it seems like a break in the ownership semantics if we grab a ref on the object passed in, and modify it beyond recognition. - Besides priv->location, we also need to keep priv->last_notified_location around. These two can't share the same object because priv->location's props are going to be updated in subsequent calls, but last_notified_location needs to stay the same -- updating location shouldn't mess up last_notified_location. What I'm doing here here is taking ref to new_location and saving it on last_notified_location instead. ----- Honestly, this is all getting a bit too complicated. I would rather change the API for 3.12 and get rid of the "old" and "new" location objects on the signal. We could have a dataless signal that just says "Hi Mr. GeoClue user, I've now updated the props on the Location object, so please feel free to go ahead and read them". I don't really see the need for having the location data in three different places: (1) the Location object, (2) LocationUpdated.old, (3) LocationUpdated.new, it just makes the implementation complicated.
Created attachment 85745 [details] [review] locator: Fix units in distance threshold comparison Make sure we don't compare kilometers to meters.
Created attachment 85746 [details] [review] locator: Rework the location updating logic Previously, we would emit the LocationUpdated signal for any location property change, disregarding the DistanceThreshold value. A change in the "description" property would trigger the LocationUpdated signal, even though we were well below the threshold. This commit changes it to emit LocationUpdated only when we cross the threshold. We keep track of the previous location where LocationUpdated was last emitted. While below the threshold, we update the location properties, but don't notify the clients; the clients only get notified once the threshold is crossed.
(In reply to comment #51) > (In reply to comment #49) > > > g_debug ("Updating location"); > > > > This debug was supposed to be for the case when we actually update location. > > I guess its not needed anymore. > > Fixed. > > > > > + if (priv->last_notified_location != NULL) { > > > + threshold_km = priv->threshold / 1000.0; > > > > you might want to move threshold_km declaration in this block? > > Sure. > > > > > > @@ +178,5 @@ > > > + } > > > + > > > + if (notify_location || priv->location == NULL) { > > > + g_clear_object (&priv->location); > > > + priv->location = g_object_new (GCLUE_TYPE_LOCATION_INFO, NULL); > > > > why not just take a ref to new_location instead and then not set the props > > for this case? I think that would make more readable code. > > Yes, at first I was going to do exactly what you are suggesting, but then I > went with the current approach for two reasons: > > - In subsequent calls we're going to keep the same priv->location and > modify its props. To me it seems like a break in the ownership semantics if > we grab a ref on the object passed in, and modify it beyond recognition. That is fine, ipclient returns 'transfer full' so we can do anything to the object we want after we have it in locator. > - Besides priv->location, we also need to keep priv->last_notified_location > around. These two can't share the same object because priv->location's props > are going to be updated in subsequent calls, but last_notified_location > needs to stay the same -- updating location shouldn't mess up > last_notified_location. What I'm doing here here is taking ref to > new_location and saving it on last_notified_location instead. Now that you mention it, it seems like we really don't need to keep the whole last_notified_location but total distance traveled. We can keep a 'gdouble distance_traveled' private field and we do: priv->distance_traveled += gclue_location_info_get_distance_from(priv->loc, new_loc); each time there is a new location available. Once it goes beyond threshold, we reset it to zero. > ----- > > Honestly, this is all getting a bit too complicated. I would rather change > the API for 3.12 and get rid of the "old" and "new" location objects on the > signal. I don't see how that will help as long as we have this threshold handling. > We could have a dataless signal that just says "Hi Mr. GeoClue user, I've > now updated the props on the Location object, so please feel free to go > ahead and read them". I don't really see the need for having the location > data in three different places: (1) the Location object, (2) > LocationUpdated.old, (3) LocationUpdated.new, it just makes the > implementation complicated. What I suggested above will rid of one location instance already. :) (In reply to comment #51) > (In reply to comment #49) > > > g_debug ("Updating location"); > > > > This debug was supposed to be for the case when we actually update location. > > I guess its not needed anymore. > > Fixed. > > > > > + if (priv->last_notified_location != NULL) { > > > + threshold_km = priv->threshold / 1000.0; > > > > you might want to move threshold_km declaration in this block? > > Sure. > > > > > > @@ +178,5 @@ > > > + } > > > + > > > + if (notify_location || priv->location == NULL) { > > > + g_clear_object (&priv->location); > > > + priv->location = g_object_new (GCLUE_TYPE_LOCATION_INFO, NULL); > > > > why not just take a ref to new_location instead and then not set the props > > for this case? I think that would make more readable code. > > Yes, at first I was going to do exactly what you are suggesting, but then I > went with the current approach for two reasons: > > - In subsequent calls we're going to keep the same priv->location and > modify its props. To me it seems like a break in the ownership semantics if > we grab a ref on the object passed in, and modify it beyond recognition. > > - Besides priv->location, we also need to keep priv->last_notified_location > around. These two can't share the same object because priv->location's props > are going to be updated in subsequent calls, but last_notified_location > needs to stay the same -- updating location shouldn't mess up > last_notified_location. What I'm doing here here is taking ref to > new_location and saving it on last_notified_location instead. > > ----- > > Honestly, this is all getting a bit too complicated. I would rather change > the API for 3.12 and get rid of the "old" and "new" location objects on the > signal. > > We could have a dataless signal that just says "Hi Mr. GeoClue user, I've > now updated the props on the Location object, so please feel free to go > ahead and read them". I don't really see the need for having the location > data in three different places: (1) the Location object, (2) > LocationUpdated.old, (3) LocationUpdated.new, it just makes the > implementation complicated.
I pushed all patches but one.
Created attachment 85876 [details] [review] locator: Keep track of the distance travelled Previously, we would emit the LocationUpdated signal for any location property change, disregarding the DistanceThreshold value. A change in the "description" property would trigger the LocationUpdated signal, even if we were well below the threshold. This commit changes it to emit LocationUpdated only when we cross the threshold. We keep track of the distance travelled and while it's below the distance threshold, we update the location properties, but don't emit the signal.
Created attachment 85877 [details] [review] service-client: Use free instead of g_clear_pointer A small unrelated cleanup that's a leftover from yesterday's approach to move threshold tracking to service-client.
Pushed your alternative patches from your github branch. Unfortunately the patches got the wrong URL (to bug#69105 on gnome bz) due to git-bz defaulting to gnome bz and me forgetting about that. :(
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.