Merge lp:~cjwatson/ubiquity/libsoup-timezone into lp:ubiquity

[Colin Watson]

Fix bug 837217 by fetching geoname data asynchronously using libsoup.

[Evan (ev)]

./tests/run-pyflakes
ubiquity/plugins/ubi-timezone.py:116: undefined name 'Gtk'
ubiquity/plugins/ubi-timezone.py:116: undefined name 'GObject'
ubiquity/plugins/ubi-timezone.py:116: undefined name 'GObject'
ubiquity/plugins/ubi-timezone.py:117: undefined name 'GObject'
ubiquity/plugins/ubi-timezone.py:117: undefined name 'GObject'
ubiquity/plugins/ubi-timezone.py:118: undefined name 'GObject'

[Evan (ev)]

(I'm happy to fix those up as I merge)

[Evan (ev)]

Shouldn't the block of code that does a lookup against self.zones still live in the changed() function? Otherwise, the user will only get results, including locally-computed ones, if the callback for hitting the geonames server succeeds.

review: Needs Information

[Evan (ev)]

(This caused the test_city_entry test to fail, which expects to get some local results upon setting the entry to Eastern.)

[Colin Watson (cjwatson)]

Fixed the missing imports, thanks.

The callback is supposed to fire no matter what, whether by cancellation or timeout or whatever; in my tests, I got locally-computed results even when the network was dropping all outbound port-80 packets. But perhaps it would be better to set the completion model straight away and fill it in with network results once they arrive; the main gotcha there is making sure that we only cache once we get network results.

[Evan (ev)]

Apologies, I was indeed wrong about the callback only firing on success.

Matthew suggests that a reasonable threshold is that if the timeout is going to be greater than 2-3 seconds, we should show the locally-computed results while we wait for network results.

[lp:~cjwatson/ubiquity/libsoup-timezone]

4997. By Colin Watson on 2011-09-28

missing imports

4998. By Colin Watson on 2011-09-30

Show tzdb completion entries after a short timeout.

4999. By Colin Watson on 2011-09-30

need to encode text before passing to urllib.quote

5000. By Colin Watson on 2011-09-30

fix up tests

=== modified file 'debian/changelog'
--- debian/changelog	2011-09-30 13:49:36 +0000
+++ debian/changelog	2011-09-30 21:34:24 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
 ubiquity (2.8.0) UNRELEASED; urgency=low
 
   * Fix SSID UTF-8 decoding, and replace any still-invalid characters with
@@ -29,6 +30,15 @@
 
  -- Adam Conrad <[e-mail]>  Wed, 28 Sep 2011 16:43:42 -0600
 
+=======
+ubiquity (2.7.38) UNRELEASED; urgency=low
+
+  * GTK frontend:
+    - Fetch geoname data asynchronously (LP: #837217).
+
+ -- Colin Watson <[e-mail]>  Wed, 28 Sep 2011 13:43:44 +0100
+
+>>>>>>> MERGE-SOURCE
 ubiquity (2.7.37) oneiric; urgency=low
 
   [ Evan Dandrea ]

=== modified file 'debian/control'
--- debian/control	2011-09-28 09:54:01 +0000
+++ debian/control	2011-09-30 21:34:24 +0000
@@ -43,7 +43,7 @@
 
 Package: ubiquity-frontend-gtk
 Architecture: any
-Depends: ${shlibs:Depends}, ${misc:Depends}, ${python:Depends}, ubiquity (= ${binary:Version}), python-dbus, gir1.2-gtk-3.0, gir1.2-gdkpixbuf-2.0, gir1.2-gstreamer-0.10, gir1.2-vte-2.90, gir1.2-webkit-3.0, iso-codes, x-window-manager, gksu, python-xklavier, gir1.2-timezonemap-1.0, python-libxml2, python-cairo, python-appindicator, gstreamer0.10-plugins-good
+Depends: ${shlibs:Depends}, ${misc:Depends}, ${python:Depends}, ubiquity (= ${binary:Version}), python-dbus, gir1.2-gtk-3.0, gir1.2-gdkpixbuf-2.0, gir1.2-gstreamer-0.10, gir1.2-soup-2.4, gir1.2-vte-2.90, gir1.2-webkit-3.0, iso-codes, x-window-manager, gksu, python-xklavier, gir1.2-timezonemap-1.0, python-libxml2, python-cairo, python-appindicator, gstreamer0.10-plugins-good
 Suggests: gnome-control-center
 Conflicts: ubuntu-express-frontend-gtk, espresso-frontend-gtk, ubiquity (<< 2.4.3)
 Replaces: ubuntu-express-frontend-gtk, espresso-frontend-gtk, ubiquity (<< 2.4.3)

=== modified file 'tests/test_timezone.py'
--- tests/test_timezone.py	2011-08-30 17:20:27 +0000
+++ tests/test_timezone.py	2011-09-30 21:34:24 +0000
@@ -19,10 +19,26 @@
         controller.dbfilter = self.ubi_timezone.Page(None, db=db)
         self.gtk = self.ubi_timezone.PageGtk(controller)
 
+    @mock.patch('gi.repository.Soup.SessionAsync')
+    @mock.patch('gi.repository.Soup.Message')
     @mock.patch('json.loads')
-    @mock.patch('urllib2.build_opener')
-    def test_city_entry(self, opener_mock, json_mock):
+    def test_city_entry(self, json_mock, *args):
+        from gi.repository import GObject
+
+        # Patch GObject.timeout_add_seconds to call the supplied function
+        # immediately rather than waiting for the interval to expire.
+        def side_effect_factory(real_method):
+            def side_effect(interval, function, data):
+                function(data)
+            return side_effect
+
         json_mock.return_value = []
+        real_method = GObject.timeout_add_seconds
+        method = mock.patch('gi.repository.GObject.timeout_add_seconds')
+        timeout_mock = method.start()
+        timeout_mock.side_effect = side_effect_factory(real_method)
+        self.addCleanup(method.stop)
+
         self.gtk.set_timezone('America/New_York')
         self.gtk.city_entry.set_text('Eastern')
         self.gtk.changed(self.gtk.city_entry)

=== modified file 'ubiquity/plugins/ubi-timezone.py'
--- ubiquity/plugins/ubi-timezone.py	2011-08-30 17:20:27 +0000
+++ ubiquity/plugins/ubi-timezone.py	2011-09-30 21:34:24 +0000
@@ -52,6 +52,9 @@
         self.timezone = None
         self.zones = []
         self.plugin_widgets = self.page
+        self.geoname_cache = {}
+        self.geoname_session = None
+        self.geoname_timeout_id = None
 
     def plugin_translate(self, lang):
         #c = self.controller
@@ -84,54 +87,95 @@
 
 
     def changed(self, entry):
-        from gi.repository import Gtk, GObject
+        import urllib
+        from gi.repository import Gtk, GObject, Soup
+
         text = self.city_entry.get_text().decode('utf-8')
         if not text:
             return
         # TODO if the completion widget has a selection, return?  How do we
         # determine this?
-        if text in self.changed.cache:
-            model = self.changed.cache[text]
+        if text in self.geoname_cache:
+            model = self.geoname_cache[text]
+            self.city_entry.get_completion().set_model(model)
         else:
-            # fetch
             model = Gtk.ListStore(GObject.TYPE_STRING, GObject.TYPE_STRING,
                                   GObject.TYPE_STRING, GObject.TYPE_STRING,
                                   GObject.TYPE_STRING)
-            self.changed.cache[text] = model
-            # TODO benchmark this
-            results = [(name, self.tzdb.get_loc(city))
-                        for (name, city) in
-                            [(x[0], x[1]) for x in self.zones
-                                if x[0].lower().split('(', 1)[-1] \
-                                                .startswith(text.lower())]]
-            for result in results:
-                # We use name rather than loc.human_zone for i18n.
-                # TODO this looks pretty awful for US results:
-                # United States (New York) (United States)
-                # Might want to match the debconf format.
-                name, loc = result
-                model.append([name, '', loc.human_country,
-                              str(loc.latitude), str(loc.longitude)])
-
-            try:
-                import urllib2, urllib, json
-                opener = urllib2.build_opener()
-                opener.addheaders = [('User-agent', 'Ubiquity/1.0')]
-                url = opener.open(_geoname_url %
-                    (urllib.quote(text), misc.get_release().version))
-                for result in json.loads(url.read()):
-                    model.append([result['name'],
-                                  result['admin1'],
-                                  result['country'],
-                                  result['latitude'],
-                                  result['longitude']])
-            except Exception, e:
-                print 'exception:', e
-                # TODO because we don't return here, we could cache a
-                # result that doesn't include the geonames results because
-                # of a network error.
-        self.city_entry.get_completion().set_model(model)
-    changed.cache = {}
+
+            if self.geoname_session is None:
+                self.geoname_session = Soup.SessionAsync()
+            url = _geoname_url % (urllib.quote(text.encode('UTF-8')),
+                                  misc.get_release().version)
+            message = Soup.Message.new('GET', url)
+            message.request_headers.append('User-agent', 'Ubiquity/1.0')
+            self.geoname_session.abort()
+            if self.geoname_timeout_id is not None:
+                GObject.source_remove(self.geoname_timeout_id)
+            self.geoname_timeout_id = \
+                GObject.timeout_add_seconds(2, self.geoname_timeout,
+                                            (text, model))
+            self.geoname_session.queue_message(message, self.geoname_cb,
+                                               (text, model))
+
+    def geoname_add_tzdb(self, text, model):
+        if len(model):
+            # already added
+            return
+
+        # TODO benchmark this
+        results = [(name, self.tzdb.get_loc(city))
+                    for (name, city) in
+                        [(x[0], x[1]) for x in self.zones
+                            if x[0].lower().split('(', 1)[-1] \
+                                            .startswith(text.lower())]]
+        for result in results:
+            # We use name rather than loc.human_zone for i18n.
+            # TODO this looks pretty awful for US results:
+            # United States (New York) (United States)
+            # Might want to match the debconf format.
+            name, loc = result
+            model.append([name, '', loc.human_country,
+                          str(loc.latitude), str(loc.longitude)])
+
+    def geoname_timeout(self, user_data):
+        text, model = user_data
+        self.geoname_add_tzdb(text, model)
+        self.geoname_timeout_id = None
+        self.city_entry.get_completion().set_model(model)
+        return False
+
+    def geoname_cb(self, session, message, user_data):
+        import syslog
+        import json
+        from gi.repository import GObject, Soup
+
+        text, model = user_data
+
+        if self.geoname_timeout_id is not None:
+            GObject.source_remove(self.geoname_timeout_id)
+            self.geoname_timeout_id = None
+        self.geoname_add_tzdb(text, model)
+
+        if message.status_code == Soup.KnownStatusCode.CANCELLED:
+            # Silently ignore cancellation.
+            pass
+        elif message.status_code != Soup.KnownStatusCode.OK:
+            # Log but otherwise ignore failures.
+            syslog.syslog('Geoname lookup for "%s" failed: %d %s' %
+                          (text, message.status_code, message.reason_phrase))
+        else:
+            for result in json.loads(message.response_body.data):
+                model.append([result['name'],
+                              result['admin1'],
+                              result['country'],
+                              result['latitude'],
+                              result['longitude']])
+
+            # Only cache positive results.
+            self.geoname_cache[text] = model
+
+        self.city_entry.get_completion().set_model(model)
 
     def setup_page(self):
         # TODO Put a frame around the completion to add contrast (LP: #605908)