GNOME Bugzilla – Bug 756647
CurrentLocationController: Use new geoclue library
Last modified: 2015-10-27 14:03:48 UTC
Here is a small gift. :) I have tested it to work but more testing always welcome. I'll be rolling out a release before 3.19.1 tarballs due date so you can already merge it.
Created attachment 313380 [details] [review] CurrentLocationController: Use new geoclue library Make use of new Geoclue convenience library to get rid of most of the code related to fetching current location.
Review of attachment 313380 [details] [review]: Thanks for the patch, it looks good and I'll merge it before 3.19.1 Don't mind the comments, they are mostly for me to remember before I merge it. ::: src/app/currentLocationController.js @@ +21,3 @@ const Lang = imports.lang; const GWeather = imports.gi.GWeather; +const Geoclue = imports.gi.Geoclue; It would be nice to mention the dependency in configure.ac @@ +71,3 @@ + let client = this._simple.get_client(); + client.distance_threshold = 100; Out of curiosity, why is this configured differently from the other parameters? And does it matter, given that we give the accuracy level? @@ +120,3 @@ } } else { + this._simple.disconnectSignal(this._locationUpdatedId); This should be a disconnect - disconnectSignal is for DBus signals.
Review of attachment 313380 [details] [review]: ::: src/app/currentLocationController.js @@ +21,3 @@ const Lang = imports.lang; const GWeather = imports.gi.GWeather; +const Geoclue = imports.gi.Geoclue; I think it's very wrong to require runtime deps at buildtime. configure is supposed to be only to check for buildtime deps, not runtime. @@ +71,3 @@ + let client = this._simple.get_client(); + client.distance_threshold = 100; > why is this configured differently from the other parameters? You mean why it's not part of Simple API? The accuracy_level and desktop_id had to be properties of Simple to get it passed to it's async construction machinery, otherwise they wouldn't be part of it. They need to be set at construct on Simple cause they need to be set on client before it is started and client is started as part of Simple construction. I hope I made sense. :) Oh and this parameter is not set by most apps so that's another reason not to have it as part of Simple. > And does it matter, given that we give the accuracy level? Not sure if it would matter in practice to the case of very low accuracy of "city-level" but just cause an app need/want very high accuracy, does not necessarily mean it would need to be notified for every small location change. Hence this parameter is provided. I'm only setting it cause the code I replaced was doing so. @@ +120,3 @@ } } else { + this._simple.disconnectSignal(this._locationUpdatedId); Ah ok. I'll send an updated patch.
Created attachment 313400 [details] [review] CurrentLocationController: Use new geoclue library Make use of new Geoclue convenience library to get rid of most of the code related to fetching current location.
So shall I merge the v2 of the patch?
(In reply to Zeeshan Ali (Khattak) from comment #3) > Review of attachment 313380 [details] [review] [review]: > > ::: src/app/currentLocationController.js > @@ +21,3 @@ > const Lang = imports.lang; > const GWeather = imports.gi.GWeather; > +const Geoclue = imports.gi.Geoclue; > > I think it's very wrong to require runtime deps at buildtime. configure is > supposed to be only to check for buildtime deps, not runtime. configure is there to check everything that is needed to run the app. If configure misses some dep, it's possible to ship a broken build because some optional runtime dep was not caught in testing (especially in the bundled case). But anyway, thanks for the v2 of the patch, and sorry I missed it until today because I wasn't CCed for some reason. Merged it, it will be in 3.19.1 out today.
(In reply to Giovanni Campagna from comment #6) > (In reply to Zeeshan Ali (Khattak) from comment #3) > > Review of attachment 313380 [details] [review] [review] [review]: > > > > ::: src/app/currentLocationController.js > > @@ +21,3 @@ > > const Lang = imports.lang; > > const GWeather = imports.gi.GWeather; > > +const Geoclue = imports.gi.Geoclue; > > > > I think it's very wrong to require runtime deps at buildtime. configure is > > supposed to be only to check for buildtime deps, not runtime. > > configure is there to check everything that is needed to run the app. Not really. Build and runtime are not always the same. By requiring a runtime-only dep at configure time, you only make the lives of distro packagers and cross-compiling folks harder. > If configure misses some dep, it's possible to ship a broken build because > some optional runtime dep was not caught in testing (especially in the > bundled case). That is certainly true but simply requiring runtime-only dep at build time, isn't the right solution here IMO. Although a warning from configure would be helpful. > But anyway, thanks for the v2 of the patch, and sorry I missed it until > today because I wasn't CCed for some reason. > Merged it, it will be in 3.19.1 out today. Np. I must admit that I somehow got the 3.19 schedule wrong so I haven't yet released geoclue lib. I'll do that now..
(In reply to Zeeshan Ali (Khattak) from comment #7) > (In reply to Giovanni Campagna from comment #6) > > (In reply to Zeeshan Ali (Khattak) from comment #3) > > > Review of attachment 313380 [details] [review] [review] [review] [review]: > > Np. I must admit that I somehow got the 3.19 schedule wrong so I haven't yet > released geoclue lib. I'll do that now.. Done.