GNOME Bugzilla – Bug 699311
wrong time displayed for the Novosibirsk, Russia location
Last modified: 2013-05-07 07:45:34 UTC
gnome-clocks shows always a wrong time at least for one location, looks like a timezone issue. <mclasen> ibqn: for me, novosibirsk has the same time as Boston <mclasen> so, I would guess we simply don't have a timezone for it, and fall back to the system time <mclasen> which is hilariously wrong
right after adding a Novosibirsk location, the following assertion occurs: $ gnome-clocks (gnome-clocks:7205): GWeather-CRITICAL **: gweather_timezone_get_tzid: assertion `zone != NULL' failed
I could probably handle the null timezone there, but the critical warning seems to suggest that the gweather function is never supposed to return a null tzid and that this is a bug in the gweather database. I am reassigning the bug there. Maybe gweather should have a unit test to verify that all locations have a tzid,
Created attachment 243288 [details] a small test case app with which you can simply check if found city has a corresponding time zone attached is a small test case app with which you can simply check if found city has a corresponding time zone. after playing around with it, I have found cities, which has no time zone, like a prominent mentioned above example. I also searched a bit in libgweather source code and discovered, that actually Locations.xml.in file contains a Novosibirsk location with a corresponding time zone Asia/Omsk!!! So this bug report is not about adding a missing city or a time zone, but about something going wrong during parsing of the xml data!!! Russia has this time zone as well: <country> <!-- RU - Russian Federation --> <_name>Russia</_name> ... <timezone id="Asia/Omsk"> <!-- A Russian time zone, used in the Omsk and Novosibirsk oblasts and surrounding areas of south-central Russia. The Russian name is "Омское время". This string is only used in places where "Russia" is already clear from context. --> <_name>Omsk Time</_name> <obsoletes>Asia/Novosibirsk</obsoletes> </timezone> and the city Novosibirsk actually has a tz-hint for this time zone: <city> <!-- A city in Russia. The local name in Russian is "Новосибирск". --> <_name>Novosibirsk</_name> <coordinates>55.041111 82.934444</coordinates> <location> <name>Novosibirsk</name> <code>UNNT</code> <tz-hint>Asia/Omsk</tz-hint> <coordinates>55.083333 82.900000</coordinates> </location> </city>
This should be also a gnome-clocks 3.8.x bug report, because gnome-clocks should not allow users to create broken clocks without a warning message! People should be able to rely on this app. I have developed patches, which I want to propose and going to submit them this week end.
Created attachment 243303 [details] [review] Fix timezone lookup for some cities move tz-hint from weather stations up to cities for cities that has no timezone info
Created attachment 243304 [details] [review] Add timezone for Khanty-Mansiysk and Ul'yanovsk
Created attachment 243312 [details] [review] first patch proposal for gnome-clocks-3.8.1:trivial patch with a minimal code change, simply do not allow a user to create a new clock for a location with a unknown time zone I have tested this patch. It fixes the error message from comment 1. Here, by creating a new clock for a defined and matched location the OK button of the dialog remains disabled, if the time zone for this location is not known, so the user cannot add such a broken clock.
(In reply to comment #5) > Created an attachment (id=243303) [details] [review] > Fix timezone lookup for some cities > > move tz-hint from weather stations up to cities > for cities that has no timezone info where do you have this patch from? it simply breaks the xml content: xmllint --valid --noout ../data/Locations.xml.in ../data/Locations.xml.in:789: element city: validity error : Element city content does not follow the DTD, expecting ((_name | name+) , coordinates? , location+), got (_name coordinates tz-hint location ) </city>
Created attachment 243321 [details] [review] Fix timezone lookup for some cities v2 >it simply breaks the xml content v2: update dtd >where do you have this patch from? I wrote this patch.
(In reply to comment #9) > Created an attachment (id=243321) [details] [review] > Fix timezone lookup for some cities v2 > > >it simply breaks the xml content > > v2: update dtd > > >where do you have this patch from? > > I wrote this patch. nice. I tested this one successfully, however this one requires a modification of the dtd content. I am going to try to suggest another one, in which I plan to modify the GWeatherTimezone * gweather_location_get_timezone (GWeatherLocation *loc) function so that if it fails to find the tz_hint by its parent, it should look for it by its children. Let's if it works))
Review of attachment 243312 [details] [review]: I agree with this approach: it is unobtrusive enough and makes clocks robust against this corner case. A few comments on the patch itself: - please clone clocks from the git repo and submit the patch with git format-patch (see https://live.gnome.org/GnomeLove/SubmittingPatches ) - be careful to not use tabs for the indentation - use { } also for the single line "if" - within the if add a line with warning("Timezone not defined for %s. This is a bug in libgweather database")
Created attachment 243331 [details] [review] libgweather: alternative solution for timezone issue alternative way to fix the timezone issue for Novosibirsk and similar cities in comparison with the working patch proposed by Pavel Vosin, in which instead of changing the DTD of the Locations.xml (makes sense in my opinion) enhances the functionality of the gweather_location_get_timezone (GWeatherLocation *loc) function. working principle: if the tz-hint cannot be found by searching in the parent location (which was a default case), try to determine it by searching through the children (which are weather station locations having a tz-hint) both patches can coexist together.
Created attachment 243376 [details] [review] reworked patch
Comment on attachment 243312 [details] [review] first patch proposal for gnome-clocks-3.8.1:trivial patch with a minimal code change, simply do not allow a user to create a new clock for a location with a unknown time zone there is a rework of it
Comment on attachment 243376 [details] [review] reworked patch Thanks! I applied the gnome-clocks patch.
Ok, so, from my point of view it doesn't make sense to have a city level location that looks inside it's children for the timezone, as potentially they can different (that's the point of tz-hint on <location> after all). I applied the two patches from Pavel, thank you everyone!
I have seen this has been committed only to "master", could it be also committed to 3.8 branch to get it fixed in future 3.8.x tarballs?
I will cherry-pick it before I roll the 3.8.2 tarball, at the next stable milestone (which would be next week)
Nice, we are awaiting it for gentoo)))
Giovanni, does it make sense to add a self-test in libgweather for this, to avoid any locations having missing information?