Created attachment 364727 [details] [review]
location: Fix gir-scanner compile time error

libgweather/gweather-location.c:804: Warning: GWeather: gweather_location_detect_nearest_city: invalid return annotation

There's nothing returned from the function, so no need to document
"Returns".

Created attachment 364728 [details] [review]
GWeatherLocationEntry: Simplify tree store insertions

There's no need to append to then insert, when we could insert new
values directly.

Created attachment 364729 [details] [review]
GWeatherLocationEntry: Make single words match

A single word should really be matching against another single word
string.

Created attachment 364730 [details] [review]
location: Add helper to create GWeatherLocation struct

Created attachment 364731 [details] [review]
location: Add gweather_location_new_utc()

This creates a new world with a single "UTC" country, which can be used
to search for the UTC virtual timezone using the same code as for
searching through the physical world location.

Created attachment 364732 [details] [review]
location: Add support for deserialising UTC

Created attachment 364733 [details] [review]
GWeatherLocationEntry: Build model after object is constructed

So that additional construct time properties can be used.

Created attachment 364734 [details] [review]
GWeatherLocationEntry: Add show-utc property

To show or not the "UTC" timezone as a location in the completion list.

Created attachment 364735 [details] [review]
lib: Add test for show-utc property

Created attachment 364736 [details] [review]
Add UTC Clock

Requires a newer libgweather, and some careful stepping around the fact
that there is no weather info for the UTC timezone.

Comment on attachment 364736 [details] [review]
Add UTC Clock

Removing that, it's the gnome-clocks patch

A couple of things:
- we might want to add some API to allow gnome-clocks to show this clock differently, rather than assume it's always day there.
- I'm not sure I'm creating the GWeatherLocations correctly, we might want them to be boxed objects
- I'm also not sure about the memory management

Review of attachment 364727 [details] [review]:

ACK-by: me

Review of attachment 364728 [details] [review]:

ACK-by: me

Review of attachment 364729 [details] [review]:

::: libgweather/gweather-location-entry.c
@@ +646,3 @@
+    match = match_compare_name (key, local_compare_name) ||
+	    match_compare_name (key, english_compare_name) ||
+	    g_ascii_strcasecmp (key, english_compare_name) == 0;

Looks good, though for future enhancement we may want to use g_str_match_string() from GLib to do substring matches with tokenization and case folding.

Review of attachment 364730 [details] [review]:

ACK-by: me

Review of attachment 364731 [details] [review]:

Before landing this, there should be a commit bumping up the version of the project. There has been a 3.27.1 release already, and without a version bump it projects won't be able to bump their dependencies accurately.

Review of attachment 364732 [details] [review]:

::: libgweather/gweather-location.c
@@ +1363,3 @@
     GWeatherLocation *found;
 
+    if (g_strcmp0 (station_code, "UTC") == 0)

Should this be case insensitive?

(In reply to Emmanuele Bassi (:ebassi) from comment #18)
> Review of attachment 364732 [details] [review] [review]:
> 
> ::: libgweather/gweather-location.c
> @@ +1363,3 @@
>      GWeatherLocation *found;
>  
> +    if (g_strcmp0 (station_code, "UTC") == 0)
> 
> Should this be case insensitive?

The station code is the same one we create in gweather_location_new_utc() serialised to gsettings, then deserialised, so it should be exactly this.

Review of attachment 364733 [details] [review]:

ACK-by: me

Review of attachment 364734 [details] [review]:

ACK-by: me

Review of attachment 364735 [details] [review]:

It would be nice to turn all these interactive tests into unit tests

(In reply to Bastien Nocera from comment #19)
> (In reply to Emmanuele Bassi (:ebassi) from comment #18)
> > Review of attachment 364732 [details] [review] [review] [review]:
> > 
> > ::: libgweather/gweather-location.c
> > @@ +1363,3 @@
> >      GWeatherLocation *found;
> >  
> > +    if (g_strcmp0 (station_code, "UTC") == 0)
> > 
> > Should this be case insensitive?
> 
> The station code is the same one we create in gweather_location_new_utc()
> serialised to gsettings, then deserialised, so it should be exactly this.

Okay, so: ACK-by: me.

I've said it before, and I'll say it again. I don't like this.

UTC is not a location. It does not have coordinates. It does not have a weather station code. It does not have a parent city or country (hence no country code). Basically none of the GWeatherLocation APIs work, except get_timezones().

UTC does not make sense to show in GWeatherLocationEntry for 2/3 of the users of GWeatherLocationEntry, hence the hack of a show-utc property.

What we need is a GWeatherTimezoneEntry, replacing GWeatherTimezoneMenu with search, and returning GWeatherTimezones. Then gnome-clocks can stop abusing the location entry, we get other timezone abbreviations for free, and this whole problem is over.

Also, if UTC is added, then one might wonder why not Anywhere on Earth (which is just as useful), at which point one realizes that this special-case approach does not really scale.

(In reply to Giovanni Campagna from comment #25)
> Also, if UTC is added, then one might wonder why not Anywhere on Earth
> (which is just as useful), at which point one realizes that this
> special-case approach does not really scale.

A couple of reasons:
- It's a calendar day, not a time. "For this day, this is the Anywhere on Earth time for it in that timezone". It would need a very different UI in gnome-clocks.
- It's the first time I ever hear of it, and it's the first time anyone has ever mentioned it in a gnome-clocks or libgweather bug:
https://bugzilla.gnome.org/buglist.cgi?longdesc=anywhere%20on%20earth&longdesc_type=allwordssubstr&product=gnome-clocks&product=libgweather&query_format=advanced

Nobody has asked for UTC+X or UTC-X timezones either, and their naming would make it really hard to integrate (UTC+5 is UTC minus 5 hours, see "TZ=UTC+5 date", yay POSIX timezones!).

If you have better ideas about the API, please let me know, but I think we're solving the use case people care about. I could certainly try to make the GWeatherLocation changes more generic, returning a "virtual world" rather than a single location.

I'm holding off on pushing the patches because I want to try and see what test cases I can create to ensure I don't introduce more refcounting problems.

(In reply to Bastien Nocera from comment #26)
> (In reply to Giovanni Campagna from comment #25)
> > Also, if UTC is added, then one might wonder why not Anywhere on Earth
> > (which is just as useful), at which point one realizes that this
> > special-case approach does not really scale.
> 
> A couple of reasons:
> - It's a calendar day, not a time. "For this day, this is the Anywhere on
> Earth time for it in that timezone". It would need a very different UI in
> gnome-clocks.

Isn't that the the same clocks UI?
Like, when I need AoE time (which is usually around deadlines) I want to know what time it is in AoE, and how many hours are there to midnight.
AoE is a timezone afterall.

You can also flip it and say, "when is midnight AoE in my timezone", but that pretty much applies to any timezone anyway, so it would be a UI issue in Clocks, not a libgweather problem. E.g, I've seen conference deadlines at all possible times, sometime in PDT, EST, HAST.
What's more, is that the posted deadline will use the timezone name or abbreviation, which means that you really want to search for the timezone in the entry.

> - It's the first time I ever hear of it, and it's the first time anyone has
> ever mentioned it in a gnome-clocks or libgweather bug:
> https://bugzilla.gnome.org/buglist.
> cgi?longdesc=anywhere%20on%20earth&longdesc_type=allwordssubstr&product=gnome
> -clocks&product=libgweather&query_format=advanced
> 
> Nobody has asked for UTC+X or UTC-X timezones either, and their naming would
> make it really hard to integrate (UTC+5 is UTC minus 5 hours, see "TZ=UTC+5
> date", yay POSIX timezones!).

POSIX timezones don't really matter here; we can decide that UTC-X means UTC minus X hours like it should.
OTOH I would expect people will ask for Pacific/Mountain/Central/Eastern Times, because at least in the US that's how timezones are referred to, rather than a representative city.

> 
> If you have better ideas about the API, please let me know, but I think
> we're solving the use case people care about. I could certainly try to make
> the GWeatherLocation changes more generic, returning a "virtual world"
> rather than a single location.

The better API, as I suggested two comments ago (and previously in the Clocks bug), is a GWeatherTimezoneEntry, that does not touch GWeatherLocations at all. 


It's worth also mentioning that Clocks shows the sunrise/sunset times for a city, which don't make sense for UTC or any of the timezones, which means Clocks will need to special case UTC and it gains nothing by reusing GWeatherLocation.

(In reply to Giovanni Campagna from comment #27)
> (In reply to Bastien Nocera from comment #26)
> > (In reply to Giovanni Campagna from comment #25)
> > > Also, if UTC is added, then one might wonder why not Anywhere on Earth
> > > (which is just as useful), at which point one realizes that this
> > > special-case approach does not really scale.
> > 
> > A couple of reasons:
> > - It's a calendar day, not a time. "For this day, this is the Anywhere on
> > Earth time for it in that timezone". It would need a very different UI in
> > gnome-clocks.
> 
> Isn't that the the same clocks UI?
> Like, when I need AoE time (which is usually around deadlines) I want to
> know what time it is in AoE, and how many hours are there to midnight.
> AoE is a timezone afterall.
> 
> You can also flip it and say, "when is midnight AoE in my timezone", but
> that pretty much applies to any timezone anyway, so it would be a UI issue
> in Clocks, not a libgweather problem. E.g, I've seen conference deadlines at
> all possible times, sometime in PDT, EST, HAST.
> What's more, is that the posted deadline will use the timezone name or
> abbreviation, which means that you really want to search for the timezone in
> the entry.

Fair enough. But still, nobody's ever asked for it, or even mentioned it before. I don't think it's fair to be expected to add features nobody's ever asked for.

> > - It's the first time I ever hear of it, and it's the first time anyone has
> > ever mentioned it in a gnome-clocks or libgweather bug:
> > https://bugzilla.gnome.org/buglist.
> > cgi?longdesc=anywhere%20on%20earth&longdesc_type=allwordssubstr&product=gnome
> > -clocks&product=libgweather&query_format=advanced
> > 
> > Nobody has asked for UTC+X or UTC-X timezones either, and their naming would
> > make it really hard to integrate (UTC+5 is UTC minus 5 hours, see "TZ=UTC+5
> > date", yay POSIX timezones!).
> 
> POSIX timezones don't really matter here; we can decide that UTC-X means UTC
> minus X hours like it should.
> OTOH I would expect people will ask for Pacific/Mountain/Central/Eastern
> Times, because at least in the US that's how timezones are referred to,
> rather than a representative city.

There's nothing stopping us from making the timezones something we can search for in GWeatherLocationEntry.

> > If you have better ideas about the API, please let me know, but I think
> > we're solving the use case people care about. I could certainly try to make
> > the GWeatherLocation changes more generic, returning a "virtual world"
> > rather than a single location.
> 
> The better API, as I suggested two comments ago (and previously in the
> Clocks bug), is a GWeatherTimezoneEntry, that does not touch
> GWeatherLocations at all. 

I don't think that this makes for appealing UI, having 2 separate entries. Users will expect to be able to enter UTC (or anything else) into the one entry, and have completion work as expected.

> It's worth also mentioning that Clocks shows the sunrise/sunset times for a
> city, which don't make sense for UTC or any of the timezones, which means
> Clocks will need to special case UTC and it gains nothing by reusing
> GWeatherLocation.

Already done, it's in the gnome-clocks patch.

Attachment 364727 [details] pushed as b1cb7dc - location: Fix gir-scanner compile time error
Attachment 364728 [details] pushed as 0d400ec - GWeatherLocationEntry: Simplify tree store insertions
Attachment 364730 [details] pushed as 2083210 - location: Add helper to create GWeatherLocation struct
Attachment 364733 [details] pushed as 8060e45 - GWeatherLocationEntry: Build model after object is constructed

I pushed all the cleanup patches in the set, as those shouldn't be too controversial.

(In reply to Emmanuele Bassi (:ebassi) from comment #15)
> Review of attachment 364729 [details] [review] [review]:
> 
> ::: libgweather/gweather-location-entry.c
> @@ +646,3 @@
> +    match = match_compare_name (key, local_compare_name) ||
> +	    match_compare_name (key, english_compare_name) ||
> +	    g_ascii_strcasecmp (key, english_compare_name) == 0;
> 
> Looks good, though for future enhancement we may want to use
> g_str_match_string() from GLib to do substring matches with tokenization and
> case folding.

Filed as https://bugzilla.gnome.org/show_bug.cgi?id=791198

Created attachment 364898 [details] [review]
location: Add gweather_location_new_virtual()

This creates a new world with a single "UTC" entry, which can be used
to search for the UTC virtual timezone using the same code as for
searching through the physical world location.

This can also be extended to include more virtual locations as needed in
front-ends.

Created attachment 364899 [details] [review]
location: Add support for deserialising virtual locations

Created attachment 364900 [details] [review]
GWeatherLocationEntry: Add show-virtual property

To show or not virtual locations, such as the "UTC" timezone as
a location in the completion list.

Created attachment 364901 [details] [review]
lib: Add test for show-virtual property

I turned the single "utc" location into a world of virtual locations containing a single entry. This would make it extensible if you wanted to add "Anywhere on Earth" support, or another virtual location.

For timezones, I think that making the entry autocomplete on timezone names might be the better option.

Created attachment 364903 [details] [review]
build: Bump version for new API

Created attachment 365050 [details] [review]
GWeatherLocation: add named timezones

Named timezones represent things like UTC, which are locations
enough that some apps want them in a GWeatherLocationEntry, but not
not quite cities or weather stations.

They are stored in Locations.xml as usual, as children of the world
location.

Created attachment 365051 [details] [review]
GWeatherLocationEntry: Add show-named-timezone property

To show or not named timezone locations, such as the "UTC" timezone as
a location in the completion list.

Based on a patch by Bastien Nocera <hadess@hadess.net>

Created attachment 365052 [details] [review]
lib: Add test for show-virtual property

Created attachment 365053 [details] [review]
GWeatherLocationEntry: fix matching with parenthesis

Treat ( as the start of a new "subname" for the location, like ","
does

This is a counterproposal.
It requires less new API than yours, which is more future proof.
It is more extensible to new location (one database addition instead of code changes).
It puts the location translations in the right place.

What do you think?

Review of attachment 365050 [details] [review]:

Looks fine otherwise.

::: libgweather/gweather-location.c
@@ +1341,3 @@
     candidates = g_hash_table_lookup (world->metar_code_cache, station_code);
 
+    /* A station code beginning with @ indicates a named timezone entry, just

I don't find this very elegant, and it's not verified in the DTD.

Review of attachment 365051 [details] [review]:

>  Add show-named-timezone property

timezones

> To show or not named timezone locations

Whether to

Review of attachment 365052 [details] [review]:

Sure.

Review of attachment 365053 [details] [review]:

This really doesn't end up making it more readable :/

Apart from nits and the "@" prefix which I find weird (we could just as well have it prefixed with "TZ:" and use the POSIX name for the timezone, no?), looks good.

(In reply to Bastien Nocera from comment #42)
> Review of attachment 365050 [details] [review] [review]:
> 
> Looks fine otherwise.
> 
> ::: libgweather/gweather-location.c
> @@ +1341,3 @@
>      candidates = g_hash_table_lookup (world->metar_code_cache,
> station_code);
>  
> +    /* A station code beginning with @ indicates a named timezone entry,
> just
> 
> I don't find this very elegant, and it's not verified in the DTD.

I agree it's not as elegant as it could be.
It's better than just using UTC as the station code though, cause at least you know that no ICAO code can start with @.
(Any other non-alphabetic character would do. TZ: would also do because of the colon, but then it's not as convenient)

As for the DTD, that's the limitation of DTDs. XMLSchema would be able to validate that, but I don't really want to write an XML Schema.

Then I'm fine with it if you're fine with it.

Attachment 364729 [details] pushed as 2d9cf10 - GWeatherLocationEntry: Make single words match
Attachment 364901 [details] pushed as 2e34a45 - lib: Add test for show-virtual property
Attachment 364903 [details] pushed as e2cb8a9 - build: Bump version for new API
Attachment 365050 [details] pushed as cd8464f - GWeatherLocation: add named timezones
Attachment 365052 [details] pushed as 2e34a45 - lib: Add test for show-virtual property
Attachment 365053 [details] pushed as 8a5d5bb - GWeatherLocationEntry: fix matching with parenthesis