GNOME Bugzilla – Bug 704537
[PATCH] Fix map not panning to new user location
Last modified: 2013-09-21 20:26:09 UTC
This patchset includes cleanups and refactorings leading up to fixing a bug where the map wouldn't pan to new locations as retrieved by GeoClue. I must admit I might have gotten a bit overeager here but I decided to fix issues I saw as I found them while I was looking for the root cause of the bug. I hope the changes makes sense.
Created attachment 249598 [details] [review] Add a .jshintrc config file This commit adds a config file for the javascript linter jshint. It doesn't add any dependencies on jshint, but helps me checking my code for style errors before commits. The config file is set up according to what I believe our coding conventions are.
Created attachment 249599 [details] [review] Cleanups Reindentions, trailing commas, extra spaces, try to fit 80 char columns and more js-idiomatic code.
Created attachment 249600 [details] [review] Util: remove code referencing pkg Since we don't use the pkg stuff Giovanni has written, we shouldn't have code that references it. This commit removes one such reference in utils.js
Created attachment 249601 [details] [review] MapLocation: move zoomToFit-functionality Move and rename zoom to fit-functionality to MapLocation since it's probably not very general anyways.
Created attachment 249602 [details] [review] UserLocation: move accuracy desc here Move accuracy-description code here since it's not very general anyways.
Created attachment 249603 [details] [review] Add documentation Document a few functions here and there.
Created attachment 249604 [details] [review] MainWindow: make _init smaller Split out initialization of actions and restoration of window geometry from _init making it a bit more manageable.
Created attachment 249605 [details] [review] Utils: add new method 'once' Utils.once connects to a signal once and then immediately disconnects. Since I don't know if it is possible to monkey patch GObject prototypes this it is only added to the prototypes of js objects. GObjects need to call Utils.once(gobject, signalName, callback);
Created attachment 249606 [details] [review] MainWindow: more robust track toggle Make the track user location toggle just represent a GSettingsAction. This makes the code a bit cleaner and also the app a bit more robus in case of a user fiddling with gsettings while Maps is running.
Created attachment 249607 [details] [review] MainWindow: split out signal setup Split out signal connection setup into its own method, finally making the _init of MainWindow reasonably short.
Created attachment 249608 [details] [review] Fix map not panning to new user location Fix a bug where when a new user location is found the map wouldn't pan to it even if the track-user-location setting is enabled. This patch doesn't make the map not animate if the new location was found on start of the program.
Review of attachment 249608 [details] [review]: "This patch doesn't make the map not animate if the new location was found on start of the program" That would be a regression then so needs some justification.
(In reply to comment #12) > Review of attachment 249608 [details] [review]: > > "This patch doesn't make the map not animate if the new location was > found on start of the program" > > That would be a regression then so needs some justification. I don't think it is actually. The old code (as far as I understand it) never tried to go to new user locations at all. It would go to the last stored location on startup, and it would do _that_ without animating. It would update the user location marker when location changed though. What we could do is to connect to geoclue and look for a new location (not the stored old one) on start up and if it is retrieved before some defined timeout go to that location (without animate) and only after that show the map. On the other hand Google Maps on my phone is actually panning on startup if I'm at a new location. Not sure here.
(In reply to comment #13) > (In reply to comment #12) > > Review of attachment 249608 [details] [review] [details]: > > > > "This patch doesn't make the map not animate if the new location was > > found on start of the program" > > > > That would be a regression then so needs some justification. > > I don't think it is actually. The old code (as far as I understand it) never > tried to go to new user locations at all. It would go to the last stored > location on startup, and it would do _that_ without animating. It would update > the user location marker when location changed though. > > What we could do is to connect to geoclue and look for a new location (not the > stored old one) on start up and if it is retrieved before some defined timeout > go to that location (without animate) and only after that show the map. I don't think we want that. We should first quickly show the last known location (without animating) and then check currently location and take the map there with animation.
(In reply to comment #14) > I don't think we want that. We should first quickly show the last known > location (without animating) and then check currently location and take the map > there with animation. Yeah I agree. This is what the patch does (or should do).
Created attachment 249758 [details] [review] Fix map not panning to new user location Fix a bug where when a new user location is found the map wouldn't pan to it even if the track-user-location setting is enabled.
Review of attachment 249598 [details] [review]: Haven't tried myself but seems good. Next step: `make check` makes us of jshint it available.
Review of attachment 249599 [details] [review]: Looks good otherwise. ::: src/mainWindow.js @@ +166,2 @@ _onConfigureEvent: function(widget, event) { + if (this._configureId !== 0) { As pointed out in the other but, you need to provide some proof that we really do need to use these weird looking operators. ::: src/mapLocation.js @@ +69,3 @@ + this._view.disconnect(id); + + id = this._view.connect("animation-completed::go-to", (function() { I would want to rather follow the conventions set by gnome-documents and shell. Talking of: (function() { ... }).bind(this)); rather than: Lang.bind(this, function() {.. ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { that seems weird? If other gnome js apps are not using this confusing JS-specific syntax, I'd rather we don't either. @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, I think our limit is 120 chars (should be the same as other gnome apps) so only break if this line doesn't fit in that. @@ +108,3 @@ log ("Failed to search '" + string + "': " + e.message); } + }).bind(this)); same comment about Lang.bind @@ +122,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + }); Really happy with this part. I assume you tested with diff arbitrary values? @@ +139,3 @@ + .get_bounding_box() + .covers(this._userLocation.latitude, + this._userLocation.longitude); Same comment about JS-specific weird syntax here too. Seems it doesn't give us much anyways. @@ +152,3 @@ + this._userLocation = + new UserLocation.UserLocation(this._geoclue.location, + this); Same question about if breaking the line is really needed here? ::: src/userLocation.js @@ +50,3 @@ })); let pin_actor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg"); + if (!pin_actor) eeh, no please. Explicit checks please. @@ +69,3 @@ + let descriptionActor = new Clutter.Actor({ + layout_manager: new Clutter.BinLayout() + }); Don't really see this as any cleanup. @@ +77,3 @@ + vertical: true + }) + }); This change seems to be making code less readable to me.
Review of attachment 249600 [details] [review]: ACK
Review of attachment 249601 [details] [review]: ACK
Review of attachment 249602 [details] [review]: ::: src/userLocation.js @@ +140,3 @@ }, + + getAccuracyDescription: function() { Well if zoomToFit belongs in parent class, so does this.
Review of attachment 249603 [details] [review]: ::: src/mapLocation.js @@ +35,3 @@ +/** + * A map location object with an added accuracy. + */ I've been using the '//' for comments and seems gnome-documents does the same so lets consistently use '//'.
Review of attachment 249604 [details] [review]: ACK
Review of attachment 249599 [details] [review]: My comments below. ::: src/mainWindow.js @@ +166,2 @@ _onConfigureEvent: function(widget, event) { + if (this._configureId !== 0) { So the difference between "==" and "===" is that "==" has non-trivial semantics and I don't believe using == is worth the trouble. A short explanation is this: "==" does type coercion via a series of steps that aren't (at least to me) very easy to reason about. This becomes extra hard when reading code that another person has written. "===" on the other hand starts by checking if the types matches and then does very sane value equality checks after that. The js-projects I've worked with previously as well as the default JSHint config disallows "==" and "!=" for the above mentioned reasons. The only time I depart from this and actually use type coercion is when checking if objects or functions are defined. Like this: > if(func) func(); and > if(!obj) obj = ... I do this since it's the kind of check that comes up regularly, the truthy-coercions are simpler than the "==" coercions and the good / safe alternative is doing clunky specific type checks, like this: > if(typeof(func) === "function") or > if(typeof(obj) === "object" && obj !== null) There's a lot of material online about type coercions, the use of === vs == and truthy values. Not everyone agrees on this ofcourse but I prefer the safe "===" side. :) ::: src/mapLocation.js @@ +69,3 @@ + this._view.disconnect(id); + + id = this._view.connect("animation-completed::go-to", (function() { Hm. We could of course do that. I went with the standard bind here since it was easier to get the code to fit 80 chars that way (I have another comment about that later on!) but also since Function.bind is a standard part of javascript and Lang.bind is not. Lang.bind seems to be a relic from '08 when Litl wrote lang.js. Since then bind was added to the language and I see little reason to our custom implementation instead. ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { I'm guessing by the comment below that it is the method chaining you find confusing. This is common practice in JavaScript but also in Java and C#. The reason I did it here was that the forward var gets shadowed inside the callback further down and I wanted to avoid that and since the variable was only used one time I thought it to be unnecessary noise to declare it. This is subjective of course. @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, Oh. :( I'm a fan of 80 chars for two reasons: 1) I can fit 3 emacs buffers side by side on my screen :) 2) it's easier for me to scan through code that isn't wider If this is a gnome policy we'll follow that though. @@ +108,3 @@ log ("Failed to search '" + string + "': " + e.message); } + }).bind(this)); Yep, see the previous comment. @@ +122,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + I didn't since I've written this code many times at my old work. But I really should. Thanks! @@ +139,3 @@ + .get_bounding_box() + .covers(this._userLocation.latitude, + this._userLocation.longitude); It lets us avoid one variable declaration. For me this code is much clearer. But yeah, that's subjective I guess. @@ +152,3 @@ + this._userLocation = + new UserLocation.UserLocation(this._geoclue.location, + this); If we go by 120 chars instead of 80, I guess not. I don't particularly like all these UserLocation.UserLocation and MapView.MapView etc though, but that's another question. ::: src/userLocation.js @@ +50,3 @@ })); let pin_actor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg"); + if (!pin_actor) This is the best / simplest way I've found for checking if objects are defined or not. Checking against null with a "==" works fine in this case but I find the coercion rules really confusing in general and would prefer if we avoid them as much as possible. Checking for truthyness (admittedly via coercion) is pretty simple though which is why I'm ok with that. Look here: http://james.padolsey.com/javascript/truthy-falsey/ @@ +69,3 @@ + let descriptionActor = new Clutter.Actor({ + layout_manager: new Clutter.BinLayout() + }); I'm not a fan of defining vars that are only used once. Only having the actually needed vars in scope helps me read the code better. This again is obviously subjective, and I can revert it if you want to. @@ +77,3 @@ + vertical: true + }) + }); See above. Other than that we should probably not re-init layout so I guess something like: > let locationActorLayour = new Clutter.BoxLayout({ vertical: true }); > let locationActor = new Clutter.Actor({ layout_manager: locationActorLayour }); ?
Review of attachment 249602 [details] [review]: ::: src/userLocation.js @@ +140,3 @@ }, + + getAccuracyDescription: function() { Agree. Stupid me. Will fix. Thanks! :)
Review of attachment 249603 [details] [review]: ::: src/mapLocation.js @@ +35,3 @@ +/** + * A map location object with an added accuracy. + */ The above style is used by a lot of projects for documentation-style comments. I was thinking we could make code comments use // and docs-comments use /** */. Since we don't have any docs tools though this is probably just unnecessary, will change! Thanks!
Review of attachment 249605 [details] [review]: In the log: then immediately disconnects -> then disconnects on first emission. Looks good otherwise. ::: src/mapView.js @@ +128,3 @@ gotoUserLocation: function(animate) { + this._userLocation.once("gone-to", + this.emit.bind(this, 'gone-to-user-location')); This would be less readable to JS-novices like myself. Can we not do this. Besides its not relevant to this patch. ::: src/utils.js @@ +53,3 @@ +/** + * Connect to a signal on an object and immediately + * disconnect on first call. * call -> call to 'callback' (and 'immediately' is redundant here). * No need to break on multiple lines. * C++ style comments '//' please. @@ +62,3 @@ +} + +function addSignalMethods(proto) { Why are you renaming this function? @@ +63,3 @@ + +function addSignalMethods(proto) { + Signals.addSignalMethods(proto); So i'm guessing this call takes care of everything the four calls you removed were doing? @@ +64,3 @@ +function addSignalMethods(proto) { + Signals.addSignalMethods(proto); + proto.once = once.bind(undefined, proto); What's happening here? @@ -87,3 @@ } - irrelevant change.
Review of attachment 249606 [details] [review]: * In the log, robus -> robust. * User is not supposed to fiddle with such things so thats not really an argument. :) Having said that, we should theoretically be acting on gsettings changes. Looks good otherwise. ::: src/main-window.ui @@ +41,3 @@ <property name="can_focus">True</property> <property name="symbolic-icon-name">find-location-symbolic</property> + <property name="action_name">win.track-user-location</property> Doesn't seem aligned with previous property nodes ::: src/mainWindow.js @@ +84,3 @@ + this.mapView.gotoUserLocation(true); + } + }).bind(this)); As in previous patches, we want to use Lang.bind(). @@ +169,3 @@ + _connectMapMove: function() { + if(!this._viewMovedId) { I would prefer to keep the explicit (integer) checks here. @@ +177,3 @@ + }, + _disconnectMapMove: function() { + if(this._viewMovedId) { I would prefer explicit integer checks here.
Review of attachment 249607 [details] [review]: Looks good. ::: src/mainWindow.js @@ +118,3 @@ + }, + + _connectMapMove: function() { Why are you moving these functions above?
Review of attachment 249758 [details] [review]: Commit shortlog should ideally say what the patch is doing rather than its effect. And *if* its not obvious, the details part should specify how the change fixes the issue. ::: src/mainWindow.js @@ +133,3 @@ }, + _onUserLocationChange: function() { big time nitpick: _onUserLocationChange -> _onUserLocationChanged
Review of attachment 249605 [details] [review]: ::: src/mapView.js @@ +128,3 @@ gotoUserLocation: function(animate) { + this._userLocation.once("gone-to", + this.emit.bind(this, 'gone-to-user-location')); An explanation: Lang.bind and function.bind just binds function parameters to functions, including the implicit 'this' parameter. With Lang.bind I think this would be the same but in reverse: > Lang.bind('gone-to-user-location', this, this.emit) The reason I did this was that the callback function shrunk to only one statement: > function() { this.emit('gone-to-user-location'); } which is just clunkier syntax for: > this.emit.bind(this, 'gone-to-user-location') (like above). Anyway, I'll go with an explicit function call (and Lang.bind or function.bind depending on what we agree on in the other bug). ::: src/utils.js @@ +53,3 @@ +/** + * Connect to a signal on an object and immediately + * disconnect on first call. Sure! @@ +63,3 @@ + +function addSignalMethods(proto) { + let id = obj.connect(signal, function() { Yeah. The Utils.addJSSignalMethods is a version of Signals.addSignalMethods that appends "JS" to the end of all methods. So you would use this.emitJS('some-signal') instead of this.emit('some-signal'). I'm guessing it's imported from some other module when we started the project. We never used it though and since I added a new addSignalMethods I thought I should remove that one. I should have done that in a separate commit though. @@ +64,3 @@ +function addSignalMethods(proto) { + Signals.addSignalMethods(proto); + obj.disconnect(id); It's the same as: > proto.once = function(signal, callback) { > once(proto, signal, callback); > } I just add the once-function as a method on the object we're extending. @@ -87,3 @@ } - True, thanks!
Review of attachment 249599 [details] [review]: ::: src/mainWindow.js @@ +166,2 @@ _onConfigureEvent: function(widget, event) { + if (this._configureId !== 0) { Thanks for the explanation. ::: src/mapLocation.js @@ +69,3 @@ + this._view.disconnect(id); + + id = this._view.connect("animation-completed::go-to", (function() { I don't think our code will ever be running in browsers so for us its more important to follow GNOME conventions (it will make it easier for other GNOME apps devs to contribute to Maps). ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { * On second look, I realize its not JS-specific at all but I just never saw anyone styling it like this before (or if i had, i've long forgotten). Usually one just does: x.y().z(); but if that doesn't fit one line or looks ugly, I've always seen people introducing a variable, like the existing code does. * Its the same object that gets shadowed so not really any problem. @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, Not a GNOME policy but we been following that first in gnome-boxes and then I kinda followed the same in Maps. If you wanna change that, you'll have to provide a patch that changes it everywhere. :) @@ +152,3 @@ + this._userLocation = + new UserLocation.UserLocation(this._geoclue.location, + this); Me neither but since other projects do the same, I can live with it fine. ::: src/userLocation.js @@ +50,3 @@ })); let pin_actor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg"); + if (!pin_actor) I'm really starting to hate this language. :) @@ +69,3 @@ + let descriptionActor = new Clutter.Actor({ + layout_manager: new Clutter.BinLayout() + }); ok, no big diff for me.. @@ +77,3 @@ + vertical: true + }) + }); Why we shouldn't reinit layout? Can't we just re-assign it (i-e remove the 'let')?
Review of attachment 249606 [details] [review]: ::: src/main-window.ui @@ +41,3 @@ <property name="can_focus">True</property> <property name="symbolic-icon-name">find-location-symbolic</property> + <property name="action_name">win.track-user-location</property> True, will fix! ::: src/mainWindow.js @@ +84,3 @@ + this.mapView.gotoUserLocation(true); + } + this.mapView.connect('gone-to-user-location', I'm pretty sure the only reason we got Lang.bind in gjs at all is because Function.bind wasn't implemented in '08 when gjs was started. Now that it is, the only reason I can see to use it is because your code base is full of Lang.bind's already and doing a transition is pointless. Lang.bind is nowadays also just a wrapper for Function.bind in most cases. In the case of Maps we are still very young and moving to Function.bind isn't hard at all. The rewards are: 1) we use the standard bind method so our code is a bit more familiar to newcomers (outside of gnome). 2) it's shorter to type 3) we go directly to Function.bind instead of through a wrapper that adds nothing 4) when binding named functions the function name comes first which (subjective again) I think increases readability a little. @@ +169,3 @@ + _connectMapMove: function() { + if(!this._viewMovedId) { Sure, that makes sense. Thanks. @@ +177,3 @@ + }, + _disconnectMapMove: function() { + Application.settings.set_boolean('track-user-location', false); Yup, agree!
Review of attachment 249607 [details] [review]: ::: src/mainWindow.js @@ +118,3 @@ + }, + + this.mapView.connect('going-to-user-location', Because they are directly related to the _initSignals-method. I could probably define them inside of _initSignals even.
Review of attachment 249758 [details] [review]: ::: src/mainWindow.js @@ +133,3 @@ }, + _onUserLocationChange: function() { Sure! I'm all for nitpicks. :)
Review of attachment 249599 [details] [review]: ::: src/mainWindow.js @@ +166,2 @@ _onConfigureEvent: function(widget, event) { + if (this._configureId !== 0) { Np. Js is full of ugly quirks. ::: src/mapLocation.js @@ +69,3 @@ + this._view.disconnect(id); + + id = this._view.connect("animation-completed::go-to", (function() { Nah of course it won't run in browsers (Lang.bind would work fine in a recent enough browser though). But people coming from a Node.js or a browser background probably know about Function.bind already. This isn't a strong point though I must admit, since guessing what Lang.bind does isn't very hard. It's just that its use is... well unnecessary and it bugs me. :) ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { Ah. Now I see. Yeah I started writing code like that when I learned C# and JavaScript because of their funtional-style libraries. It kind of stuck. What do you think about the extra variable declaration though? I'm not a fan of those. @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, Ok sure. I'll revert this in this patch. Would you take an 80-chars patch though? It would make me (and my OCD) very happy. :) @@ +152,3 @@ + this._userLocation = + new UserLocation.UserLocation(this._geoclue.location, + this); Yup I guess it's a question that's out of scope for Maps. ::: src/userLocation.js @@ +50,3 @@ })); let pin_actor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg"); + if (!pin_actor) Haha, that's a reasonable position to have. :) I've gone back and forth and is at the "I respect this language for all its faults" point currently. @@ +77,3 @@ + vertical: true + }) + }); Yeah we could do that instead (and would need to at some point since this likely will throw an exception in ES6). Still layout isn't used anywhere else. So why declare it at all?
Review of attachment 249599 [details] [review]: ::: src/mapLocation.js @@ +69,3 @@ + this._view.disconnect(id); + + id = this._view.connect("animation-completed::go-to", (function() { Its not unnecessary. Its doing the same as JS-standard bind. Its more likely that other GNOME app devs contribute to Maps than people coming from Node.js and browser background IMO and even if they do, they are likely to also/first contribute to shell or documents etc so they'll need to learn about this Lang.bind anyways. However, maybe its time GNOME app devs learn about bind function so lets keep this. ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { I don't mind amount of variables much now that i know that modern compilers/interpreters know how to optimize stack usage for you. In dynamic languages, it usually makes things more readable actually if you give nice names to variables (its not the case in here though). @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, Now that I have changed my settings for 120 chars, its about "my settings vs. yours" but I wouldn't mind going to 80-chars. Having said that, you don't have a lot of time (midterms are already here) for your SoC left and especially if you want to complete the optional parts of your project so my suggestion would be not to distract yourself with such minor issues for now. @@ +122,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + }); Cool, don't forget to comment here once you tested. ::: src/userLocation.js @@ +77,3 @@ + vertical: true + }) + }); We might have other issues too with ES6 so don't worry until we actually start using it. :) As for you question, to avoid the nested construction thats less readable than using a variable.
Created attachment 249944 [details] [review] Cleanups Reindentions, trailing commas, extra spaces, try to fit 80 char columns and more js-idiomatic code.
Created attachment 249945 [details] [review] Util: remove code referencing pkg Since we don't use the pkg stuff Giovanni has written, we shouldn't have code that references it. This commit removes one such reference in utils.js
Created attachment 249946 [details] [review] Utils: remove addJSSignalMethods This code was added in the beginning phases of the project but is never actively used. The point seem to be to make a difference between signals that come from gobjects and those that come from pure js-objects. We can easily resurrect this later if we find a reason for this.
Created attachment 249947 [details] [review] MapLocation: move zoomToFit-functionality Move and rename zoom to fit-functionality to MapLocation since it's probably not very general anyways.
Created attachment 249948 [details] [review] MapLocation: move accuracy desc here Move accuracy-description code here since it's not very general anyways.
Created attachment 249949 [details] [review] Add documentation Document a few functions here and there.
Created attachment 249950 [details] [review] MainWindow: make _init smaller Split out initialization of actions and restoration of window geometry from _init making it a bit more manageable.
Created attachment 249951 [details] [review] Utils: add new method 'once' Utils.once connects to a signal once and then disconnects on first emission. Since I don't know if it is possible to monkey patch GObject prototypes this it is only added to the prototypes of js objects. GObjects need to call Utils.once(gobject, signalName, callback);
Created attachment 249952 [details] [review] MainWindow: split out signal setup Split out signal connection setup into its own method, finally making the _init of MainWindow reasonably short.
Created attachment 249953 [details] [review] MapView: pan to new user locations Fix a bug where when a new user location is found the map wouldn't pan to it even if the track-user-location setting is enabled.
Review of attachment 249944 [details] [review]: ::: src/mainWindow.js @@ +99,3 @@ this.window.maximize(); + this.window.connect('delete-event', Lang.bind(this, this._quit)); Why not use JS bind in here as well? @@ +106,3 @@ + this._searchEntry.connect('activate', + Lang.bind(this, this._onSearchActivate)); here too ::: src/mapView.js @@ +88,3 @@ geocodeSearch: function(string) { + Geocode.Forward + .new_for_string(string) The conclusion was that we don't write code this way. Temporary variables aren't bad. @@ +120,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + }); So I guess you tested this then? @@ +135,3 @@ userLocationVisible: function() { + return this.view + .get_bounding_box() same here, keep the previous code.
Review of attachment 249945 [details] [review]: ACK
Review of attachment 249946 [details] [review]: ACK
Review of attachment 249947 [details] [review]: Assuming this has remained the same, ACK
Review of attachment 249948 [details] [review]: If it hasn't changed, ACK.
Review of attachment 249949 [details] [review]: The description is not correct, you are documenting a class and a method.
Review of attachment 249949 [details] [review]: Actually, ACK with commit log adjusted (no need to resubmit the patch).
Review of attachment 249950 [details] [review]: If it hasn't changed, ACK.
Review of attachment 249951 [details] [review]: ::: src/mainWindow.js @@ +85,3 @@ + this.mapView.gotoUserLocation(true); + } + }).bind(this)); These changes doesn't seem to fit in this patch.
Review of attachment 249952 [details] [review]: ::: src/mainWindow.js @@ +187,3 @@ }, _connectMapMove: function() { Rebase went wrong? _(dis)connectMapMove are not defined twice.
Review of attachment 249953 [details] [review]: ACK
Review of attachment 249599 [details] [review]: Forgot to press Publish on this review. :( ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { Ok, so the verdict in this specific case is that I can keep the chaining? @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, Yup I won't. I'll take this up on a later date. @@ +122,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + Seems to work good. Made a small test case here: https://gist.github.com/moonlite/6064786#file-test-ensurevisible ::: src/userLocation.js @@ +77,3 @@ + vertical: true + }) + }); Ok sure!
Review of attachment 249944 [details] [review]: ::: src/mainWindow.js @@ +99,3 @@ this.window.maximize(); + this.window.connect('delete-event', Lang.bind(this, this._quit)); I'll go over to JS-bind in all these patches then. Cool! ::: src/mapView.js @@ +88,3 @@ geocodeSearch: function(string) { + Geocode.Forward + .new_for_string(string) Sure, np. Just a misunderstanding. @@ +120,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + Yeah, sorry. I forgot to post the review. @@ +135,3 @@ userLocationVisible: function() { + return this.view + .get_bounding_box() Ok sure!
Review of attachment 249944 [details] [review]: ::: src/mapView.js @@ +120,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + My old comment: > Seems to work good. Made a small test case here: > > https://gist.github.com/moonlite/6064786#file-test-ensurevisible
Review of attachment 249951 [details] [review]: Bad rebase, should have looked closer. ::: src/mainWindow.js @@ +85,3 @@ + this.mapView.gotoUserLocation(true); + } + this._disconnectMapMove.bind(this)); An accidental merge of two commits I think. Will rework it.
Review of attachment 249952 [details] [review]: ::: src/mainWindow.js @@ +187,3 @@ }, _connectMapMove: function() { Yep something went wrong along the way. Will fix.
Review of attachment 249599 [details] [review]: ::: src/mapView.js @@ +89,3 @@ + Geocode.Forward + .new_for_string(string) + .search_async (null, (function(forward, res) { No, "In dynamic.." part doesn't apply here. A temporary variable is fine. @@ +100,3 @@ + + let mapLocation + = new MapLocation.MapLocation(location, huh? You already provided a patch for 80 char. :) @@ +122,3 @@ + bbox.bottom = Math.min(bbox.bottom, location.latitude); + bbox.top = Math.max(bbox.top, location.latitude); + }); Good.
Review of attachment 249944 [details] [review]: ::: src/mainWindow.js @@ +106,3 @@ + this._searchEntry.connect('activate', + Lang.bind(this, this._onSearchActivate)); I'm not sure if you want just these two changed to function.bind, or these four in init or all in the entire code base. I'm thinking that moving the entire code base should go into a separate patch (when I have time for that) and that changing two out of four in a group seems strange so I'm going with these four. Hope that's what you mean.
Created attachment 249989 [details] [review] Cleanups Reindentions, trailing commas, extra spaces, try to fit 80 char columns and more js-idiomatic code.
Created attachment 249990 [details] [review] Utils: add new method 'once' Utils.once connects to a signal once and then disconnects on first emission. Since I don't know if it is possible to monkey patch GObject prototypes this it is only added to the prototypes of js objects. GObjects need to call Utils.once(gobject, signalName, callback);
Created attachment 249991 [details] [review] Utils: add new method 'once' Utils.once connects to a signal once and then disconnects on first emission. Since I don't know if it is possible to monkey patch GObject prototypes this it is only added to the prototypes of js objects. GObjects need to call Utils.once(gobject, signalName, callback);
Created attachment 249992 [details] [review] MainWindow: split out signal setup Split out signal connection setup into its own method, finally making the _init of MainWindow reasonably short.
Created attachment 249993 [details] [review] MainWindow: more robust track toggle Make the track user location toggle just represent a GSettingsAction. This makes the code a bit cleaner and the app a bit more robust.
Created attachment 249997 [details] [review] MapView: pan to new user locations Fix a bug where when a new user location is found the map wouldn't pan to it even if the track-user-location setting is enabled.
(In reply to comment #75) > Created an attachment (id=249997) [details] [review] > MapView: pan to new user locations > > Fix a bug where when a new user location is found the map wouldn't pan > to it even if the track-user-location setting is enabled. This one was already marked submit, but I reverted one of those very wrapped 80-chars-fixes here.
Review of attachment 249944 [details] [review]: ::: src/mainWindow.js @@ +106,3 @@ + this._searchEntry.connect('activate', + Lang.bind(this, this._onSearchActivate)); If you are going to have a separate patch to port the entire code, I don't see the point of changing any of Lang.bind then. I really hate inconsistent conding-style and we shouldn't at least make it inconsistent intentionally.
Review of attachment 249989 [details] [review]: ::: src/mapLocation.js @@ +76,3 @@ + + this._view.go_to(this.latitude, this.longitude); + }).bind(this)); As I said in previous comment, we either want to convert the whole code to JS' bind in this patch or not convert any code. Same goes for fitting in 80-chars.
Review of attachment 249991 [details] [review]: ::: src/mapView.js @@ +127,3 @@ + this._userLocation.once("gone-to", (function() { + this.emit('gone-to-user-location'); + }).bind(this)); Use Lang.bind unless you modify the cleanup patch to convert the whole code.
Review of attachment 249992 [details] [review]: ACK
Review of attachment 249993 [details] [review]: Looks good otherwise ::: src/mainWindow.js @@ -81,3 @@ - // Disable animation for goto animation on startup only - let animateGotoUserLocation = !trackUserLocation; - toggle.connect('toggled', Lang.bind(this, Where went the code to handle the button toggled by user? @@ +113,3 @@ ], this); + this.window.add_action( + Application.settings.create_action('track-user-location')); Use a variable please. Also when arguments dont fit, you want to break the line before '('.
Review of attachment 249997 [details] [review]: Looks good otherwise. ::: src/mainWindow.js @@ +114,3 @@ + this._onUserLocationChanged.bind(this)); + Application.settings.connect('changed::track-user-location', + this._onUserLocationChanged.bind(this)); You might need to change these lines depending on what you decide to do about 'bind' in cleanups patch.
Created attachment 250429 [details] [review] Cleanups Reindentions, trailing commas, extra spaces and more js-idiomatic code.
Review of attachment 249989 [details] [review]: ::: src/mapLocation.js @@ +76,3 @@ + + this._view.go_to(this.latitude, this.longitude); + this._view.disconnect(id); I just went with standard bind. Wasn't as big a change as I thought..
Review of attachment 249991 [details] [review]: ::: src/mapView.js @@ +127,3 @@ + this._userLocation.once("gone-to", (function() { + this.emit('gone-to-user-location'); + }).bind(this)); Yup, going for the whole code.
Review of attachment 249993 [details] [review]: ::: src/mainWindow.js @@ -81,3 @@ - // Disable animation for goto animation on startup only - let animateGotoUserLocation = !trackUserLocation; - if (!this.mapView.userLocationVisible()) I added a GAction for this, see here: https://developer.gnome.org/gio/2.35/GSettings.html#g-settings-create-action The net result is that we only need to react to the setting changing and it will still cover both cases. @@ +113,3 @@ ], this); + this.window.add_action( + Application.settings.create_action('track-user-location')); Ok sure!
Created attachment 250430 [details] [review] Utils: add new method 'once' Utils.once connects to a signal once and then disconnects on first emission. Since I don't know if it is possible to monkey patch GObject prototypes this it is only added to the prototypes of js objects. GObjects need to call Utils.once(gobject, signalName, callback);
Created attachment 250431 [details] [review] MainWindow: more robust track toggle Make the track user location toggle just represent a GSettingsAction. This makes the code a bit cleaner and the app a bit more robust.
Review of attachment 250429 [details] [review]: Looks good
Review of attachment 250430 [details] [review]: looks good otherwise. ACK once you answer the question. ::: src/utils.js @@ +61,3 @@ +function addSignalMethods(proto) { + Signals.addSignalMethods(proto); + proto.once = once.bind(undefined, proto); I still don't know why you need to add 'once' to prototype?
Review of attachment 250431 [details] [review]: ACK
Created attachment 250486 [details] [review] Utils: add new method 'once' Utils.once connects to a signal once and then disconnects on first emission. The method can also be added to the prototype of select objects with Utils.addSignalMethods, much like Signals.addSignalMethods Since I don't know how to monkey patch GObject prototypes this can only be added to the prototypes of pure JavaScript objects. GObjects need to call Utils.once(gobject, signalName, callback);
Updated thw wording (like you can see) on the once-patch.
Review of attachment 250486 [details] [review]: ACK