*** Bug 530179 has been marked as a duplicate of this bug. ***

*** Bug 530180 has been marked as a duplicate of this bug. ***

It looks like weather.com lets you enter any known city name, and then it just automatically picks the closest weather station to that city. In the case of Manizales, it seems to be using the weather data from Pereira:

http://www.weather.com/outlook/travel/businesstraveler/local/COXX0019
http://www.weather.com/outlook/travel/businesstraveler/local/COXX0024

They differ only in sunrise/sunset times (which it presumably can figure out automatically based on the difference in longitude, latitude, and altitude?)

Would they offer an api like the national weather service? 

no

and I didn't actually mean to link to this bug from yours, I meant to link to bug 562141 :-). (Or maybe both of them. That one is "figure out where you are without forcing you to say so explicitly" and this "figure out the closest weather station, given that we know where you are".)

Why not use GeoNames?

They provide XML/JSON APIs for everything that libgweather currently does (and more).

(In reply to comment #6)
> Why not use GeoNames?
> 
> They provide XML/JSON APIs for everything that libgweather currently does (and
> more).

That's a really good idea.. One more vote for the www.geonames.org db!

(In reply to comment #6)
> Why not use GeoNames?
> 
> They provide XML/JSON APIs for everything that libgweather currently does (and
> more).

I'm all for having more locations, my only issue is if the content is of decent quality. I mean, a lookup of my city brings up many results that all point to the same place instead of one result with all that information in it.

You can tell geonames you only want cities for matches, not "every database item that contains this string". http://live.gnome.org/LibGWeather/UseGeoNames gives an example of a query that does that.

While looking at the GeoNames site I stumbled across a URL where you submit the query based on latitude and longitude and get a bunch of stuff.  Boston MA, for example:  http://ws.geonames.org/findNearByWeatherXML?lat=42.35&lng=-71.06

If we combine this with another query that estimates current lat/lon that avoids the need for any user input altogether.

I think we're a bit late to make this change for 2.30 given the UI impact but 3.0 certainly seems feasible.

*** Bug 606899 has been marked as a duplicate of this bug. ***

Created attachment 181545 [details] [review]
Queries geonames.org for user specified location

Comment on attachment 181545 [details] [review]
Queries geonames.org for user specified location

OK...

>+/* gweather-geonames.c - Location-handling code
>+ *
>+ * Copyright 2011, Red Hat, Inc.

You probably didn't mean that :)

>+    soup_message_set_flags (msg, SOUP_MESSAGE_NO_REDIRECT);
>+    soup_session_send_message (session, msg);
>+
>+    if (SOUP_STATUS_IS_REDIRECTION(msg->status_code)) {

Did you copy this from somewhere else? You're telling libsoup to not handle redirects automatically, and then you're handling them yourself manually. Except that you're leaving the body of the redirect message in msg, so gweather_geonames_location_query() is going to see that, instead of what you fetched into msg2... Just remove the set_flags call and this whole if(). It's unnecessary.

Also, you really shouldn't use soup_session_send_message() here. If the network request takes a long time (either because geonames.org is busy, or just due to a temporary network glitch), this could block the caller for an unknown length of time.

The right way to do this is to use soup_session_queue_message(), and then do the processing from the callback you pass to that. (So you'd have the entry pass a callback to gweather_geonames_location_query() as well, which it would call after doing it's processing after the callback from soup_session_queue_message().)

>+    language = getenv("GDM_LANG");
>+    if (!language) language = getenv("LANG");
>+    if (!language) language = (char *)langdflt;

call g_get_language_names(), and use the first language in the returned array. Note that you may have to convert POSIX-style "en_US" into internet-style "en-us" (or maybe even just "en", depending on what geonames's API supports).

>+    parser = g_slice_new0 (GWeatherParser);

You shouldn't duplicate this code outside of parser.c. The exact details of how a GWeatherParser are constructed and freed are private.

But anyway, it looks like you're basically only using parser->xml anyway, so you don't even need a GWeatherParser. Just use a plain xmlTextReaderPtr.

>+static GWeatherLocation *
>+location_new_from_geocities (GWeatherParser *parser,

geoNAMES, not geocities :-)

>+    loc = g_slice_new0 (GWeatherLocation);

as above, you shouldn't be constructing GWeatherLocations on your own. The simplest thing here might be to move this function into gweather-location.c and call it g_weather_location_new_from_geonames() or something. (And then you don't need to move struct _GWeatherLocation into a different file either.)

>+	    loc->name = g_strdup (gweather_parser_get_localized_value (parser));

This won't work, because the geonames results aren't in the format that _get_localized_value() expects. (eg, the French translation of <name> is under <alternateName lang="fr">, not <name xml:lang="fr">)

>+	else if (!strcmp (tagname, "timezone") && !loc->tz_hint) {
>+	    loc->tz_hint = g_strdup (gweather_parser_get_value (parser->xml));
>+	    if (!loc->tz_hint)
>+		goto error_skip;
>+	}

Need to make sure that this does the right thing when this returns a timezone name that we consider "deprecated". I forget at what point we normalize the timezone names...

>+/*
>+ * Process the response from geonames.org and
>+ * add their matches to the GWeatherLocation tree
>+ */

It would be better to have this just return a GList of GWeatherLocation, and then have the entry add those locations to its completion list by itself.

>+void
>+geonames_start_open (WeatherInfo *info)

this is consistent with the other weather-lookup function names, but it's a little confusing since there are lots of other geonames methods too. It would probably be better as "geonames_start_weather_lookup" or something.

>+    do {
>+	mid = low + (high - low) / 2;
>+	loc = children[mid];

You should be able to just use bsearch() here.

>+    children = (GWeatherLocation**)g_try_realloc_n (parent->children,

in general, we don't bother with the _try_ alloc functions, unless you're allocating something VERY large.

>-gweather_parser_get_value (GWeatherParser *parser)
>+gweather_parser_get_value (xmlTextReaderPtr reader)

Should rename that to not have "gweather_parser" in the name if it's not a GWeatherParser method any more.

Created attachment 182010 [details] [review]
Revised patch

>>+/* gweather-geonames.c - Location-handling code
>>+ *
>>+ * Copyright 2011, Red Hat, Inc.
>
>You probably didn't mean that :)

Yeah, c&p; grabbed the title line and got the copyright one out of the source too.  Is there a correct assignment for this?


>>+/*
>>+ * Process the response from geonames.org and
>>+ * add their matches to the GWeatherLocation tree
>>+ */
>
>It would be better to have this just return a GList of GWeatherLocation, and
>then have the entry add those locations to its completion list by itself.

I thought about that but there was the country and state fields which would have to be GListed as well.  I can still switch it to that if you'd prefer though.

>>+    do {
>>+	mid = low + (high - low) / 2;
>>+	loc = children[mid];
>
>You should be able to just use bsearch() here.

This particular routine indicates where the insertion point is when the entry isn't found: two birds, one stone as it were.

There's one annoying quirk. When I was trying this out with the test_locations program I would enter the full name of a location that I know of the location wouldn't appear until I hit backspace.  I'm still looking into why it doesn't just appear. If I find the solution I'll attach it as another patch but didn't want to withhold it in the meantime.

Created attachment 182011 [details]
Revised patch

Corrected patch

Created attachment 182028 [details] [review]
Corrected patch (really!)

(still trying to get git...)

Comment on attachment 182028 [details] [review]
Corrected patch (really!)

>+CFLAGS += -g3 -O2

I think you probably didn't mean to commit that?

>+ * gweather_location_new_from_geonames:

>+ * Create a new #GWeatherLocation based on the contents of the next
>+ * <geoname> record found in a response from geonames.org

You have to say "&lt;geoname&gt;". (Well, I think currently we don't generate HTML docs anyway, but at some point we probably will, and gtk-doc requires you to escape xml chars.)

>+ * Return value: (allow-none) a %GWEATHER_LOCATION_CITY entry

need a ":" after "(allow-none)" or it won't be picked up.

>+	if (!strcmp (tagname, "name") && !loc->name) {

So it looks like you're not looking at the alternateNames at all? Oh, but geonames uses the language you passed in in the request to decide what language the <name> should be, right?

>+	}
>+	else if (!strcmp (tagname, "countryCode") && !loc->country_code) {

minor nit, but the style in the rest of the code is to put the else on the same line as the "}".

>+	    if (!value)
>+		goto error_skip;
>+	    dvalue = g_ascii_strtod (value, &p) * M_PI / 180.0;

            dvalue = DEGREES_TO_RADIANS (g_ascii_strtod (value, &p));

>+	    if (!*p)
>+		loc->latitude = dvalue;

a bit odd that if there's no value, we skip this entry, but if there's an unparsable value, we keep it, just with no latitude set...

>+    gweather_location_ref (loc);
>+    return loc;

you should set loc->ref_count to 1 at the start instead of calling _ref here...

>+    g_free (loc->name);
>+    g_free (loc->sort_name);
>+    g_free (loc->country_code);
>+    g_free (loc->tz_hint);
>+    g_slice_free (GWeatherLocation, loc);

...and then you can just use g_weather_location_unref() here instead of freeing it by hand

>+find_child_location (char              *name,

>+    g_return_val_if_fail (children, FALSE);

People usually always do an explicit == or != comparison in return-if-fails, because "assertion 'children != NULL' failed" is a more obvious error message than "assertion 'children' failed".

>+    do {
>+	mid = low + (high - low) / 2;
>+      ...

I'm not bothering to check your math here, although it occurs to me that if there was an off-by-one error, it might be the cause of your matching bug?

>+gweather_location_insert (GWeatherLocationEntry *entry,

This is still weird; it's a GWeatherLocation method, but it takes a GWeatherEntry as its first parameter?

I think you can make it:

gboolean gweather_location_insert (GWeatherLocation *top,
                                   GWeatherLocation *loc,
                                   const char       *country,
                                   const char       *state);

and then you'd call it like:

    if (gweather_location_insert (entry->top, loc, country, state))
        gweather_location_entry_add_model (entry, loc);

Except... wait... actually, nothing ever calls gweather_location_insert()!? Oh, it looks like gweather-geonames.c is missing from this patch...

>+    g_return_val_if_fail (entry != NULL, FALSE);

g_return_val_if_fail (GWEATHER_IS_LOCATION_ENTRY (entry), FALSE);

(which checks for both NULL and for wrong-type)

>+    children[offset] = loc;

you probably need to gweather_location_ref (loc)

>+++ b/libgweather/location-entry.c

>+#include <assert.h>

you don't need that

>+    entry->gns_id = g_timeout_add (500, gweather_geonames_gsourcefunc, entry);

Ideally this would only run when the entry had at least GWEATHER_LOCATION_MIN_COMPLETION_KEY_LENGTH characters in it, although I guess it's not like the entry is going to be visible for along time anyway, so probably doesn't really matter.

>+void
>+gweather_location_entry_add_model (GWeatherLocationEntry *entry,

either "add_location" or "add_to_model" would make more sense

>+gweather_geonames_gsourcefunc (gpointer entryp)

I'll probably have more comments on this after seeing how it's used from gweather-geonames.c

>+++ b/libgweather/location-entry.h

>+#include "parser.h"

You can't include parser.h from here, because location-entry.h gets installed as a public header and parser.h doesn't. But it looks like you don't need it anyway?

>+    SoupSession     *gns_session;
>+    char            *gns_query;
>+    guint16          gns_qlen;
>+    SoupMessage     *gns_msg;
>+    guint            gns_id;
>+    int              processed;
>+    int              total_results;
> } GWeatherLocationEntry;

ugh. we really need to do the "GWeatherLocationEntryPrivate" thing. Maybe later.

> 							const char            *code);
> 
>+void              gweather_location_entry_add_model    (GWeatherLocationEntry *entry,

try to keep the existing alignment within the .h file

>+++ b/libgweather/parser.h

>+#include "weather-priv.h"

I don't think that's needed there?

>-char           *gweather_parser_get_value           (GWeatherParser *parser);
>-char           *gweather_parser_get_localized_value (GWeatherParser *parser);
>+char           *gweather_reader_get_value           (xmlTextReaderPtr reader);
>+char           *gweather_parser_get_localized_value (GWeatherParser  *parser);

Can you move gweather_reader_get_value() to a separate section (ie, after a blank line after gweather_parser_get_localized_value(), so it's not mixed in with the methods that still take a GWeatherParser).

Working on these presently; one or two comments...

(In reply to comment #17)
> >+ * gweather_location_new_from_geonames:
>... 
> >+	if (!strcmp (tagname, "name") && !loc->name) {
> 
> So it looks like you're not looking at the alternateNames at all? Oh, but
> geonames uses the language you passed in in the request to decide what language
> the <name> should be, right?

Right.  When the request includes the language the response doesn't include alternameNames.

> 
> >+gweather_location_insert (GWeatherLocationEntry *entry,
> 
> This is still weird; it's a GWeatherLocation method, but it takes a
> GWeatherEntry as its first parameter?
> 
> I think you can make it:
> 
> gboolean gweather_location_insert (GWeatherLocation *top,
>                                    GWeatherLocation *loc,
>                                    const char       *country,
>                                    const char       *state);
>...
The end of the routine passes the GWeatherLocationEntry* off to gtk_entry_get_location(), largely to get the GtkTreeStore* for the gtk_tree_store calls.  I figured that this was analogous to gweather_location_entry_build_model() so kept the operations to get to the tree store pointer inside the routine rather than outside.

> >+    children[offset] = loc;
>
> you probably need to gweather_location_ref (loc)

Even though it was set to 1 when created in location_new_from_xml() ?  Or is the proper approach to always call gweather_location_unref() in the caller? Currently .._unref() is called only when the insert operation fails.

I should have another patch ready tomorrow or Sunday.

Created attachment 182744 [details] [review]
Revised patch

The "nit" problem still exists.  I don't think that it relates to the children ordering since backspacing wouldn't reveal the missing entry.  Somehow the matcher function isn't picking it up when it should.

played with this a little last night. a few notes:

In gweather_geonames_gsourcefunc():

>+	    if ((strncmp (value, entry->gns_query, entry->gns_qlen) != 0)) {
>+		/* Name has changed since the last iteration */

I think that's your bug; you want to strcmp() the whole string, or else it will only notice changes when the string gets shorter, not when it gets longer.

Also, it looks like the do/while is just there so you can use "break" in the middle? If it's not ever going to actually loop, it would be nicer to just remove the do/while and use a goto instead.

>+    entry->gns_msg = soup_form_request_new
>+		    ("GET",             "http://ws.geonames.org/search",
>+		     "name_startsWith", g_utf8_normalize (entry->gns_query, -1,
>+							  G_NORMALIZE_ALL),
>+		     "featureClass",    "P",  /* city, village.. */
>+		     "lang",            g_utf8_normalize (language, -1,
>+							  G_NORMALIZE_ALL),
>+		     "startRow",        start,
>+		     "style",           "full",  /* for timezone */
>+		     "isNameRequired",  "true",
>+		     NULL);

g_utf8_normalize() returns new memory, so you have to assign to temporary variables and then free it afterward.

I don't think we want "name_startsWith"; that will only work if the user is looking for a city with a reasonably unique name. If you're looking for a specific "Springfield", you're probably going to need to type in the state name as well. (The default number of responses returned seems to be pretty small, so it's not going to include every Springfield if you search for just that.)

If you use "q" instead of "name_startsWith", you can include multiple words, and it will require them all to match. Just turn all commas in the query into spaces, and then if the user types "Springfield, NH", it should find the right one.

Also, while looking to see if there was something better than either name_startsWith or q, I noticed that the current geonames API docs (a) refer to api.geonames.org, rather than ws.geonames.org, and (b) require that you include a "username" field, which is used to limit the number of lookups per hour/day. So I'm guessing that ws.geonames.org is probably going to go away, so we should use api instead (which is identical other than requiring the username). The username stuff doesn't look too bad, and if we just have a single "libgweather" username, I don't think we'll hit the limits (since it would mostly only get used when people are adding new locations).

Created attachment 183817 [details] [review]
Changes based on comment #20

(In reply to comment #20)
> I think that's your bug;..
No, I tried that but had the same results.

>.. you want to strcmp() the whole string, or else it will
> only notice changes when the string gets shorter, not when it gets longer.
That's actually the intent.  If the earlier query on the shorter string has already been placed into the tree, another query on the longer one will just turn up results which are already there (presuming geonames isn't getting updated at the same time).

The description section of the GtkEntryCompletion page includes the phrase "when the user modifies the text".  That's what I think the underlying issue is: the completions on the GtkEntry box doesn't get refreshed until an actual keystroke occurs.  The problem here is that I want the refresh to occur immediately rather than waiting for the keystroke.

> 
> Also, it looks like the do/while is just there so you can use "break" in the
> middle? If it's not ever going to actually loop, it would be nicer to just
> remove the do/while and use a goto instead.
OK.. it's a habit I got into years ago after working with someone who would bring code reviews to a grinding halt whenever a "goto" statement appeared.

> 
> >+    entry->gns_msg = soup_form_request_new
> >+		    ("GET",             "http://ws.geonames.org/search",
> >+		     "name_startsWith", g_utf8_normalize (entry->gns_query, -1,
> >+							  G_NORMALIZE_ALL),
> >+		     "featureClass",    "P",  /* city, village.. */
> >+		     "lang",            g_utf8_normalize (language, -1,
> >+							  G_NORMALIZE_ALL),
> >+		     "startRow",        start,
> >+		     "style",           "full",  /* for timezone */
> >+		     "isNameRequired",  "true",
> >+		     NULL);
> 
> g_utf8_normalize() returns new memory, so you have to assign to temporary
> variables and then free it afterward.
Done.  I'm setting entry->gns_query in the normalized form now.

> 
> I don't think we want "name_startsWith"; that will only work if the user is
> looking for a city with a reasonably unique name...
OK

> The username stuff doesn't look too bad, and if we just have a single
> "libgweather" username, I don't think we'll hit the limits.
OK -- I had originally assumed that the field was intended to identify the person making the query rather than the app but this makes more sense.

Comment on attachment 183817 [details] [review]
Changes based on comment #20

These are to be applied on top of the previous patch

Is there any reason why the location lookup isn't done through geoclue instead?

It would allow plugging services other than geonames.org in place, without modifying the libgweather API in the future.

hm... there's a bug about using geoclue to find your current coordinates (bug 562141), but I didn't realize it also supported lookups by name...

but anyway, it probably doesn't return all of the information we need (eg, timezones, weather station)

(In reply to comment #24)
> hm... there's a bug about using geoclue to find your current coordinates (bug
> 562141), but I didn't realize it also supported lookups by name...

That's what the geocode part of it is.

> but anyway, it probably doesn't return all of the information we need (eg,
> timezones, weather station)

We could actually export those fairly easily, by replacing the current API which returns long/lat/alt with a hash table of metadata provided by the backend.
This API change was actually discussed nearly two years ago at GCDS.

If Frank is interested, I could certainly help push the patches, and answer questions.

Sure, I'd be willing.

Would we want to commit these changes in the meantime while that effort is under way?

*** Bug 655875 has been marked as a duplicate of this bug. ***

Hi, I'm the developer of the weather extension in GNOME Shell. Many of my user complaint about the lack of city in libGWeather and this idea could resolve it. Like you can see : "-Grimmer : I really like your weather extension, but there's one major flaw. Just like gnome-weather there are too few cities... Weather data for my town can't be found, making this app almost useless. :(", "-kopstukken : Was a good plug in. Changing to another weather service makes it less usefull for me. Due to the limited Dutch cities available.", "-revoluzzer : Many cities are no more present. So it's useless for me. Same problem with the weather application of gnome." etc ...

Why don't you use geocode-glib to find the coordinates for the town the user types, and find the nearest weather station from there? This is what weather does, and what you should do as well. If there's an area that's not covered (eg. has a weather station which isn't listed in libgweather) then they should get added. But it's likely that the user just doesn't know the name of the nearest airport.

What happens with my city? I'm at Botucatu, one of the most important cities in the most important state of Brazil. My city has more than 130,000 people living here, and we have one of the most important aircraft manufacturer of the World, Embraer. If only cities greater than 100k people, why my city, with 130k, is no longer available?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libgweather/-/issues/92.