Merge lp:~cjwatson/ubiquity/libsoup-timezone into lp:ubiquity
- libsoup-timezone
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 5006 |
Proposed branch: | lp:~cjwatson/ubiquity/libsoup-timezone |
Merge into: | lp:ubiquity |
Diff against target: |
219 lines (+112/-42) (has conflicts) 4 files modified
debian/changelog (+10/-0) debian/control (+1/-1) tests/test_timezone.py (+18/-2) ubiquity/plugins/ubi-timezone.py (+83/-39) Text conflict in debian/changelog |
To merge this branch: | bzr merge lp:~cjwatson/ubiquity/libsoup-timezone |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Evan (community) | Needs Information | ||
Review via email: mp+77324@code.launchpad.net |
Commit message
Description of the change
Fix bug 837217 by fetching geoname data asynchronously using libsoup.
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.
(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) wrote : | # |
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.
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.
- 4997. By Colin Watson
-
missing imports
- 4998. By Colin Watson
-
Show tzdb completion entries after a short timeout.
- 4999. By Colin Watson
-
need to encode text before passing to urllib.quote
- 5000. By Colin Watson
-
fix up tests
Preview Diff
1 | === modified file 'debian/changelog' | |||
2 | --- debian/changelog 2011-09-30 13:49:36 +0000 | |||
3 | +++ debian/changelog 2011-09-30 21:34:24 +0000 | |||
4 | @@ -1,3 +1,4 @@ | |||
5 | 1 | <<<<<<< TREE | ||
6 | 1 | ubiquity (2.8.0) UNRELEASED; urgency=low | 2 | ubiquity (2.8.0) UNRELEASED; urgency=low |
7 | 2 | 3 | ||
8 | 3 | * Fix SSID UTF-8 decoding, and replace any still-invalid characters with | 4 | * Fix SSID UTF-8 decoding, and replace any still-invalid characters with |
9 | @@ -29,6 +30,15 @@ | |||
10 | 29 | 30 | ||
11 | 30 | -- Adam Conrad <adconrad@ubuntu.com> Wed, 28 Sep 2011 16:43:42 -0600 | 31 | -- Adam Conrad <adconrad@ubuntu.com> Wed, 28 Sep 2011 16:43:42 -0600 |
12 | 31 | 32 | ||
13 | 33 | ======= | ||
14 | 34 | ubiquity (2.7.38) UNRELEASED; urgency=low | ||
15 | 35 | |||
16 | 36 | * GTK frontend: | ||
17 | 37 | - Fetch geoname data asynchronously (LP: #837217). | ||
18 | 38 | |||
19 | 39 | -- Colin Watson <cjwatson@ubuntu.com> Wed, 28 Sep 2011 13:43:44 +0100 | ||
20 | 40 | |||
21 | 41 | >>>>>>> MERGE-SOURCE | ||
22 | 32 | ubiquity (2.7.37) oneiric; urgency=low | 42 | ubiquity (2.7.37) oneiric; urgency=low |
23 | 33 | 43 | ||
24 | 34 | [ Evan Dandrea ] | 44 | [ Evan Dandrea ] |
25 | 35 | 45 | ||
26 | === modified file 'debian/control' | |||
27 | --- debian/control 2011-09-28 09:54:01 +0000 | |||
28 | +++ debian/control 2011-09-30 21:34:24 +0000 | |||
29 | @@ -43,7 +43,7 @@ | |||
30 | 43 | 43 | ||
31 | 44 | Package: ubiquity-frontend-gtk | 44 | Package: ubiquity-frontend-gtk |
32 | 45 | Architecture: any | 45 | Architecture: any |
34 | 46 | 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 | 46 | 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 |
35 | 47 | Suggests: gnome-control-center | 47 | Suggests: gnome-control-center |
36 | 48 | Conflicts: ubuntu-express-frontend-gtk, espresso-frontend-gtk, ubiquity (<< 2.4.3) | 48 | Conflicts: ubuntu-express-frontend-gtk, espresso-frontend-gtk, ubiquity (<< 2.4.3) |
37 | 49 | Replaces: ubuntu-express-frontend-gtk, espresso-frontend-gtk, ubiquity (<< 2.4.3) | 49 | Replaces: ubuntu-express-frontend-gtk, espresso-frontend-gtk, ubiquity (<< 2.4.3) |
38 | 50 | 50 | ||
39 | === modified file 'tests/test_timezone.py' | |||
40 | --- tests/test_timezone.py 2011-08-30 17:20:27 +0000 | |||
41 | +++ tests/test_timezone.py 2011-09-30 21:34:24 +0000 | |||
42 | @@ -19,10 +19,26 @@ | |||
43 | 19 | controller.dbfilter = self.ubi_timezone.Page(None, db=db) | 19 | controller.dbfilter = self.ubi_timezone.Page(None, db=db) |
44 | 20 | self.gtk = self.ubi_timezone.PageGtk(controller) | 20 | self.gtk = self.ubi_timezone.PageGtk(controller) |
45 | 21 | 21 | ||
46 | 22 | @mock.patch('gi.repository.Soup.SessionAsync') | ||
47 | 23 | @mock.patch('gi.repository.Soup.Message') | ||
48 | 22 | @mock.patch('json.loads') | 24 | @mock.patch('json.loads') |
51 | 23 | @mock.patch('urllib2.build_opener') | 25 | def test_city_entry(self, json_mock, *args): |
52 | 24 | def test_city_entry(self, opener_mock, json_mock): | 26 | from gi.repository import GObject |
53 | 27 | |||
54 | 28 | # Patch GObject.timeout_add_seconds to call the supplied function | ||
55 | 29 | # immediately rather than waiting for the interval to expire. | ||
56 | 30 | def side_effect_factory(real_method): | ||
57 | 31 | def side_effect(interval, function, data): | ||
58 | 32 | function(data) | ||
59 | 33 | return side_effect | ||
60 | 34 | |||
61 | 25 | json_mock.return_value = [] | 35 | json_mock.return_value = [] |
62 | 36 | real_method = GObject.timeout_add_seconds | ||
63 | 37 | method = mock.patch('gi.repository.GObject.timeout_add_seconds') | ||
64 | 38 | timeout_mock = method.start() | ||
65 | 39 | timeout_mock.side_effect = side_effect_factory(real_method) | ||
66 | 40 | self.addCleanup(method.stop) | ||
67 | 41 | |||
68 | 26 | self.gtk.set_timezone('America/New_York') | 42 | self.gtk.set_timezone('America/New_York') |
69 | 27 | self.gtk.city_entry.set_text('Eastern') | 43 | self.gtk.city_entry.set_text('Eastern') |
70 | 28 | self.gtk.changed(self.gtk.city_entry) | 44 | self.gtk.changed(self.gtk.city_entry) |
71 | 29 | 45 | ||
72 | === modified file 'ubiquity/plugins/ubi-timezone.py' | |||
73 | --- ubiquity/plugins/ubi-timezone.py 2011-08-30 17:20:27 +0000 | |||
74 | +++ ubiquity/plugins/ubi-timezone.py 2011-09-30 21:34:24 +0000 | |||
75 | @@ -52,6 +52,9 @@ | |||
76 | 52 | self.timezone = None | 52 | self.timezone = None |
77 | 53 | self.zones = [] | 53 | self.zones = [] |
78 | 54 | self.plugin_widgets = self.page | 54 | self.plugin_widgets = self.page |
79 | 55 | self.geoname_cache = {} | ||
80 | 56 | self.geoname_session = None | ||
81 | 57 | self.geoname_timeout_id = None | ||
82 | 55 | 58 | ||
83 | 56 | def plugin_translate(self, lang): | 59 | def plugin_translate(self, lang): |
84 | 57 | #c = self.controller | 60 | #c = self.controller |
85 | @@ -84,54 +87,95 @@ | |||
86 | 84 | 87 | ||
87 | 85 | 88 | ||
88 | 86 | def changed(self, entry): | 89 | def changed(self, entry): |
90 | 87 | from gi.repository import Gtk, GObject | 90 | import urllib |
91 | 91 | from gi.repository import Gtk, GObject, Soup | ||
92 | 92 | |||
93 | 88 | text = self.city_entry.get_text().decode('utf-8') | 93 | text = self.city_entry.get_text().decode('utf-8') |
94 | 89 | if not text: | 94 | if not text: |
95 | 90 | return | 95 | return |
96 | 91 | # TODO if the completion widget has a selection, return? How do we | 96 | # TODO if the completion widget has a selection, return? How do we |
97 | 92 | # determine this? | 97 | # determine this? |
100 | 93 | if text in self.changed.cache: | 98 | if text in self.geoname_cache: |
101 | 94 | model = self.changed.cache[text] | 99 | model = self.geoname_cache[text] |
102 | 100 | self.city_entry.get_completion().set_model(model) | ||
103 | 95 | else: | 101 | else: |
104 | 96 | # fetch | ||
105 | 97 | model = Gtk.ListStore(GObject.TYPE_STRING, GObject.TYPE_STRING, | 102 | model = Gtk.ListStore(GObject.TYPE_STRING, GObject.TYPE_STRING, |
106 | 98 | GObject.TYPE_STRING, GObject.TYPE_STRING, | 103 | GObject.TYPE_STRING, GObject.TYPE_STRING, |
107 | 99 | GObject.TYPE_STRING) | 104 | GObject.TYPE_STRING) |
143 | 100 | self.changed.cache[text] = model | 105 | |
144 | 101 | # TODO benchmark this | 106 | if self.geoname_session is None: |
145 | 102 | results = [(name, self.tzdb.get_loc(city)) | 107 | self.geoname_session = Soup.SessionAsync() |
146 | 103 | for (name, city) in | 108 | url = _geoname_url % (urllib.quote(text.encode('UTF-8')), |
147 | 104 | [(x[0], x[1]) for x in self.zones | 109 | misc.get_release().version) |
148 | 105 | if x[0].lower().split('(', 1)[-1] \ | 110 | message = Soup.Message.new('GET', url) |
149 | 106 | .startswith(text.lower())]] | 111 | message.request_headers.append('User-agent', 'Ubiquity/1.0') |
150 | 107 | for result in results: | 112 | self.geoname_session.abort() |
151 | 108 | # We use name rather than loc.human_zone for i18n. | 113 | if self.geoname_timeout_id is not None: |
152 | 109 | # TODO this looks pretty awful for US results: | 114 | GObject.source_remove(self.geoname_timeout_id) |
153 | 110 | # United States (New York) (United States) | 115 | self.geoname_timeout_id = \ |
154 | 111 | # Might want to match the debconf format. | 116 | GObject.timeout_add_seconds(2, self.geoname_timeout, |
155 | 112 | name, loc = result | 117 | (text, model)) |
156 | 113 | model.append([name, '', loc.human_country, | 118 | self.geoname_session.queue_message(message, self.geoname_cb, |
157 | 114 | str(loc.latitude), str(loc.longitude)]) | 119 | (text, model)) |
158 | 115 | 120 | ||
159 | 116 | try: | 121 | def geoname_add_tzdb(self, text, model): |
160 | 117 | import urllib2, urllib, json | 122 | if len(model): |
161 | 118 | opener = urllib2.build_opener() | 123 | # already added |
162 | 119 | opener.addheaders = [('User-agent', 'Ubiquity/1.0')] | 124 | return |
163 | 120 | url = opener.open(_geoname_url % | 125 | |
164 | 121 | (urllib.quote(text), misc.get_release().version)) | 126 | # TODO benchmark this |
165 | 122 | for result in json.loads(url.read()): | 127 | results = [(name, self.tzdb.get_loc(city)) |
166 | 123 | model.append([result['name'], | 128 | for (name, city) in |
167 | 124 | result['admin1'], | 129 | [(x[0], x[1]) for x in self.zones |
168 | 125 | result['country'], | 130 | if x[0].lower().split('(', 1)[-1] \ |
169 | 126 | result['latitude'], | 131 | .startswith(text.lower())]] |
170 | 127 | result['longitude']]) | 132 | for result in results: |
171 | 128 | except Exception, e: | 133 | # We use name rather than loc.human_zone for i18n. |
172 | 129 | print 'exception:', e | 134 | # TODO this looks pretty awful for US results: |
173 | 130 | # TODO because we don't return here, we could cache a | 135 | # United States (New York) (United States) |
174 | 131 | # result that doesn't include the geonames results because | 136 | # Might want to match the debconf format. |
175 | 132 | # of a network error. | 137 | name, loc = result |
176 | 133 | self.city_entry.get_completion().set_model(model) | 138 | model.append([name, '', loc.human_country, |
177 | 134 | changed.cache = {} | 139 | str(loc.latitude), str(loc.longitude)]) |
178 | 140 | |||
179 | 141 | def geoname_timeout(self, user_data): | ||
180 | 142 | text, model = user_data | ||
181 | 143 | self.geoname_add_tzdb(text, model) | ||
182 | 144 | self.geoname_timeout_id = None | ||
183 | 145 | self.city_entry.get_completion().set_model(model) | ||
184 | 146 | return False | ||
185 | 147 | |||
186 | 148 | def geoname_cb(self, session, message, user_data): | ||
187 | 149 | import syslog | ||
188 | 150 | import json | ||
189 | 151 | from gi.repository import GObject, Soup | ||
190 | 152 | |||
191 | 153 | text, model = user_data | ||
192 | 154 | |||
193 | 155 | if self.geoname_timeout_id is not None: | ||
194 | 156 | GObject.source_remove(self.geoname_timeout_id) | ||
195 | 157 | self.geoname_timeout_id = None | ||
196 | 158 | self.geoname_add_tzdb(text, model) | ||
197 | 159 | |||
198 | 160 | if message.status_code == Soup.KnownStatusCode.CANCELLED: | ||
199 | 161 | # Silently ignore cancellation. | ||
200 | 162 | pass | ||
201 | 163 | elif message.status_code != Soup.KnownStatusCode.OK: | ||
202 | 164 | # Log but otherwise ignore failures. | ||
203 | 165 | syslog.syslog('Geoname lookup for "%s" failed: %d %s' % | ||
204 | 166 | (text, message.status_code, message.reason_phrase)) | ||
205 | 167 | else: | ||
206 | 168 | for result in json.loads(message.response_body.data): | ||
207 | 169 | model.append([result['name'], | ||
208 | 170 | result['admin1'], | ||
209 | 171 | result['country'], | ||
210 | 172 | result['latitude'], | ||
211 | 173 | result['longitude']]) | ||
212 | 174 | |||
213 | 175 | # Only cache positive results. | ||
214 | 176 | self.geoname_cache[text] = model | ||
215 | 177 | |||
216 | 178 | self.city_entry.get_completion().set_model(model) | ||
217 | 135 | 179 | ||
218 | 136 | def setup_page(self): | 180 | def setup_page(self): |
219 | 137 | # TODO Put a frame around the completion to add contrast (LP: #605908) | 181 | # TODO Put a frame around the completion to add contrast (LP: #605908) |
./tests/ run-pyflakes plugins/ ubi-timezone. py:116: undefined name 'Gtk' plugins/ ubi-timezone. py:116: undefined name 'GObject' plugins/ ubi-timezone. py:116: undefined name 'GObject' plugins/ ubi-timezone. py:117: undefined name 'GObject' plugins/ ubi-timezone. py:117: undefined name 'GObject' plugins/ ubi-timezone. py:118: undefined name 'GObject'
ubiquity/
ubiquity/
ubiquity/
ubiquity/
ubiquity/
ubiquity/