Created attachment 271134 [details] [review]
This supports better version of existing find_nearest_city method.

I am a bit confused by the patch: I think you are mixing different issues

1) add an async api for a potentially slow operation
2) integrate geolocation support in libgweather itself
3) add a way to customize what "nearest city" means (e.g. restrict to the same nation)

I think:

1) async is a worthwhile goal, but it should not silently introduce side effects like initializing geolocation and filtering to the same country

2) For now I'd keep that in the caller side

3) I think the api should allow to specify how to filter the locations from the caller, because I do not think that hardcoding the logic in libgweather is flexible enough. For instance in Clocks we need to pick the nearest city which is in the same country *and* in the same timezone.

I think we should add an api which is

find_nearest_city_full(latitude, longitude, FilterFunc filter)

where filter is a function pointer that allows the caller to apply a logic to match the desired locations.

(In reply to comment #2)
> I am a bit confused by the patch: I think you are mixing different issues
> 
> 1) add an async api for a potentially slow operation
> 2) integrate geolocation support in libgweather itself
> 3) add a way to customize what "nearest city" means (e.g. restrict to the same
> nation)
> 
> I think:
> 
> 1) async is a worthwhile goal, but it should not silently introduce side
> effects like initializing geolocation and filtering to the same country

async() only makes sense in concert with geolocation (geocoding, really) - the rest of the code, which is also the slow part unfortunately, is sync, and doing at mainloop will not help. We could run in a thread, but I prefer to see a better data structure, instead of papering over it with multi-threading.

> 2) For now I'd keep that in the caller side

A find_nearest_city() that does not do filter for country or TZ-ID does not look very useful to me.
Plus, in the future, we will integrate geolocation more in libgweather, with the goal of making it transparent to the applications. Well, with gnome-weather's goals in mind, but I believe it will help gnome-clocks and gnome-initial-setup too.

> 3) I think the api should allow to specify how to filter the locations from the
> caller, because I do not think that hardcoding the logic in libgweather is
> flexible enough. For instance in Clocks we need to pick the nearest city which
> is in the same country *and* in the same timezone.
> 
> I think we should add an api which is
> 
> find_nearest_city_full(latitude, longitude, FilterFunc filter)
> 
> where filter is a function pointer that allows the caller to apply a logic to
> match the desired locations.

The same for gnome-weather: same country and same timezone.

This said, I'm not opposed to a find_nearest_city_async_full(), but the common case should be working out of the box with find_nearest_city_async().
(find_nearest_city() would stay for backward compatibility, because gnome-initial-setup uses it, afaik, but it would be deprecated)


Btw, Saurabh, the correct way to make bindable function pointers in gobject is

function (args..., FunctionType callback, gpointer user_data, GDestroyNotify destroy, ...)

You must call callback with user_data as the last argument (so FunctionType must accept a void* in addition to the useful arguments) and you must call destroy(user_data) when you're done with callback.

A simple function(args..., FunctionType callback) should not work, and I'm surprised gobject-introspection doesn't warn you.

(In reply to comment #3)
> async() only makes sense in concert with geolocation (geocoding, really) - the
> rest of the code, which is also the slow part unfortunately, is sync, and doing
> at mainloop will not help. We could run in a thread, but I prefer to see a
> better data structure, instead of papering over it with multi-threading.
> 

What I mean is that we should not have:

libfoo_do_stuff -> returns A
libfoo_do_stuff_async -> returns B and has side effects

since that is very confusing for the api user. If async only makes sense for geocoding etc, then let's add a new function called libgweather_detect_nearest_location or something like that, which is always async and does geocoding, but let's not call it find_nearest_async


> > 2) For now I'd keep that in the caller side
> 
> A find_nearest_city() that does not do filter for country or TZ-ID does not
> look very useful to me.
> Plus, in the future, we will integrate geolocation more in libgweather, with
> the goal of making it transparent to the applications. Well, with
> gnome-weather's goals in mind, but I believe it will help gnome-clocks and
> gnome-initial-setup too.
> 

I do not know... I agree that the "simple" version of the function should be useful by default, but I also think that some flexibility in how the location database is filtered is useful. For instance we could in the future we could change the clocks design to just return capital cities, or maybe a weather app prefers to show the weather of the nearest weather station even if it is another country or timezone, or a app for public transportation could be interested in filtering cities that only have train stations and so on. I think it would be good to keep the policy out of the API.

Uhm, I agree on the API confusion, and I like the "detect_nearest_location()" name.

OTOH, if we have a filter function, then we don't need to invoke geocode-glib at all, we can do that application side...

And finally, in the context of geolocation (but for other uses cases), find_nearest_location() is just wrong. We want to take as much as data as possible from the app (in particular we should take coordinates from geolocation, not from the database; maybe the name too)...

Created attachment 271472 [details] [review]
Corrected patch

This supports two versions of find_nearest_method as discussed above. One with assumption that geocoding and filter_func is implemented on the app side and the other one with geocoding and filter_func (country level restriction) in the libgweather itself.

Review of attachment 271472 [details] [review]:

There's some funkyness in the indentation...

::: libgweather/gweather-location.c
@@ +595,3 @@
+ *                                       and false to filter the location out
+ * @user_data: user_data for customization
+ * @destroy: (scope async): method to destroy user_data

Don't need this, it's a GDestroyNotify and g-i knows what to do.

@@ +617,3 @@
+                                    GWeatherFilterFunc func,
+                                    gpointer user_data,
+                                    GDestroyNotify destroy)

*Don't* modify existing API, add new calls!

@@ +733,3 @@
+ * @error: Store error if any occurs in retrieving the result
+ *
+ * Returns: Customized GWeatherLocation

(transfer full)

::: libgweather/gweather-location.h
@@ +28,3 @@
 #include <glib.h>
+#include <gtk/gtk.h>
+#include <geocode-glib/geocode-glib.h>

Don't include external headers in public headers, we don't depend on geocode-glib at the API layer.

Created attachment 271490 [details] [review]
Corrected patch

Added new api named find_nearest_city_full.

Review of attachment 271490 [details] [review]:

Some minor programming issues, and then this is OK for me, but I'd like Paolo to review the new API too.

::: libgweather/gweather-location.c
@@ +541,3 @@
+
+    if (loc->level == GWEATHER_LOCATION_WEATHER_STATION) {
+        callback (loc, user_data);

You're changing the behavior of find_existing_city(), this should be at least documented.
Not to mention you're not actually returning a city...

@@ +725,3 @@
+        location = _gweather_location_new_detached(data.location, geocode_place_get_town (place), TRUE, data.latitude, data.longitude);
+
+        g_simple_async_result_set_op_res_gpointer (simple, gweather_location_ref (location), (GDestroyNotify)gweather_location_unref);

Don't need a ref here, location is already owned because it was newly constructed.

@@ +768,3 @@
+    simple = g_simple_async_result_new (NULL, callback, user_data, gweather_location_detect_nearest_city);
+
+    data = g_slice_new0 (ArgData);

You're not freeing this structure, are you?

@@ +771,3 @@
+    data->latitude = lat;
+    data->longitude = lon;
+    data->location = loc;

Not necessary now, because GWeatherLocations are static, but a ref here would not hurt.

Created attachment 271541 [details] [review]
Slightly corrected patch as suggested above

Review of attachment 271541 [details] [review]:

Ideally this patch would be split in two commits: one adding find_nearest_full and one adding detect_nearest

The find_nearest_full part looks mostly ok to me except for a few nitpicks (see below)

For the detect_nearest part, I have two comments:
1) I think the API should take a cancellable (see below).
2) I am also not sure whether the api should be even more complicated and take a filter func too, or if we should add a detect_full variant or if we should ignore the problem and if someone wants a filtered version he can do its own reverse geocoding. I'd be leaning toward the first option, making the API:

detect_nearest(GWeatherLocation *loc,
               double lat,
               double lon,
               GCancellable cancellable,
               GAsyncReadyCallback callback,
               gpointer user_data,
               GDestroyNotify destroy)

which is a bit scary, but not *too* bad especially considering it is a method you would use just once in an app and that one can pass NULL for most of the args

::: libgweather/gweather-location.c
@@ -525,2 +529,2 @@
 {
-    if (loc->level == GWEATHER_LOCATION_CITY) {
+    if (country_code) {

note, I am just glancing at this so I may be missing something, but why do we need the country code here?
As far as I can see this is just used for the "detect" case, but it could instead just pass data and a filter function instead of doing a special case.

nitpick: I'd call the function foreach_city not cities :-)

@@ -616,0 +643,30 @@
+ * gweather_location_find_nearest_city_full:
+ * @loc: (allow-none): The parent location, which will be searched recursively
+ * @lat: Latitude, in degrees
... 27 more ...

nitpick: either align all the args or none of them

@@ -616,0 +643,37 @@
+ * gweather_location_find_nearest_city_full:
+ * @loc: (allow-none): The parent location, which will be searched recursively
+ * @lat: Latitude, in degrees
... 34 more ...

nitpick: we usually typedef structs

@@ -616,0 +643,60 @@
+ * gweather_location_find_nearest_city_full:
+ * @loc: (allow-none): The parent location, which will be searched recursively
+ * @lat: Latitude, in degrees
... 57 more ...

nitick: void should be on the same line of static. Also no need to prefix with underscore

@@ -616,0 +643,99 @@
+ * gweather_location_find_nearest_city_full:
+ * @loc: (allow-none): The parent location, which will be searched recursively
+ * @lat: Latitude, in degrees
... 96 more ...

not sure if specifying a parent is useful when geocoding, I guess we would always want to use world here, no?

@@ +755,3 @@
+ */
+void
+}

This API should take cancellable and pass it down to the geocode-glib operation, it is important all async functions are cancellable

@@ -616,0 +643,128 @@
+ * gweather_location_find_nearest_city_full:
+ * @loc: (allow-none): The parent location, which will be searched recursively
+ * @lat: Latitude, in degrees
... 125 more ...

nitpick: declare variable at the start of the block

@@ -616,0 +643,145 @@
+ * gweather_location_find_nearest_city_full:
+ * @loc: (allow-none): The parent location, which will be searched recursively
+ * @lat: Latitude, in degrees
... 142 more ...

do not mention the internal _got_place function in the public API docs

(In reply to comment #11)

> detect_nearest(GWeatherLocation *loc,
>                double lat,
>                double lon,
>                GCancellable cancellable,
>                GAsyncReadyCallback callback,
>                gpointer user_data,
>                GDestroyNotify destroy)
> 

I obviously missed filter_func here

Created attachment 271635 [details] [review]
New api with the support of geocoding in libgweather

This patch is created on top of the previous patch that I submitted on the bug :
https://bugzilla.gnome.org/show_bug.cgi?id=726186

Review of attachment 271635 [details] [review]:

Still has problems on indentation and documentation comments.

Review of attachment 271635 [details] [review]:

It also does not address my api comments: the issue with cancellable and the issue with the filter func. It's up to Giovanni decide if we want the filter func or no, but for the cancellable I think we definitely need it.

(In reply to comment #15)
> Review of attachment 271635 [details] [review]:
> 
> It also does not address my api comments: the issue with cancellable and the
> issue with the filter func. It's up to Giovanni decide if we want the filter
> func or no, but for the cancellable I think we definitely need it.

We went through the filter func in IRC, the conclusion was that it would not have enough information (because it doesn't see the results of geocoding), so we will skip it for now.
This is 3.14 material anyway, there's some time to iterate on the API before the freeze in August.

I agree on the cancellable though.

Created attachment 271648 [details] [review]
support if new api with GCancellable and tried to correct indentation and doc comments

Created attachment 271656 [details] [review]
corrected.

Review of attachment 271656 [details] [review]:

::: libgweather/gweather-location.c
@@ +705,3 @@
+        g_message ("Failed to get geocode: %s", error->message);
+        g_error_free (error);
+        exit (1);

Uh oh, I just noticed this, very bad!

Additionally, you need to check if the error is G_IO_ERROR/G_IO_ERROR_CANCELLED, and skip the message in that case. Or even better, just propagate the error from geocode-glib to GSimpleAsyncResult and let the caller deal with that.

Plus, be careful that you still need to free user_data, even if you take an early return.

@@ +782,3 @@
+    data->simple = simple;
+
+    geocode_reverse_resolve_async (reverse, cancellable, _got_place, data);

Passing the cancellable here is not enough, you must also call g_simple_async_result_set_check_cancellable() right after where you create the GSimpleAsyncResult

Created attachment 271663 [details] [review]
Modified patch.

Review of attachment 271663 [details] [review]:

Yep.

Pushed.