GNOME Bugzilla – Bug 736326
BoundingBox for userlocation
Last modified: 2018-03-26 12:41:54 UTC
We have a circle to visualize the accuracy of the current location, but no bounding box that can help with the zoom level when going there.
Created attachment 285734 [details] [review] userLocationMarker: Add bounding box
Created attachment 285735 [details] [review] [RFC] mapWalker: more aggresive zoomToFit Right now the zoomToFit function does not really work. It tries to find the correct zoom level where the bounding box is visible. But it cannot really and the result will vary depending on the lat/lon you start from. It does not now where the bounding box center will be at the zoom level it tries to get bounding box for. Sort of. So this patch simplifies, maybe to much, Try it out and see if you like it.
Created attachment 285759 [details] Screenshot with patches This is what I see at launch with these patches in.
Review of attachment 285734 [details] [review]: Good! ::: src/userLocationMarker.js @@ +79,3 @@ + right: this._view.x_to_longitude(x + radius) + }); + }, These two properties are non-trivial and does a bunch of calculations. They should probably be functions instead?
Review of attachment 285735 [details] [review]: Looks good to me.
Created attachment 285843 [details] [review] userLocationMarker: Add bounding box
Review of attachment 285843 [details] [review]: Otherwise, looks good to me. ::: src/userLocationMarker.js @@ +58,3 @@ }, + _getRadius: function() { get radius? @@ +68,3 @@ + }, + + _getBoundingBox: function() { get boundingBox? @@ +112,3 @@ }).bind(this)); + + this.place.bounding_box = this._accuracyMarker.getBoundingBox(); It looks like the code is broken here, isn't the method called _getBoundingBox (with underscore), I'd go for the getter syntax anyways.
Review of attachment 285735 [details] [review]: Looks good to me too, but now I don't understand why we didn't used ensure_visible before xD
Review of attachment 285735 [details] [review]: It seems that _boxCover method is not used anymore after this change.
Review of attachment 285735 [details] [review]: Well, ensureVisible only ensures visibility so if we are zoom out it does nothing. This sets the zoom-level to max every time and then goes ensureVisible. It is a bit extreme. And yeah _boxCover can go away and I think all the things about place_type as well. Not much does not have a boundingbox and we could set a default zoom-level for them.
Review of attachment 285843 [details] [review]: ::: src/userLocationMarker.js @@ +68,3 @@ + }, + + _getBoundingBox: function() { Well I had them as getters before. But Mattias in his review pointed out that they do quite some calculation and are non-trivial. There are more functions than properties and I tend to agree. @@ +112,3 @@ }).bind(this)); + + this.place.bounding_box = this._accuracyMarker.getBoundingBox(); Yeah typo.
Created attachment 285944 [details] [review] userLocationMarker: Add bounding box
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-maps/issues/14.