ubuntu-system-settings

Merge lp:~mterry/ubuntu-system-settings/geonames into lp:ubuntu-system-settings

  1. geonames
  2. Merge into trunk
Proposed by Michael Terry
Status: Merged
Approved by: Michael Terry
Approved revision: 1604
Merged at revision: 1617
Proposed branch: lp:~mterry/ubuntu-system-settings/geonames
Merge into: lp:ubuntu-system-settings
Diff against target: 443 lines (+107/-199)
6 files modified
CMakeLists.txt (+1/-1)
debian/control (+1/-1)
plugins/time-date/CMakeLists.txt (+4/-2)
plugins/time-date/timedate.cpp (+5/-1)
plugins/time-date/timezonelocationmodel.cpp (+86/-142)
plugins/time-date/timezonelocationmodel.h (+10/-52)
To merge this branch: bzr merge lp:~mterry/ubuntu-system-settings/geonames
Reviewer Review Type Date Requested Status
Jonas G. Drange (community) Approve
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+287064@code.launchpad.net

Commit message

Use geonames library instead of timezonemap library to search city names, which gives us slightly better sorting (and future improvements)

Description of the change

The Desktop team has decided to maintain a new timezone library (geonames) instead of its old one (timezonemap).

They've already ported unity-system-settings to it. This ports ubuntu-system-settings. I'll also later propose a port for the new unity8 wizard.

Benefits of this new MP:
- No gtk dependency (timezonemap has one)
- No complicated threading in USS now, geonames does that for us
- No time spent parsing timezone data (geonames keeps it in binary form inside the library itself)
- Fixed bug where the activity indicator didn't show up while filtering (it only showed when initially parsing timezone data)
- Results are a little more sensible, since they are now sorted by population size
- Results are a little more sensible, since matching is now done on the beginning of city names, not anywhere inside the string. But that's subjective

Note that geonames isn't in vivid (or stable-phone-overlay) yet. It's only in xenial. When putting this in a silo, we need to add geonames.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1599
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2620/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6621/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/394/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/803/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6632/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2620/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1600
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2621/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6625/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/395/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/804/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6636/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2621/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

Whoops, noticed that if you type / backspace very fast, sometimes no results appear. Working on it.

Revision history for this message
Michael Terry (mterry) wrote :

OK, that bug was just an issue with geonames having a double-free.

https://code.launchpad.net/~mterry/geonames/+git/double-free/+merge/287234

I added some more cleanup to this branch, but otherwise I think it's still fine. (Assuming the above fix lands.)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1601
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2625/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6653/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/399/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/808/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6664/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2625/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

This is wonderful. Do you have debs or maybe a preliminary silo for this?

review: Needs Information
lp:~mterry/ubuntu-system-settings/geonames updated
1602. By Michael Terry

Remove extern c bit, geonames now has the fix

Revision history for this message
Michael Terry (mterry) wrote :

Good point. I just set up silo 46 to hold this and the required geonames branches.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1602
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2626/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6672/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/400/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/809/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6683/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2626/rebuild

review: Needs Fixing (continuous-integration)
lp:~mterry/ubuntu-system-settings/geonames updated
1603. By Michael Terry

Fix func def

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1603
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2628/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6686/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/402/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/811/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6697/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2628/rebuild

review: Needs Fixing (continuous-integration)
lp:~mterry/ubuntu-system-settings/geonames updated
1604. By Michael Terry

Revert accidental pot changes

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1604
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/2629/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6688/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-amd64-ci/404/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-vivid-i386-ci/813/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6699/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/2629/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

Works well on the phone (no lag when searching) and the first item in the result set is usually the correct one. Thanks!

review: Approve
Revision history for this message
Michael Terry (mterry) wrote :

I'll top approve this then?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2015-11-05 21:25:13 +0000
3+++ CMakeLists.txt 2016-03-01 15:22:25 +0000
4@@ -39,7 +39,7 @@
5 pkg_search_module(UPOWER_GLIB REQUIRED upower-glib)
6 pkg_search_module(LIBNM_GLIB REQUIRED libnm-glib)
7 pkg_search_module(ACCOUNTSSERVICE REQUIRED accountsservice)
8-pkg_search_module(TIMEZONEMAP REQUIRED timezonemap)
9+pkg_search_module(GEONAMES REQUIRED geonames)
10 pkg_search_module(ICU REQUIRED icu-i18n)
11 pkg_search_module(ANDR_PROP libandroid-properties)
12 pkg_check_modules(QTDBUSMOCK REQUIRED libqtdbusmock-1 REQUIRED)
13
14=== modified file 'debian/control'
15--- debian/control 2016-02-03 20:58:04 +0000
16+++ debian/control 2016-03-01 15:22:25 +0000
17@@ -13,13 +13,13 @@
18 libaccountsservice-dev,
19 libandroid-properties-dev [amd64 armhf i386],
20 libclick-0.4-dev,
21+ libgeonames-dev,
22 libglib2.0-dev (>= 2.37.92),
23 libevdev-dev,
24 libicu-dev,
25 libnm-glib-dev,
26 libpolkit-agent-1-dev,
27 libqmenumodel-dev,
28- libtimezonemap1-dev (>= 0.4.1),
29 libtrust-store-dev,
30 libudev-dev,
31 libunity-api-dev,
32
33=== modified file 'plugins/time-date/CMakeLists.txt'
34--- plugins/time-date/CMakeLists.txt 2014-04-22 13:35:21 +0000
35+++ plugins/time-date/CMakeLists.txt 2016-03-01 15:22:25 +0000
36@@ -1,4 +1,6 @@
37-include_directories(${GLIB_INCLUDE_DIRS} ${TIMEZONEMAP_INCLUDE_DIRS})
38+include_directories(${GLIB_INCLUDE_DIRS} ${GEONAMES_INCLUDE_DIRS})
39+
40+add_definitions(-DQT_NO_KEYWORDS)
41
42 set(QML_SOURCES ChooseTimeZone.qml PageComponent.qml Scroller.qml TimePicker.qml)
43
44@@ -11,7 +13,7 @@
45 ${QML_SOURCES}
46 )
47 qt5_use_modules(UbuntuTimeDatePanel Qml Quick DBus Concurrent)
48-target_link_libraries(UbuntuTimeDatePanel ${GLIB_LDFLAGS} ${TIMEZONEMAP_LDFLAGS})
49+target_link_libraries(UbuntuTimeDatePanel ${GLIB_LDFLAGS} ${GEONAMES_LDFLAGS})
50
51
52 set(PLUG_DIR ${PLUGIN_PRIVATE_MODULE_DIR}/Ubuntu/SystemSettings/TimeDate)
53
54=== modified file 'plugins/time-date/timedate.cpp'
55--- plugins/time-date/timedate.cpp 2014-12-08 17:28:29 +0000
56+++ plugins/time-date/timedate.cpp 2016-03-01 15:22:25 +0000
57@@ -23,7 +23,6 @@
58 #include <QEvent>
59 #include <glib.h>
60 #include <glib-object.h>
61-#include <timezonemap/tz.h>
62 #include <unistd.h>
63
64 #include <iostream>
65@@ -45,6 +44,11 @@
66 this,
67 SLOT (slotNameOwnerChanged (QString, QString, QString)));
68
69+ connect (&m_timeZoneModel, SIGNAL (filterBegin ()),
70+ this, SIGNAL (listUpdatingChanged ()));
71+ connect (&m_timeZoneModel, SIGNAL (filterComplete ()),
72+ this, SIGNAL (listUpdatingChanged ()));
73+
74 m_useNTP = getUseNTP();
75
76 setUpInterface();
77
78=== modified file 'plugins/time-date/timezonelocationmodel.cpp'
79--- plugins/time-date/timezonelocationmodel.cpp 2015-09-11 14:55:40 +0000
80+++ plugins/time-date/timezonelocationmodel.cpp 2016-03-01 15:22:25 +0000
81@@ -21,68 +21,26 @@
82 #include "timezonelocationmodel.h"
83 #include <glib.h>
84 #include <glib-object.h>
85-#include <timezonemap/tz.h>
86
87 #include <QDebug>
88
89 TimeZoneLocationModel::TimeZoneLocationModel(QObject *parent):
90 QAbstractTableModel(parent),
91- modelUpdating(true),
92- m_pattern(),
93- m_workerThread(new QThread()),
94- m_populateWorker(new TimeZonePopulateWorker()),
95- m_watcher()
96-{
97- qRegisterMetaType<TzLocation>();
98- qRegisterMetaType<QList<TimeZoneLocationModel::TzLocation> >();
99-
100- QObject::connect(m_workerThread,
101- SIGNAL(started()),
102- m_populateWorker,
103- SLOT(run()));
104- QObject::connect(m_populateWorker,
105- &TimeZonePopulateWorker::resultReady,
106- this,
107- &TimeZoneLocationModel::store);
108- QObject::connect(m_workerThread,
109- SIGNAL(finished()),
110- m_workerThread,
111- SLOT(deleteLater()));
112- QObject::connect(m_populateWorker,
113- SIGNAL(finished()),
114- m_populateWorker,
115- SLOT(deleteLater()));
116-
117- m_populateWorker->moveToThread(m_workerThread);
118- m_workerThread->start(QThread::IdlePriority);
119-}
120-
121-void TimeZoneLocationModel::store(QList<TzLocation> sortedLocations)
122-{
123- m_originalLocations = sortedLocations;
124- m_workerThread = nullptr;
125- modelUpdating = false;
126-
127- QObject::connect(&m_watcher,
128- &QFutureWatcher<TzLocation>::finished,
129- this,
130- &TimeZoneLocationModel::filterFinished);
131-
132- if (!m_pattern.isEmpty())
133- filter(m_pattern);
134-}
135-
136-void TimeZoneLocationModel::setModel(QList<TzLocation> locations)
137+ modelUpdating(false),
138+ m_cancellable(nullptr)
139+{
140+}
141+
142+void TimeZoneLocationModel::setModel(const QList<GeonamesCity *> &locations)
143 {
144 beginResetModel();
145+
146+ Q_FOREACH(GeonamesCity *city, m_locations) {
147+ geonames_city_free(city);
148+ }
149+
150 m_locations = locations;
151 endResetModel();
152- Q_EMIT(filterComplete());
153-}
154-
155-void TimeZoneLocationModel::filterFinished()
156-{
157- setModel(m_watcher.future().results());
158 }
159
160 int TimeZoneLocationModel::rowCount(const QModelIndex &parent) const
161@@ -105,29 +63,26 @@
162 index.row() < 0)
163 return QVariant();
164
165- TzLocation tz = m_locations[index.row()];
166-
167- QString country(tz.full_country.isEmpty() ? tz.country : tz.full_country);
168+ GeonamesCity *city = m_locations[index.row()];
169
170 switch (role) {
171 case Qt::DisplayRole:
172- if (!tz.state.isEmpty())
173- return QVariant(QString("%1, %2, %3").arg(tz.city).arg(tz.state)
174- .arg(country));
175- else
176- return QVariant(QString("%1, %2").arg(tz.city).arg(country));
177+ return QVariant(QString("%1, %2, %3").arg(geonames_city_get_name(city))
178+ .arg(geonames_city_get_state(city))
179+ .arg(geonames_city_get_country(city)));
180 break;
181 case SimpleRole:
182- return QVariant(QString("%1, %2").arg(tz.city).arg(country));
183+ return QVariant(QString("%1, %2").arg(geonames_city_get_name(city))
184+ .arg(geonames_city_get_country(city)));
185 break;
186 case TimeZoneRole:
187- return tz.timezone;
188+ return geonames_city_get_timezone(city);
189 break;
190 case CountryRole:
191- return tz.country;
192+ return geonames_city_get_country(city);
193 break;
194 case CityRole:
195- return tz.city;
196+ return geonames_city_get_name(city);
197 break;
198 default:
199 return QVariant();
200@@ -147,90 +102,79 @@
201 return m_roleNames;
202 }
203
204+void TimeZoneLocationModel::filterFinished(GObject *source_object,
205+ GAsyncResult *res,
206+ gpointer user_data)
207+{
208+ Q_UNUSED(source_object);
209+
210+ g_autofree gint *cities = nullptr;
211+ guint cities_len = 0;
212+ g_autoptr(GError) error = nullptr;
213+
214+ cities = geonames_query_cities_finish(res, &cities_len, &error);
215+ if (error) {
216+ if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
217+ TimeZoneLocationModel *model = static_cast<TimeZoneLocationModel *>(user_data);
218+ g_clear_object(&model->m_cancellable);
219+ qWarning() << "Could not filter timezones:" << error->message;
220+ }
221+ return;
222+ }
223+
224+ QList<GeonamesCity *> locations;
225+
226+ for (guint i = 0; i < cities_len; ++i) {
227+ GeonamesCity *city = geonames_get_city(cities[i]);
228+ if (city) {
229+ locations.append(city);
230+ }
231+ }
232+
233+ TimeZoneLocationModel *model = static_cast<TimeZoneLocationModel *>(user_data);
234+
235+ g_clear_object(&model->m_cancellable);
236+
237+ model->setModel(locations);
238+ model->modelUpdating = false;
239+
240+ Q_EMIT model->filterComplete();
241+}
242+
243 void TimeZoneLocationModel::filter(const QString& pattern)
244 {
245- QList<TzLocation> list;
246-
247- if (m_watcher.isRunning())
248- m_watcher.cancel();
249-
250- if (pattern.isEmpty() || pattern.isNull() ||
251- (m_workerThread && m_workerThread->isRunning())) {
252- setModel(QList<TzLocation>());
253- m_pattern = pattern;
254+ modelUpdating = true;
255+ Q_EMIT filterBegin();
256+
257+ if (m_cancellable) {
258+ g_cancellable_cancel(m_cancellable);
259+ g_clear_object(&m_cancellable);
260+ }
261+
262+ setModel(QList<GeonamesCity *>());
263+
264+ if (pattern.isEmpty()) {
265+ modelUpdating = false;
266+ Q_EMIT filterComplete();
267 return;
268 }
269
270- if (!m_pattern.isEmpty() && !m_locations.isEmpty() &&
271- pattern.startsWith(m_pattern))
272- list = m_locations; // search in the smaller list
273- else
274- list = m_originalLocations; //search in the whole list
275-
276- m_pattern = pattern;
277-
278- QFuture<TzLocation> future (QtConcurrent::filtered(
279- list,
280- [pattern] (const TzLocation& tz) {
281- QString display("%1, %2");
282- return g_str_match_string (pattern.toStdString().c_str(),
283- display.arg(tz.city)
284- .arg(tz.full_country.isEmpty() ? tz.country
285- : tz.full_country)
286- .toStdString().c_str(),
287- TRUE);
288- }));
289-
290- Q_EMIT (filterBegin());
291-
292- m_watcher.setFuture(future);
293-}
294-
295-void TimeZonePopulateWorker::run()
296-{
297- buildCityMap();
298- emit finished();
299-}
300-
301-void TimeZonePopulateWorker::buildCityMap()
302-{
303- QList<TimeZoneLocationModel::TzLocation> locations;
304- TzDB *tzdb = tz_load_db();
305- GPtrArray *tz_locations = tz_get_locations(tzdb);
306-
307- TimeZoneLocationModel::TzLocation tmpTz;
308-
309- for (guint i = 0; i < tz_locations->len; ++i) {
310- auto tmp = static_cast<CcTimezoneLocation *>(g_ptr_array_index(tz_locations, i));
311- gchar *en_name, *country, *zone, *state, *full_country;
312- g_object_get (tmp, "en_name", &en_name,
313- "country", &country,
314- "zone", &zone,
315- "state", &state,
316- "full_country", &full_country,
317- nullptr);
318- // There are empty entries in the DB
319- if (g_strcmp0(en_name, "") != 0) {
320- tmpTz.city = en_name;
321- tmpTz.country = country;
322- tmpTz.timezone = zone;
323- tmpTz.state = state;
324- tmpTz.full_country = full_country;
325- }
326- g_free (en_name);
327- g_free (country);
328- g_free (zone);
329- g_free (state);
330- g_free (full_country);
331-
332- locations.append(tmpTz);
333- }
334-
335- qSort(locations.begin(), locations.end());
336- Q_EMIT (resultReady(locations));
337- g_ptr_array_free (tz_locations, TRUE);
338+ m_cancellable = g_cancellable_new();
339+ geonames_query_cities(pattern.toUtf8().data(),
340+ GEONAMES_QUERY_DEFAULT,
341+ m_cancellable,
342+ filterFinished,
343+ this);
344 }
345
346 TimeZoneLocationModel::~TimeZoneLocationModel()
347 {
348+ if (m_cancellable) {
349+ g_cancellable_cancel(m_cancellable);
350+ g_clear_object(&m_cancellable);
351+ }
352+
353+ Q_FOREACH(GeonamesCity *city, m_locations) {
354+ geonames_city_free(city);
355+ }
356 }
357
358=== modified file 'plugins/time-date/timezonelocationmodel.h'
359--- plugins/time-date/timezonelocationmodel.h 2015-09-11 14:48:06 +0000
360+++ plugins/time-date/timezonelocationmodel.h 2016-03-01 15:22:25 +0000
361@@ -27,7 +27,7 @@
362
363 #include <QtConcurrent>
364
365-class TimeZonePopulateWorker;
366+#include <geonames.h>
367
368 class TimeZoneLocationModel : public QAbstractTableModel
369 {
370@@ -44,22 +44,6 @@
371 SimpleRole
372 };
373
374- struct TzLocation {
375- bool operator<(const TzLocation &other) const
376- {
377- QString pattern("%1, %2");
378-
379- return pattern.arg(city).arg(country) <
380- pattern.arg(other.city).arg(other.country);
381- }
382-
383- QString city;
384- QString country;
385- QString timezone;
386- QString state;
387- QString full_country;
388- };
389-
390 void filter(const QString& pattern);
391
392 // implemented virtual methods from QAbstractTableModel
393@@ -73,41 +57,15 @@
394 Q_SIGNALS:
395 void filterBegin();
396 void filterComplete();
397- void modelUpdated();
398-
399-public Q_SLOTS:
400- void store(QList<TzLocation> sortedLocations);
401- void filterFinished();
402-
403-private:
404- QList<TzLocation> m_locations;
405- QList<TzLocation> m_originalLocations;
406- QString m_pattern;
407-
408- QThread *m_workerThread;
409- TimeZonePopulateWorker *m_populateWorker;
410-
411- bool substringFilter(const QString& input);
412- QFutureWatcher<TzLocation> m_watcher;
413- void setModel(QList<TzLocation> locations);
414-};
415-
416-Q_DECLARE_METATYPE (TimeZoneLocationModel::TzLocation)
417-
418-class TimeZonePopulateWorker : public QObject
419-{
420- Q_OBJECT
421-
422-public slots:
423- void run();
424-
425-Q_SIGNALS:
426- void resultReady(const QList<TimeZoneLocationModel::TzLocation> &sortedList);
427- void finished();
428-
429-private:
430- void buildCityMap();
431-
432+
433+private:
434+ QList<GeonamesCity *> m_locations;
435+ GCancellable *m_cancellable;
436+
437+ static void filterFinished(GObject *source_object,
438+ GAsyncResult *res,
439+ gpointer user_data);
440+ void setModel(const QList<GeonamesCity *> &locations);
441 };
442
443 #endif // TIMEZONELOCATIONMODEL_H

Subscribers

People subscribed via source and target branches