Merge lp:~charlesk/indicator-datetime/lp-1074999 into lp:indicator-datetime/13.04
- lp-1074999
- Merge into trunk.13.04
Status: | Merged |
---|---|
Approved by: | Allan LeSage |
Approved revision: | 200 |
Merged at revision: | 194 |
Proposed branch: | lp:~charlesk/indicator-datetime/lp-1074999 |
Merge into: | lp:indicator-datetime/13.04 |
Diff against target: |
303 lines (+89/-87) 1 file modified
src/datetime-service.c (+89/-87) |
To merge this branch: | bzr merge lp:~charlesk/indicator-datetime/lp-1074999 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot (community) | continuous-integration | Needs Fixing | |
Lars Karlitski (community) | Approve | ||
Review via email:
|
Commit message
Don't use geoclue until the user clicks the "Show time in auto-detected location" checkbox.
Description of the change
Don't use geoclue until the user clicks the "Show time in auto-detected location" checkbox.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Lars Karlitski (larsu) wrote : | # |
Nice cleanup!
`geo_master` doesn't need to be a global variable: it's only ever used in geo_start.
Why are you calling geo_stop unconditionally in on_use_
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Charles Kerr (charlesk) wrote : | # |
> `geo_master` doesn't need to be a global variable: it's only ever used in geo_start.
The issue is that geoclue_
> Why are you calling geo_stop unconditionally in on_use_
It's to ensure that even if the callback triggered multiple calls to geo_start(), we would have a call to geo_stop() in between to guarantee that geo_master, geo_client, and geo_address were cleaned up.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Lars Karlitski (larsu) wrote : | # |
> > `geo_master` doesn't need to be a global variable: it's only ever used in
> geo_start.
>
> The issue is that geoclue_
> GeoclueMasterAs
> it. That's why datetime-service keeps its own ref.
That should be fixed in geoclue.
> > Why are you calling geo_stop unconditionally in on_use_
> put it in the `else` below, in case the callback is called even though the
> value hasn't changed (which may happen according to the docs).
>
> It's to ensure that even if the callback triggered multiple calls to
> geo_start(), we would have a call to geo_stop() in between to guarantee that
> geo_master, geo_client, and geo_address were cleaned up.
Sounds reasonable.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Allan LeSage (allanlesage) wrote : | # |
Misconfiguration foiled autolanding; re-approving to send through again.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Charles Kerr (charlesk) wrote : | # |
Thanks Allan!
Preview Diff
1 | === modified file 'src/datetime-service.c' | |||
2 | --- src/datetime-service.c 2012-11-03 16:26:05 +0000 | |||
3 | +++ src/datetime-service.c 2012-11-05 01:31:20 +0000 | |||
4 | @@ -64,11 +64,9 @@ | |||
5 | 64 | #define SETTINGS_APP_INVOCATION "gnome-control-center datetime" | 64 | #define SETTINGS_APP_INVOCATION "gnome-control-center datetime" |
6 | 65 | #endif | 65 | #endif |
7 | 66 | 66 | ||
8 | 67 | static void geo_create_client (GeoclueMaster * master, GeoclueMasterClient * client, gchar * path, GError * error, gpointer user_data); | ||
9 | 68 | static gboolean update_appointment_menu_items (gpointer user_data); | 67 | static gboolean update_appointment_menu_items (gpointer user_data); |
10 | 69 | static void update_location_menu_items (void); | 68 | static void update_location_menu_items (void); |
11 | 70 | static void day_timer_reset (void); | 69 | static void day_timer_reset (void); |
12 | 71 | static void geo_client_invalid (GeoclueMasterClient * client, gpointer user_data); | ||
13 | 72 | static gboolean get_greeter_mode (void); | 70 | static gboolean get_greeter_mode (void); |
14 | 73 | 71 | ||
15 | 74 | static void quick_set_tz (DbusmenuMenuitem * menuitem, guint timestamp, gpointer user_data); | 72 | static void quick_set_tz (DbusmenuMenuitem * menuitem, guint timestamp, gpointer user_data); |
16 | @@ -96,10 +94,6 @@ | |||
17 | 96 | static GList * appointment_sources = NULL; | 94 | static GList * appointment_sources = NULL; |
18 | 97 | 95 | ||
19 | 98 | 96 | ||
20 | 99 | /* Geoclue trackers */ | ||
21 | 100 | static GeoclueMasterClient * geo_master = NULL; | ||
22 | 101 | static GeoclueAddress * geo_address = NULL; | ||
23 | 102 | |||
24 | 103 | /* Our 2 important timezones */ | 97 | /* Our 2 important timezones */ |
25 | 104 | static gchar * current_timezone = NULL; | 98 | static gchar * current_timezone = NULL; |
26 | 105 | static gchar * geo_timezone = NULL; | 99 | static gchar * geo_timezone = NULL; |
27 | @@ -1185,31 +1179,41 @@ | |||
28 | 1185 | g_signal_connect(proxy, "g-signal", G_CALLBACK(session_active_change_cb), user_data); | 1179 | g_signal_connect(proxy, "g-signal", G_CALLBACK(session_active_change_cb), user_data); |
29 | 1186 | } | 1180 | } |
30 | 1187 | 1181 | ||
31 | 1182 | |||
32 | 1183 | /**** | ||
33 | 1184 | ***** GEOCLUE | ||
34 | 1185 | ****/ | ||
35 | 1186 | |||
36 | 1187 | static void geo_start (void); | ||
37 | 1188 | static void geo_stop (void); | ||
38 | 1189 | static void geo_create_client (GeoclueMaster * master, GeoclueMasterClient * client, gchar * path, GError * error, gpointer user_data); | ||
39 | 1190 | static void geo_client_invalid (GeoclueMasterClient * client, gpointer user_data); | ||
40 | 1191 | |||
41 | 1192 | static GeoclueMaster * geo_master = NULL; | ||
42 | 1193 | static GeoclueMasterClient * geo_client = NULL; | ||
43 | 1194 | static GeoclueAddress * geo_address = NULL; | ||
44 | 1195 | |||
45 | 1196 | static void | ||
46 | 1197 | geo_set_timezone (const gchar * timezone) | ||
47 | 1198 | { | ||
48 | 1199 | if (geo_timezone != timezone) { | ||
49 | 1200 | g_clear_pointer (&geo_timezone, g_free); | ||
50 | 1201 | geo_timezone = g_strdup (timezone); | ||
51 | 1202 | g_debug("Geoclue timezone is: %s", timezone ? timezone : "(Null)"); | ||
52 | 1203 | update_location_menu_items(); | ||
53 | 1204 | } | ||
54 | 1205 | } | ||
55 | 1206 | |||
56 | 1188 | /* Callback from getting the address */ | 1207 | /* Callback from getting the address */ |
57 | 1189 | static void | 1208 | static void |
58 | 1190 | geo_address_cb (GeoclueAddress * address, int timestamp, GHashTable * addy_data, GeoclueAccuracy * accuracy, GError * error, gpointer user_data) | 1209 | geo_address_cb (GeoclueAddress * address, int timestamp, GHashTable * addy_data, GeoclueAccuracy * accuracy, GError * error, gpointer user_data) |
59 | 1191 | { | 1210 | { |
61 | 1192 | if (error != NULL) { | 1211 | if (error == NULL) { |
62 | 1212 | geo_set_timezone (g_hash_table_lookup (addy_data, "timezone")); | ||
63 | 1213 | } else { | ||
64 | 1193 | g_warning("Unable to get Geoclue address: %s", error->message); | 1214 | g_warning("Unable to get Geoclue address: %s", error->message); |
65 | 1194 | g_clear_error (&error); | 1215 | g_clear_error (&error); |
84 | 1195 | return; | 1216 | } |
67 | 1196 | } | ||
68 | 1197 | |||
69 | 1198 | g_debug("Geoclue timezone is: %s", (gchar *)g_hash_table_lookup(addy_data, "timezone")); | ||
70 | 1199 | |||
71 | 1200 | if (geo_timezone != NULL) { | ||
72 | 1201 | g_free(geo_timezone); | ||
73 | 1202 | geo_timezone = NULL; | ||
74 | 1203 | } | ||
75 | 1204 | |||
76 | 1205 | gpointer tz_hash = g_hash_table_lookup(addy_data, "timezone"); | ||
77 | 1206 | if (tz_hash != NULL) { | ||
78 | 1207 | geo_timezone = g_strdup((gchar *)tz_hash); | ||
79 | 1208 | } | ||
80 | 1209 | |||
81 | 1210 | update_location_menu_items(); | ||
82 | 1211 | |||
83 | 1212 | return; | ||
85 | 1213 | } | 1217 | } |
86 | 1214 | 1218 | ||
87 | 1215 | /* Clean up the reference we kept to the address and make sure to | 1219 | /* Clean up the reference we kept to the address and make sure to |
88 | @@ -1217,16 +1221,10 @@ | |||
89 | 1217 | static void | 1221 | static void |
90 | 1218 | geo_address_clean (void) | 1222 | geo_address_clean (void) |
91 | 1219 | { | 1223 | { |
94 | 1220 | if (geo_address == NULL) { | 1224 | if (geo_address != NULL) { |
95 | 1221 | return; | 1225 | g_signal_handlers_disconnect_by_func (geo_address, geo_address_cb, NULL); |
96 | 1226 | g_clear_object (&geo_address); | ||
97 | 1222 | } | 1227 | } |
98 | 1223 | |||
99 | 1224 | g_signal_handlers_disconnect_by_func(G_OBJECT(geo_address), geo_address_cb, NULL); | ||
100 | 1225 | g_object_unref(G_OBJECT(geo_address)); | ||
101 | 1226 | |||
102 | 1227 | geo_address = NULL; | ||
103 | 1228 | |||
104 | 1229 | return; | ||
105 | 1230 | } | 1228 | } |
106 | 1231 | 1229 | ||
107 | 1232 | /* Clean up and remove all signal handlers from the client as we | 1230 | /* Clean up and remove all signal handlers from the client as we |
108 | @@ -1234,16 +1232,10 @@ | |||
109 | 1234 | static void | 1232 | static void |
110 | 1235 | geo_client_clean (void) | 1233 | geo_client_clean (void) |
111 | 1236 | { | 1234 | { |
114 | 1237 | if (geo_master == NULL) { | 1235 | if (geo_client != NULL) { |
115 | 1238 | return; | 1236 | g_signal_handlers_disconnect_by_func (geo_client, geo_client_invalid, NULL); |
116 | 1237 | g_clear_object (&geo_client); | ||
117 | 1239 | } | 1238 | } |
118 | 1240 | |||
119 | 1241 | g_signal_handlers_disconnect_by_func(G_OBJECT(geo_master), geo_client_invalid, NULL); | ||
120 | 1242 | g_object_unref(G_OBJECT(geo_master)); | ||
121 | 1243 | |||
122 | 1244 | geo_master = NULL; | ||
123 | 1245 | |||
124 | 1246 | return; | ||
125 | 1247 | } | 1239 | } |
126 | 1248 | 1240 | ||
127 | 1249 | /* Callback from creating the address */ | 1241 | /* Callback from creating the address */ |
128 | @@ -1263,14 +1255,11 @@ | |||
129 | 1263 | geo_address_clean(); | 1255 | geo_address_clean(); |
130 | 1264 | 1256 | ||
131 | 1265 | g_debug("Created Geoclue Address"); | 1257 | g_debug("Created Geoclue Address"); |
140 | 1266 | geo_address = address; | 1258 | geo_address = g_object_ref (address); |
141 | 1267 | g_object_ref(G_OBJECT(geo_address)); | 1259 | |
142 | 1268 | 1260 | geoclue_address_get_address_async (geo_address, geo_address_cb, NULL); | |
143 | 1269 | geoclue_address_get_address_async(geo_address, geo_address_cb, NULL); | 1261 | |
144 | 1270 | 1262 | g_signal_connect (address, "address-changed", G_CALLBACK(geo_address_cb), NULL); | |
137 | 1271 | g_signal_connect(G_OBJECT(address), "address-changed", G_CALLBACK(geo_address_cb), NULL); | ||
138 | 1272 | |||
139 | 1273 | return; | ||
145 | 1274 | } | 1263 | } |
146 | 1275 | 1264 | ||
147 | 1276 | /* Callback from setting requirements */ | 1265 | /* Callback from setting requirements */ |
148 | @@ -1281,7 +1270,6 @@ | |||
149 | 1281 | g_warning("Unable to set Geoclue requirements: %s", error->message); | 1270 | g_warning("Unable to set Geoclue requirements: %s", error->message); |
150 | 1282 | g_clear_error (&error); | 1271 | g_clear_error (&error); |
151 | 1283 | } | 1272 | } |
152 | 1284 | return; | ||
153 | 1285 | } | 1273 | } |
154 | 1286 | 1274 | ||
155 | 1287 | /* Client is killing itself rather oddly */ | 1275 | /* Client is killing itself rather oddly */ |
156 | @@ -1289,25 +1277,28 @@ | |||
157 | 1289 | geo_client_invalid (GeoclueMasterClient * client, gpointer user_data) | 1277 | geo_client_invalid (GeoclueMasterClient * client, gpointer user_data) |
158 | 1290 | { | 1278 | { |
159 | 1291 | g_warning("Master client invalid, rebuilding."); | 1279 | g_warning("Master client invalid, rebuilding."); |
179 | 1292 | 1280 | geo_stop (); | |
180 | 1293 | /* Client changes we can assume the address is now invalid so we | 1281 | geo_start (); |
181 | 1294 | need to unreference the one we had. */ | 1282 | } |
182 | 1295 | geo_address_clean(); | 1283 | |
183 | 1296 | 1284 | static void | |
184 | 1297 | /* And our master client is invalid */ | 1285 | geo_stop (void) |
185 | 1298 | geo_client_clean(); | 1286 | { |
186 | 1299 | 1287 | geo_set_timezone (NULL); | |
187 | 1300 | GeoclueMaster * master = geoclue_master_get_default(); | 1288 | |
188 | 1301 | geoclue_master_create_client_async(master, geo_create_client, NULL); | 1289 | geo_address_clean (); |
189 | 1302 | 1290 | geo_client_clean (); | |
190 | 1303 | if (geo_timezone != NULL) { | 1291 | g_clear_object (&geo_master); |
191 | 1304 | g_free(geo_timezone); | 1292 | } |
192 | 1305 | geo_timezone = NULL; | 1293 | |
193 | 1306 | } | 1294 | static void |
194 | 1307 | 1295 | geo_start (void) | |
195 | 1308 | update_location_menu_items(); | 1296 | { |
196 | 1309 | 1297 | g_warn_if_fail (geo_master == NULL); | |
197 | 1310 | return; | 1298 | |
198 | 1299 | g_clear_object (&geo_master); | ||
199 | 1300 | geo_master = geoclue_master_get_default(); | ||
200 | 1301 | geoclue_master_create_client_async (geo_master, geo_create_client, NULL); | ||
201 | 1311 | } | 1302 | } |
202 | 1312 | 1303 | ||
203 | 1313 | /* Callback from creating the client */ | 1304 | /* Callback from creating the client */ |
204 | @@ -1316,7 +1307,7 @@ | |||
205 | 1316 | { | 1307 | { |
206 | 1317 | g_debug("Created Geoclue client at: %s", path); | 1308 | g_debug("Created Geoclue client at: %s", path); |
207 | 1318 | 1309 | ||
209 | 1319 | geo_master = client; | 1310 | geo_client = client; |
210 | 1320 | 1311 | ||
211 | 1321 | if (error != NULL) { | 1312 | if (error != NULL) { |
212 | 1322 | g_warning("Unable to get a GeoClue client! '%s' Geolocation based timezone support will not be available.", error->message); | 1313 | g_warning("Unable to get a GeoClue client! '%s' Geolocation based timezone support will not be available.", error->message); |
213 | @@ -1324,17 +1315,17 @@ | |||
214 | 1324 | return; | 1315 | return; |
215 | 1325 | } | 1316 | } |
216 | 1326 | 1317 | ||
218 | 1327 | if (geo_master == NULL) { | 1318 | if (client == NULL) { |
219 | 1328 | g_warning(_("Unable to get a GeoClue client! Geolocation based timezone support will not be available.")); | 1319 | g_warning(_("Unable to get a GeoClue client! Geolocation based timezone support will not be available.")); |
220 | 1329 | return; | 1320 | return; |
221 | 1330 | } | 1321 | } |
222 | 1331 | 1322 | ||
224 | 1332 | g_object_ref(G_OBJECT(geo_master)); | 1323 | g_object_ref (geo_client); |
225 | 1333 | 1324 | ||
226 | 1334 | /* New client, make sure we don't have an address hanging on */ | 1325 | /* New client, make sure we don't have an address hanging on */ |
227 | 1335 | geo_address_clean(); | 1326 | geo_address_clean(); |
228 | 1336 | 1327 | ||
230 | 1337 | geoclue_master_client_set_requirements_async(geo_master, | 1328 | geoclue_master_client_set_requirements_async(geo_client, |
231 | 1338 | GEOCLUE_ACCURACY_LEVEL_REGION, | 1329 | GEOCLUE_ACCURACY_LEVEL_REGION, |
232 | 1339 | 0, | 1330 | 0, |
233 | 1340 | FALSE, | 1331 | FALSE, |
234 | @@ -1342,12 +1333,23 @@ | |||
235 | 1342 | geo_req_set, | 1333 | geo_req_set, |
236 | 1343 | NULL); | 1334 | NULL); |
237 | 1344 | 1335 | ||
244 | 1345 | geoclue_master_client_create_address_async(geo_master, geo_create_address, NULL); | 1336 | geoclue_master_client_create_address_async(geo_client, geo_create_address, NULL); |
245 | 1346 | 1337 | ||
246 | 1347 | g_signal_connect(G_OBJECT(client), "invalidated", G_CALLBACK(geo_client_invalid), NULL); | 1338 | g_signal_connect(client, "invalidated", G_CALLBACK(geo_client_invalid), NULL); |
247 | 1348 | 1339 | } | |
248 | 1349 | return; | 1340 | |
249 | 1350 | } | 1341 | static void |
250 | 1342 | on_use_geoclue_changed_cb (GSettings * settings, gchar * key, gpointer unused G_GNUC_UNUSED) | ||
251 | 1343 | { | ||
252 | 1344 | geo_stop (); | ||
253 | 1345 | |||
254 | 1346 | if (g_settings_get_boolean (conf, SETTINGS_SHOW_DETECTED_S)) | ||
255 | 1347 | geo_start (); | ||
256 | 1348 | } | ||
257 | 1349 | |||
258 | 1350 | /**** | ||
259 | 1351 | ***** | ||
260 | 1352 | ****/ | ||
261 | 1351 | 1353 | ||
262 | 1352 | static gboolean | 1354 | static gboolean |
263 | 1353 | get_greeter_mode (void) | 1355 | get_greeter_mode (void) |
264 | @@ -1413,7 +1415,9 @@ | |||
265 | 1413 | 1415 | ||
266 | 1414 | /* Set up GSettings */ | 1416 | /* Set up GSettings */ |
267 | 1415 | conf = g_settings_new(SETTINGS_INTERFACE); | 1417 | conf = g_settings_new(SETTINGS_INTERFACE); |
269 | 1416 | // TODO Add a signal handler to catch gsettings changes and respond to them | 1418 | g_signal_connect (conf, "changed::show-auto-detected-location", |
270 | 1419 | G_CALLBACK(on_use_geoclue_changed_cb), NULL); | ||
271 | 1420 | // TODO Add a signal handler to catch other gsettings changes and respond to them | ||
272 | 1417 | 1421 | ||
273 | 1418 | /* Build our list of appointment calendar sources. | 1422 | /* Build our list of appointment calendar sources. |
274 | 1419 | When a source changes, update our menu items. | 1423 | When a source changes, update our menu items. |
275 | @@ -1439,8 +1443,8 @@ | |||
276 | 1439 | update_current_timezone(); | 1443 | update_current_timezone(); |
277 | 1440 | 1444 | ||
278 | 1441 | /* Setup geoclue */ | 1445 | /* Setup geoclue */ |
281 | 1442 | GeoclueMaster * master = geoclue_master_get_default(); | 1446 | if (g_settings_get_boolean (conf, SETTINGS_SHOW_DETECTED_S)) |
282 | 1443 | geoclue_master_create_client_async(master, geo_create_client, NULL); | 1447 | geo_start (); |
283 | 1444 | 1448 | ||
284 | 1445 | /* Setup dbus interface */ | 1449 | /* Setup dbus interface */ |
285 | 1446 | dbus = g_object_new(DATETIME_INTERFACE_TYPE, NULL); | 1450 | dbus = g_object_new(DATETIME_INTERFACE_TYPE, NULL); |
286 | @@ -1471,7 +1475,6 @@ | |||
287 | 1471 | free_appointment_sources(); | 1475 | free_appointment_sources(); |
288 | 1472 | 1476 | ||
289 | 1473 | g_object_unref(G_OBJECT(conf)); | 1477 | g_object_unref(G_OBJECT(conf)); |
290 | 1474 | g_object_unref(G_OBJECT(master)); | ||
291 | 1475 | g_object_unref(G_OBJECT(dbus)); | 1478 | g_object_unref(G_OBJECT(dbus)); |
292 | 1476 | g_object_unref(G_OBJECT(service)); | 1479 | g_object_unref(G_OBJECT(service)); |
293 | 1477 | g_object_unref(G_OBJECT(server)); | 1480 | g_object_unref(G_OBJECT(server)); |
294 | @@ -1480,8 +1483,7 @@ | |||
295 | 1480 | 1483 | ||
296 | 1481 | icaltimezone_free_builtin_timezones(); | 1484 | icaltimezone_free_builtin_timezones(); |
297 | 1482 | 1485 | ||
300 | 1483 | geo_address_clean(); | 1486 | geo_stop (); |
299 | 1484 | geo_client_clean(); | ||
301 | 1485 | 1487 | ||
302 | 1486 | return 0; | 1488 | return 0; |
303 | 1487 | } | 1489 | } |
PASSED: Continuous integration, rev:200 jenkins. qa.ubuntu. com/job/ indicator- datetime- ci/22/ jenkins. qa.ubuntu. com/job/ indicator- datetime- ci/./build= pbuilder, distribution= quantal, flavor= amd64/22/ console jenkins. qa.ubuntu. com/job/ indicator- datetime- ci/./build= pbuilder, distribution= quantal, flavor= i386/22/ console
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://