GNOME Bugzilla – Bug 756580
geoclue: Make use of new convenience library
Last modified: 2015-10-23 13:15:51 UTC
I want Maps to be the first one to take advantage of the new geoclue convenience library. I wouldn't merge this patch yet though and wait for the new API to be released first. It goes without saying that this is meant for 3.20.
Created attachment 313314 [details] [review] geoclue: Make use of new convenience library
API docs: http://www.freedesktop.org/software/geoclue/docs/libgeoclue/GClueSimple.html
Created attachment 313318 [details] [review] geoclue: Make use of new convenience library v2: The import line in previous version was all wrong. I also realized that I didn't actually get to test the patch since the location was from a previous run and I get a crash not long after the launch (in jhbuild) with this on console: (org.gnome.Maps:17168): folks-WARNING **: Error preparing persona store 'bluez:F8:84:F2:A5:DA:D7': An OBEX address book transfer from device ‘Zeeshan's galaxy’ could not be started: GDBus.Error:org.bluez.obex.Error.Failed: Unable to find service record (org.gnome.Maps:17168): GLib-ERROR **: Creating pipes for GWakeup: Too many open files
Love it! Thanks! Will try it out later to make sure all use-cases still work. So when will the next GeoClue release be do you think?
(In reply to Jonas Danielsson from comment #4) > Love it! Great. :) > Thanks! Will try it out later to make sure all use-cases still work. Cool, please do. I didn't really get to test it due to that issue. > So when will the next GeoClue release be do you think? Actually, I think the patch should be merged already if it works so that we can get new API tested before I can release. I can release before 3.19.1 tarballs due date.
Created attachment 313388 [details] [review] geoclue: Make use of new convenience library v2: This one I tested. :)
Review of attachment 313388 [details] [review]: Just some small nits. Great work Zeeshan! ::: src/geoclue.js @@ +22,2 @@ const GObject = imports.gi.GObject; +const GClue = imports.gi.Geoclue; This should be Geoclue (for consistency). @@ +114,3 @@ }, + _onSimpleReady: function(object, result) { I'd just make this an anonymous function passed to GClue.Simple.new @@ +132,3 @@ this.state = State.ON; + + this._onLocationUpdated(this._simple); We use present tense for most signal handlers I believe. So I think you should go with this._onLocationUpdate instead.
Review of attachment 313388 [details] [review]: ::: src/geoclue.js @@ +22,2 @@ const GObject = imports.gi.GObject; +const GClue = imports.gi.Geoclue; It was but it conflicts with the 'const Geoclue' below. @@ +114,3 @@ }, + _onSimpleReady: function(object, result) { Why? IMO code is less clean with anonymous functions. @@ +132,3 @@ this.state = State.ON; + + this._onLocationUpdated(this._simple); I did not come up with a new function or name in this patch, I just had to change the signature of it for the new API.
Review of attachment 313388 [details] [review]: ::: src/geoclue.js @@ +132,3 @@ this.state = State.ON; + + this._onLocationUpdated(this._simple); Yeah, but when we are changing stuff we might as well change it!
Review of attachment 313388 [details] [review]: ::: src/geoclue.js @@ +132,3 @@ this.state = State.ON; + + this._onLocationUpdated(this._simple); ok. :)
Created attachment 313401 [details] [review] geoclue: Make use of new convenience library v2: Better name for notify handler function and id field.
Review of attachment 313388 [details] [review]: ::: src/geoclue.js @@ +22,2 @@ const GObject = imports.gi.GObject; +const GClue = imports.gi.Geoclue; True and also a bit is awkward. We could rename this whole class to locationService or something I guess, but let's keep it as GClue for now. :) @@ +114,3 @@ }, + _onSimpleReady: function(object, result) { Because it isn't used anywhere else and because this is more about doing async workflow then event handling.
Review of attachment 313388 [details] [review]: ::: src/geoclue.js @@ +114,3 @@ }, + _onSimpleReady: function(object, result) { If it was simply completing the async call and had < 3 LOCs, I'd also prefer anonymous function. I don't think async vs event handling is relevant here. There is a discussion here about the subject: http://stackoverflow.com/questions/1960517/anonymous-functions-considered-harmful . The keypoint "deep indentation is considered bad programming practice, and generally points out at design issues if you indent more than three or four levels." is important IMO and I always tend to avoid indentation, whenever possible.
(In reply to Zeeshan Ali (Khattak) from comment #11) > Created attachment 313401 [details] [review] [review] > geoclue: Make use of new convenience library > > v2: Better name for notify handler function and id field. So is this good now?
Review of attachment 313401 [details] [review]: Thanks!
Attachment 313401 [details] pushed as a79b23b - geoclue: Make use of new convenience library