After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699311 - wrong time displayed for the Novosibirsk, Russia location
wrong time displayed for the Novosibirsk, Russia location
Status: RESOLVED FIXED
Product: libgweather
Classification: Core
Component: locations
3.8.x
Other Linux
: Normal normal
: future
Assigned To: libgweather-maint
libgweather-maint
Depends on:
Blocks:
 
 
Reported: 2013-04-30 13:47 UTC by Evgeny Bobkin
Modified: 2013-05-07 07:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
a small test case app with which you can simply check if found city has a corresponding time zone (12.46 KB, application/x-compressed-tar)
2013-05-04 14:01 UTC, Evgeny Bobkin
  Details
Fix timezone lookup for some cities (23.14 KB, patch)
2013-05-04 18:31 UTC, Pavel Vasin
none Details | Review
Add timezone for Khanty-Mansiysk and Ul'yanovsk (989 bytes, patch)
2013-05-04 18:31 UTC, Pavel Vasin
committed 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 (869 bytes, patch)
2013-05-04 20:39 UTC, Evgeny Bobkin
needs-work Details | Review
Fix timezone lookup for some cities v2 (23.79 KB, patch)
2013-05-05 05:22 UTC, Pavel Vasin
committed Details | Review
libgweather: alternative solution for timezone issue (970 bytes, patch)
2013-05-05 12:54 UTC, Evgeny Bobkin
rejected Details | Review
reworked patch (1.59 KB, patch)
2013-05-06 10:54 UTC, Evgeny Bobkin
committed Details | Review

Description Evgeny Bobkin 2013-04-30 13:47:59 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
Comment 1 Evgeny Bobkin 2013-05-01 15:34:42 UTC
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
Comment 2 Paolo Borelli 2013-05-01 17:46:08 UTC
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,
Comment 3 Evgeny Bobkin 2013-05-04 14:01:14 UTC
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>
Comment 4 Evgeny Bobkin 2013-05-04 14:09:34 UTC
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.
Comment 5 Pavel Vasin 2013-05-04 18:31:24 UTC
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
Comment 6 Pavel Vasin 2013-05-04 18:31:42 UTC
Created attachment 243304 [details] [review]
Add timezone for Khanty-Mansiysk and Ul'yanovsk
Comment 7 Evgeny Bobkin 2013-05-04 20:39:24 UTC
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.
Comment 8 Evgeny Bobkin 2013-05-04 23:44:13 UTC
(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>
Comment 9 Pavel Vasin 2013-05-05 05:22:36 UTC
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.
Comment 10 Evgeny Bobkin 2013-05-05 08:08:46 UTC
(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))
Comment 11 Paolo Borelli 2013-05-05 09:57:26 UTC
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")
Comment 12 Evgeny Bobkin 2013-05-05 12:54:26 UTC
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.
Comment 13 Evgeny Bobkin 2013-05-06 10:54:18 UTC
Created attachment 243376 [details] [review]
reworked patch
Comment 14 Evgeny Bobkin 2013-05-06 10:56:32 UTC
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 15 Paolo Borelli 2013-05-06 12:13:27 UTC
Comment on attachment 243376 [details] [review]
reworked patch

Thanks! I applied the gnome-clocks patch.
Comment 16 Giovanni Campagna 2013-05-06 17:38:02 UTC
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!
Comment 17 Pacho Ramos 2013-05-06 20:22:46 UTC
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?
Comment 18 Giovanni Campagna 2013-05-06 20:26:48 UTC
I will cherry-pick it before I roll the 3.8.2 tarball, at the next stable milestone (which would be next week)
Comment 19 Evgeny Bobkin 2013-05-06 20:40:53 UTC
Nice, we are awaiting it for gentoo)))
Comment 20 Bastien Nocera 2013-05-07 07:45:34 UTC
Giovanni, does it make sense to add a self-test in libgweather for this, to avoid any locations having missing information?