Bug 89719 - Provides GPS location before less accurate locations
Summary: Provides GPS location before less accurate locations
Status: RESOLVED FIXED
Alias: None
Product: GeoClue
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Geoclue Bugs
QA Contact: Geoclue Bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-22 18:57 UTC by Fabrice Bellet
Modified: 2015-03-31 23:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
locator: Activate the sources in reverse order (1.37 KB, patch)
2015-03-22 18:57 UTC, Fabrice Bellet
Details | Splinter Review
locator: start the sources in reverse order (1.94 KB, patch)
2015-03-27 08:58 UTC, Fabrice Bellet
Details | Splinter Review
locator: Sort the sources based on their accuracy level (2.82 KB, patch)
2015-03-31 19:10 UTC, Fabrice Bellet
Details | Splinter Review

Description Fabrice Bellet 2015-03-22 18:57:45 UTC
Created attachment 114533 [details] [review]
locator: Activate the sources in reverse order

This gives modem-gps source a chance to provide a gps location faster
than other sources, if ModemManager already has a gps fix for it.  It
happens for example, if gps nmea collection has been started manually by
mmcli before the launch of geoclue.

This is useful for example with applications that do a single-shot geolocation
request, and will only use the first answer provided by geoclue. So IMO geoclue should try if possible to report the most accurate location first.
Comment 1 Zeeshan Ali 2015-03-23 18:36:24 UTC
Comment on attachment 114533 [details] [review]
locator: Activate the sources in reverse order

Review of attachment 114533 [details] [review]:
-----------------------------------------------------------------

Some nits:

* mode-gps -> ModemGPS.
* gps nmea -> GPS NMEA.

::: src/gclue-locator.c
@@ +368,5 @@
>                          continue;
>                  }
>  
> +                locator->priv->active_sources = g_list_prepend (locator->priv->active_sources,
> +                                                                src);

* This wouldn't be obvious to everyone so a comment would be nice.
* Instead of complicating code here with introduction of a new loop, why not just replace g_list_append calls with g_list_prepend, or change the order of source initializations in constructed function?

@@ -368,5 @@
>                          continue;
>                  }
>  
> -                locator->priv->active_sources = g_list_append (locator->priv->active_sources,
> -                                                               src);

ooh, seems this line goes beyond 80 chars and thats our limit. Can you please fix that first in a separate patch that comes before this one?

@@ +371,5 @@
> +                locator->priv->active_sources = g_list_prepend (locator->priv->active_sources,
> +                                                                src);
> +        }
> +
> +        for (node = locator->priv->active_sources; node != NULL; node = node->next) {

Does this line fit under 80 char?
Comment 2 Fabrice Bellet 2015-03-27 08:58:51 UTC
Created attachment 114662 [details] [review]
locator: start the sources in reverse order

Sure, it's better to directly change the list construction order. This new patch exactly does that. As the side effect, this modification masks the bug 89782, because starting a new client in this case doesn't generate the three "New location available" messages at source startup, each one refining the location of the previous one.
Comment 3 Zeeshan Ali 2015-03-28 14:12:40 UTC
Comment on attachment 114662 [details] [review]
locator: start the sources in reverse order

Review of attachment 114662 [details] [review]:
-----------------------------------------------------------------

Nitpicks on log:

* Reversing the order isn't the key here but rather that they are listed and therefore started in the order of most accurate to least accurate so i'll write "locator: Most accurate source first".

* Adding 'now' after 'We' would make it clear that we do this after this patch and its not a statement about existing code.

::: src/gclue-locator.c
@@ +268,1 @@
>  

1. You want a comment here that says why we prepend rather than append.

2. Sorry that I didn't think of this before but I think I have an even better/more generic solution. Instead of relying on order of append/prepend operations, why not just add a g_list_sort call at the end here? That way if another source is added in future, the author won't have to care about where to initialize/add it. The GCompareFunc will have to compare sources by their accuracy level.

Also the accuracy level changes dynamically (e.g when you insert/remove modem) so we want to re-sort the list each time a source changes its accuracy level.
Comment 4 Fabrice Bellet 2015-03-31 19:10:23 UTC
Created attachment 114781 [details] [review]
locator: Sort the sources based on their accuracy level

This patch sorts the list of sources according to comment #3. Thanks for this idea! I put the sort in function refresh_available_accuracy_level(), which seems a good place, as this function is called also when sources disappear and new sources appear. It makes the code of this function more simple, as we no longer have to iterate over the list to find the highest accuracy level.
Comment 5 Zeeshan Ali 2015-03-31 23:28:22 UTC
Comment on attachment 114781 [details] [review]
locator: Sort the sources based on their accuracy level

Fixed the patch with a few changes. Thanks!
Comment 6 Zeeshan Ali 2015-03-31 23:28:49 UTC
(In reply to Zeeshan Ali from comment #5)
> Comment on attachment 114781 [details] [review] [review]
> locator: Sort the sources based on their accuracy level
> 
> Fixed the patch with a few changes. Thanks!

Fixed -> Pushed.


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.