GNOME Bugzilla – Bug 735215
Add support for --uri
Last modified: 2015-11-02 10:49:43 UTC
Since Geocode has support for parsing geoURI and glib now have support for adding command line options, lets try to add this for maps!
Created attachment 284178 [details] [review] main.c: Use the correct argv and argc This should probably go in either way, since I consider it a bug-fix.
Created attachment 284179 [details] [review] mapWalker: Make sure we center on location after zoom Damian: It might make sense to merge this along with your patches. It fixes some things for me. When going to a place the marker might sometime be near an edge and that will result in the bubble not showing, and other funny things.
Created attachment 284180 [details] [review] Add uri command line option
Review of attachment 284180 [details] [review]: ::: src/URIMarker.js @@ +35,3 @@ + this.parent(params); + + let iconActor = Utils.CreateActorFromImageFile(Path.ICONS_DIR + "/pin.svg"); If someone has an idea for a better icon that would be lovely. ::: src/uri-bubble.ui @@ +15,3 @@ + <property name="can_focus">False</property> + <property name="margin_start">10</property> + <child> If someone knows of a nice image/icin we can have do describe this that would be lovely as well.
Review of attachment 284180 [details] [review]: ::: src/URIBubble.js @@ +43,3 @@ + ui.labelCoordinates.label = this.place.location.latitude.toFixed(5) + + ', ' + + this.place.location.longitude.toFixed(5); An alternative approach to this is to set the coordinates as name (and thus the bubble-title class) if there is no place-name.
Review of attachment 284178 [details] [review]: Patch itself is good and I don't think its just your opinion that its a bug fix. :) * A comma after "Without this" would help readability a LOT. :) A comma after 'With this fix" would also be nice but not necessary. * Were we lying to GApplication about what? * "will be able to go" -> "will be able to do"?
Review of attachment 284178 [details] [review]: Sure commas can go in there. We were lying about what our actual argv was :) Removing the program-name part of the string-array. How about: "Before this, commandline handling would not work. We were removing the program-name from the argv string array. With this fix, we will be able to use the standard GApplication options. $ gnome-maps --help-gapplication $ gnome-maps --gapplication-service etc."
Review of attachment 284179 [details] [review]: What mapWalker.js? I don't see any such module in git master. shortlog could be shorter so why not? Please always try you best to keep it under the 50 chars recommendation.
Review of attachment 284179 [details] [review]: Sure. mapWalker: Center on location after zoom mapWalker is a new module from the blocking bug, which is Damians marker bug. Since there is no mapLocation anymore mapWalker is no responsible for the goTo-code. See that bug for details.
Review of attachment 284180 [details] [review]: Cool, although I think maybe we should support this in the UI as well? Through search entry perhaps? ::: src/mapView.js @@ +154,3 @@ }, + showURI: function(uri) { With URI in URIBubble's name, I'd think it should be URIBubble that should handle parsing of URI.
Review of attachment 284179 [details] [review]: OK, please update patch with new commit log.
Created attachment 284195 [details] [review] main.c: Use the correct argv/argc Updated commit message.
Created attachment 284196 [details] [review] mapWalker: Center on location after zoom Updated shortlog.
Review of attachment 284180 [details] [review]: ::: src/mapView.js @@ +154,3 @@ }, + showURI: function(uri) { You mean URIMarker? Hmm, well it could be done. But it can't be done in the constructor since it can fail. So will have to create the marker and then see if it can parse the uri and if not error out, sounds ok?
Review of attachment 284180 [details] [review]: We could check if the search-string starts with geo: and send it to the same code path, yeah. Think that would be worthwhile?
Review of attachment 284180 [details] [review]: yeah. :)
Review of attachment 284195 [details] [review]: great
Review of attachment 284196 [details] [review]: ack
Comment on attachment 284195 [details] [review] main.c: Use the correct argv/argc Attachment 284195 [details] pushed as b5d5365 - main.c: Use the correct argv/argc
Created attachment 284377 [details] [review] Add uri command line option * placeEntry no emits an 'uri' signal on activation when the text starts with 'geo:'. * Did not move the parsing of uri to URIMarker, since the MapMarker base class expects a valid place in the constructor.
*** Bug 705604 has been marked as a duplicate of this bug. ***
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 756512 ***