GNOME Bugzilla – Bug 705604
handle location links
Last modified: 2015-08-17 12:16:42 UTC
It would be nice for Maps to handle location like URIs. Found a couple references: * http://msdn.microsoft.com/en-us/library/windows/apps/jj635237.aspx * http://en.wikipedia.org/wiki/Geo_URI * maps:q=<physical location> * http://developer.apple.com/library/ios/#featuredarticles/iPhoneURLScheme_Reference/Articles/MapLinks.html * http://developer.android.com/guide/appendix/g-app-intents.html
Created attachment 251850 [details] [review] Test patch for --uri and geo uri A test patch that works with the patch currently in the blocking geocode-glib bug. It allows you to start maps with --uri <geouri> Example: gnome-maps --uri "geo:13.4125,103.8667"
(In reply to comment #1) > Created an attachment (id=251850) [details] [review] > Test patch for --uri and geo uri > > A test patch that works with the patch currently in the blocking geocode-glib > bug. It allows you to start maps with --uri <geouri> > > Example: gnome-maps --uri "geo:13.4125,103.8667" The geocode-glib work landed a while ago. I think this also went in around the same time. :(
I haven't submitted a real patch to do the URI thing yet. Partly because other things seemed more important and partly because I wanted to do argument parsing The Right Way. Which I don't know what it is in gjs. The regular gnome commandline things are not possible I think. If you want I can cook up something, but I think it can wait until after 3.12.
(In reply to comment #3) > I haven't submitted a real patch to do the URI thing yet. Partly because other > things seemed more important and partly because I wanted to do argument parsing > The Right Way. Which I don't know what it is in gjs. > > The regular gnome commandline things are not possible I think. Ouch. > If you want I can cook up something, but I think it can wait until after 3.12. Sure. I was hoping that its just a matter of you having ready patches in your personal repo and just forgetting about them. :)
Created attachment 272683 [details] [review] A patch to handle the starting geo URI I looked at GNOME Documents as to how they are dealing with commandline arguments: the only reference I saw was they used a function create a file from a command line argument [0]. This is out of scope for our use, since it offers no support for options parsing. [0] - search.js:330: let trackerFile = Gio.file_new_for_commandline_arg(location); There seems two options available for commandline parsing, as per http://lazka.github.io/pgi-docs/api/Gio_2.0/classes/Application.html#Gio.Application.run a) GApplicationCommandline (and the virtual function command-line) that Jonas used in his patch above. This doesn't allow for advanced options parsing. b) as they mentioned in the above document, "Alternatively, you may use the GLib.OptionContext API, after setting argc = g_strv_length (argv);." I am trying this approach, somehow trying to use the OptionContext. For some reason, I am unable to create an object out of this structure, there doesn't seem to be present a constructor (which could be a bug in glib). I couldn't find any other GJS project who are using OptionContext for parsing arguments. This patch is a port of Jonas' patch to latest geocode-glib where support for uri parsing was moved from Reverse class to Location class. Also, added a change to goto the location first before trying to reverse geocode (asynchronously).
(In reply to comment #5) > > There seems two options available for commandline parsing, as per > http://lazka.github.io/pgi-docs/api/Gio_2.0/classes/Application.html#Gio.Application.run > > a) GApplicationCommandline (and the virtual function command-line) that Jonas > used in his patch above. This doesn't allow for advanced options parsing. In later versions of GApplication there is an option to add OptionEntries via Gio.Applications add_main_option_entries and get option parsing but ... > b) as they mentioned in the above document, "Alternatively, you may use the > GLib.OptionContext API, after setting argc = g_strv_length (argv);." > I am trying this approach, somehow trying to use the OptionContext. For some > reason, I am unable to create an object out of this structure, there doesn't > seem to be present a constructor (which could be a bug in glib). > > I couldn't find any other GJS project who are using OptionContext for parsing > arguments. > Yes this is correct. There are two problems with using OptionContext. One is that it requires one to pass an pointer to where the supplied argument should be stored. Something we can't do in a bounded environment as gjs. The other is that OptionEntry is not a GObject or a GBoxed object and thus can't be instantiated. So for now this is not possible. And event if we make OptionEntry a boxed type and pass it to add_main_option_entries which accept null as arg_data it expects a struct passed by value and not by pointer, so it's tricky.
Review of attachment 272683 [details] [review]: Thanks for the patch! Please for the next version consider creating the patch using git. Committing the change in your local repository with a commit log that follow the GNOME guidelines,found here: https://wiki.gnome.org/Git/CommitMessages Then you can create the patch using git format-patch -1. Another way that is great if you plan to continue contributing to GNOME or other bugzilla based projects is looking at git-bz that allows you to attach your patch to a bug from the command line with git bz attach and applying patches from bugs with git bz apply. I have some comments below: ::: src/application.js @@ +56,3 @@ + this._uri = null; + Some whitespace damage here. ::: src/mainWindow.js @@ +68,3 @@ + + if (uri) { + let location = Geocode.Location.new(0, 0, 0); This could be written as let location = new Geocode.Location(); @@ +69,3 @@ + if (uri) { + let location = Geocode.Location.new(0, 0, 0); + location.set_from_uri(uri); We need to at least check the boolean return value and maybe we should even wrap this in a try-catch block to get the GError message if it fails. @@ +70,3 @@ + let location = Geocode.Location.new(0, 0, 0); + location.set_from_uri(uri); + let mapLocation = new MapLocation.MapLocation(Geocode.Place.new_with_location("Starting location", Whitespace damage here at the end of the line. And it's also a really long line :) Could something be done about that? Maybe we can create the place with something like: let mapLocation = new MapLocation.MapLocation(new Geocode.Place({ name: 'Starting Location', place_type: Geocode.PlaceType.UNKNOWN, location: location }), this.mapView); Although I am not sure about 'Starting Location', what is your thinking? Another possibility is just using the lat/lon, I am just not sure. Maybe someone else can weigh in as well. @@ +86,3 @@ + e.message); + } + }).bind(this)); I know I had this in my example patch, but I am not sure about it. Do we always want to reverse the uri? If I give someone an URI to someplace I want them to check out, it might not be that I am referring to the closest place that we are able to reverse geocode. Could something fancier be done? Like showing the uri place and also some noteworthy places nearby? I am not sure. And speaking of things I am not sure of. I am not sure all this code belongs in mainWindow. Maybe we need a way to create a mapLocation from uri?
Created attachment 273022 [details] [review] Updated patch to handle the starting geo URI Hi Jonas, Thanks for taking the time to review the patch. I shall take care of the above points in future. I am slightly unsure about the whitespace damage part. I'm using gedit, and I'm trying to be consistent with the rest of the style. I've attached an updated patch. Please view my comments for your suggestions inline: > ::: src/application.js > @@ +56,3 @@ > > + this._uri = null; > + > > Some whitespace damage here. I hope it is fixed in the current patch. > > ::: src/mainWindow.js > @@ +68,3 @@ > + > + if (uri) { > + let location = Geocode.Location.new(0, 0, 0); > > This could be written as > let location = new Geocode.Location(); Thanks for the advice, I've fixed this. I was hoping for something like Geocode.Location.new_from_uri(uri) like method being available, but your suggestion seems like a good one too. > > @@ +69,3 @@ > + if (uri) { > + let location = Geocode.Location.new(0, 0, 0); > + location.set_from_uri(uri); > > We need to at least check the boolean return value and maybe we should even > wrap this in a try-catch block to get the GError message if it fails. Fixed in updated patch. > > @@ +70,3 @@ > + let location = Geocode.Location.new(0, 0, 0); > + location.set_from_uri(uri); > + let mapLocation = new > MapLocation.MapLocation(Geocode.Place.new_with_location("Starting location", > > Whitespace damage here at the end of the line. And it's also a really long line > :) > Could something be done about that? Maybe we can create the place with > something like: > let mapLocation = new MapLocation.MapLocation(new Geocode.Place({ > name: 'Starting Location', > place_type: Geocode.PlaceType.UNKNOWN, > location: location > }), this.mapView); Tried to fix it like this: let place = Geocode.Place.new_with_location(location.get_latitude() + ", " + location.get_longitude(), Geocode.PlaceType.UNKNOWN, location); (spans 92 columns, and I've seen some other lines reach 93). Do you think this is fine? > > Although I am not sure about 'Starting Location', what is your thinking? > Another possibility is just using the lat/lon, I am just not sure. Maybe > someone else can weigh in as well. Thanks for the suggestion. I was out of ideas what to do there, but lat, lng seems the best idea here. > > @@ +86,3 @@ > + e.message); > + } > + }).bind(this)); > > I know I had this in my example patch, but I am not sure about it. I didn't quite understand what the bind() does here, so left it as is. > > Do we always want to reverse the uri? > If I give someone an URI to someplace I want them to check out, it might not be > that I am referring to the closest place that we are able to reverse geocode. > Could something fancier be done? Like showing the uri place and also some > noteworthy places nearby? I am not sure. I feel reverse geocode gives a good reference. However, one concern is that the geouri could refer to a city (i.e. a city center, when zoom is sufficiently out) and in that case the street level reverse geocoded description shown there doesn't make much sense, as it loses the original context with which the parameter was passed in. > > And speaking of things I am not sure of. I am not sure all this code belongs in > mainWindow. Maybe we need a way to create a mapLocation from uri? I was unsure as well. Initially, I wanted to do it in MapLocation by creating a constructor that accepted a location or uri, but I couldn't think of a good way of overloading the current MapLocation constructor. In the updated patch, I've just added a reverseGeocode() method (with a callback) in MapLocation, which abstracts away some of MapLocation specific functionality. What do you think about it? Regards, Mitali
Review of attachment 273022 [details] [review]: Thanks for the updated patch! Some comments below! ::: src/application.js @@ +86,3 @@ + return 0; + }, + One blank line to many. ::: src/mainWindow.js @@ -62,3 @@ this.mapView = new MapView.MapView(); ui.windowContent.add(this.mapView); - The removal of a line here is unrelated to the patch and should not be included. @@ +74,3 @@ + } catch (e) { + log ("Error setting location from uri (" + uri + "): " + e.message); + } This seem cumbersome. Could we do this without the couldParse variable? Instead add the code to handle the uri inside the try block. Then you do not need to check the return of the set_from_uri. Also, I do not really like having log outputs for error. But since we do not have any way to do in-app notifications yet we might have to. Alternative you could have this bug depend on bug #723996 that adds in-app notifications and have this code add a notification. @@ +79,3 @@ + let place = Geocode.Place.new_with_location(location.get_latitude() + ", " + + location.get_longitude(), Geocode.PlaceType.UNKNOWN, location); + let mapLocation = new MapLocation.MapLocation(place, this.mapView); I think I prefer the way I suggested to create the place, also you can access the properties directly: let mapLocation = new MapLocation.MapLocation(new Geocode.Place({ name: location.longitude + ' ' + location.latitude, place_type: Geocode.PlaceType.UNKNOWN, location: location }), this.mapView); @@ +81,3 @@ + let mapLocation = new MapLocation.MapLocation(place, this.mapView); + mapLocation.goTo(false); + mapLocation.reverseGeocode(function(place, mapView) { I do not think this callback should include the mapView. if you write the anonymous function like: ... reverseGeocode((function(place) { [...] }).bind(this)); Then you can access the mapView as this._mapView. @@ +84,3 @@ + mapView.showLocation(place); + }); + } Again, I am not sure about reversing and would like to have Mattias and Zeeshans input. ::: src/mapLocation.js @@ +70,3 @@ + let place = reverse.resolve_finish(res); + this.description = place.location.description; + reverseCompletionCallback(place, this._mapView); Do not include mapView. @@ +75,3 @@ + this.latitude + ", " + + this.longitude + ": " + + e.message); I think this should be an Utils.debug output.
Review of attachment 273022 [details] [review]: Another comment, please look at the Gnome commit message guidelines, here: https://wiki.gnome.org/Git/CommitMessages To see how we want commit messages to look! Thanks!
(In reply to comment #8) > Created an attachment (id=273022) [details] [review] > Updated patch to handle the starting geo URI > > Hi Jonas, > Thanks for taking the time to review the patch. I shall take care of the above > points in future. I am slightly unsure about the whitespace damage part. I'm > using gedit, and I'm trying to be consistent with the rest of the style. > Hi, One way to check for whitespace damage is to use git. If you are working on a branch you can do: git diff master --check and get information about whitespace damage. Or If you want to check against latest commit, you can go: git diff HEAD~1 --check
Created attachment 273087 [details] [review] Handle a geo URI passed in as a commandline option Hi Jonas, Thanks again for your valuable comments. This helps me learn a lot about good practices here and I truly appreciate it. Please find my comments inline: > ::: src/application.js > @@ +86,3 @@ > + return 0; > + }, > + > > One blank line to many. Fixed in updated patch. > > ::: src/mainWindow.js > @@ -62,3 @@ > this.mapView = new MapView.MapView(); > ui.windowContent.add(this.mapView); > - > > The removal of a line here is unrelated to the patch and should not be > included. Fixed in updated patch. > @@ +74,3 @@ > + } catch (e) { > + log ("Error setting location from uri (" + uri + "): " + > e.message); > + } > > This seem cumbersome. Could we do this without the couldParse variable? Instead > add the code to handle the uri inside the try block. Then you do not need to > check the return of the set_from_uri. Apart from this couldParse flag, I couldn't find a clean way to disable functionality that follows the set_from_uri. One approach was to include all of the following functionality into the same try-catch block for the set_from_uri, but that seemed inefficient, since the try-catch could potentially catch other issues in the same block. Should I try this? > > Also, I do not really like having log outputs for error. But since we do not > have any way to do in-app notifications yet we might have to. Alternative you > could have this bug depend on bug #723996 that adds in-app notifications and > have this code add a notification. That's a good point. I couldn't find a way to output to the error console in GJS. I will try to take a look at #723996 and come up with a patch for that later. Leaving it with log() for now. > > @@ +79,3 @@ > + let place = > Geocode.Place.new_with_location(location.get_latitude() + ", " + > + location.get_longitude(), Geocode.PlaceType.UNKNOWN, > location); > + let mapLocation = new MapLocation.MapLocation(place, > this.mapView); > > I think I prefer the way I suggested to create the place, also you can access > the properties directly: > > let mapLocation = new MapLocation.MapLocation(new Geocode.Place({ > name: location.longitude + ' ' + location.latitude, > place_type: Geocode.PlaceType.UNKNOWN, > location: location > }), this.mapView); Done as per your suggestion. Thanks. > > @@ +81,3 @@ > + let mapLocation = new MapLocation.MapLocation(place, > this.mapView); > + mapLocation.goTo(false); > + mapLocation.reverseGeocode(function(place, mapView) { > > I do not think this callback should include the mapView. > > if you write the anonymous function like: > > ... reverseGeocode((function(place) { > [...] > }).bind(this)); > > Then you can access the mapView as this._mapView. I see. Now, I understand why bind(this) is used. However, for this patch, I've used the mapLocations._mapView, since I felt that was cleaner. What do you think? > ::: src/mapLocation.js > @@ +70,3 @@ > + let place = reverse.resolve_finish(res); > + this.description = place.location.description; > + reverseCompletionCallback(place, this._mapView); > > Do not include mapView. Removed. > @@ +75,3 @@ > + this.latitude + ", " + > + this.longitude + ": " + > + e.message); > > I think this should be an Utils.debug output. Fixed to Utils.debug(). > Another comment, please look at the Gnome commit > message guidelines, here: > https://wiki.gnome.org/Git/CommitMessages > To see how we want commit messages to look! I tried to follow the guidelines. I couldn't figure out the correct "tag" to use in the commit message first line. Have I missed something? Thanks again for taking time to review! Regards, Mitali
Created attachment 273089 [details] [review] Handle a geo URI passed in as a commandline option The description line of commit message contains not more than 74 characters per line. I missed that in my previous patch.
Review of attachment 273087 [details] [review]: Hi, thanks for the updates! The tag for the commit message is not always relevant, it is used most often when changes affect a single module. The line in the message body looks a bit to long to me, the guidelines say that lines in the body should not exceed 74 characters. And there is no need to credit me in the log :) This patch also needs to be rebased against current master. Since master has been updated. Thanks again, the patch is looking good, just a few more iterations :) ::: src/mainWindow.js @@ +85,3 @@ + mapLocation.goTo(false); + mapLocation.reverseGeocode(function(place) { + mapLocation._mapView.showLocation(place); When a member variable is prefixed with an underscore, such as the mapView property of MapLocation is here it means that it is to be considered private. And we should not access them outside of MapLocation. So you should use the bind trick and use this._mapView. @@ +89,3 @@ + } + } + But also, I still wonder if we can move this from mainWindow in some way. MainWindow seem to grow and grow with each new feature :) I wonder if you could maybe create a function in mapLocation.js, a function that lives outside of the MapLocation object. Something like: function newFromURI(uri) { [...] } (or other name similar, I am terrible with names). And this function can create and return the new mapLocation. And then we could just have mapLocation.goTo in mainWindow. Myabe that could work. Mattias: what do you think? Mattias: I would also like a discussion here whether we should reverse or not. Or if should reverse if the distance from the reversed location is within some sort of limit from the uri.
(In reply to comment #12) > > > > Also, I do not really like having log outputs for error. But since we do not > > have any way to do in-app notifications yet we might have to. Alternative you > > could have this bug depend on bug #723996 that adds in-app notifications and > > have this code add a notification. > > That's a good point. I couldn't find a way to output to the error console in > GJS. I will try to take a look at #723996 and come up with a patch for that > later. Leaving it with log() for now. > Bug #723996 has been committed to master now so you should be able to use something like Application.notificationManager.showError(msg);
Review of attachment 273087 [details] [review]: Mitali: I'm sorry for yet another late reply. Anyways from a quick test of the patch I saw that it only shows the location on the first run of gnome-maps. I think if you are already running Maps and run $ gnome-maps --uri "geo:13.4125,103.8667" A bubble should show on the running instance. Comments on the code below: ::: src/mainWindow.js @@ +71,3 @@ + let location = new Geocode.Location(); + try { + couldParse = location.set_from_uri(uri); I'd skip this couldParse variable and just put the code inside the try-block. @@ +89,3 @@ + } + } + let place = new Geocode.Place({ Yeah I agree with Jonas here about a newFromURI function in mapLocation.js I'm not sure about reverse geocoding here. It might be that the user wants to point at some specific point on the map, like: "My lost phone should be around here". <tangent> Later on (after this patch is pushed, when we finally attack the markers and bubbles designs) this should maybe add some sort of anonymous pin. That is a pin without a label and stuff, but with the possibility to init a route search and maybe (?) add as a favourite etc. Those could also potentially be added via the context menu "drop a pin here!" (or something a little less cheery). </tangent> I think the current behavior could be to just show the lat lng pair as the label (really poor ui, but this isn't very discoverable now anyways).
This option would probably be used to pass along locations from other applications. For example, open a map with the location of a business, similarly to what iOS does: https://developer.apple.com/library/mac/documentation/MapKit/Reference/MKMapItem_class/Reference/Reference.html#//apple_ref/occ/clm/MKMapItem/openMapsWithItems:launchOptions: The pre-iOS6 version of it uses "maps:" as the URI scheme: http://schwiiz.org/?p=1552 Can additional information such as the name of the location be encoded in the geo: URI?
(In reply to comment #17) > This option would probably be used to pass along locations from other > applications. For example, open a map with the location of a business, > similarly to what iOS does: > https://developer.apple.com/library/mac/documentation/MapKit/Reference/MKMapItem_class/Reference/Reference.html#//apple_ref/occ/clm/MKMapItem/openMapsWithItems:launchOptions: > Yeah, that would indeed be nice. > The pre-iOS6 version of it uses "maps:" as the URI scheme: > http://schwiiz.org/?p=1552 > We could add maps: as a scheme that geocode could parse and get it for free in Maps. > Can additional information such as the name of the location be encoded in the > geo: URI? It _could_, the RFC tells us how new parameters fits in the scheme, but there are interoperability concerns, I guess. If we just add it it would be an un-official gnome-extension and technically not 'geo' URI anymore. The offcial way would be to try to register a parameter with IANA. Let's say "description" or "name". The RFC says about this: 4. 'geo' URI Parameters Registry This specification creates a new IANA Registry named "'geo' URI Parameters" registry for the <parameter> component of the URI. Parameters for the 'geo' URI and values for these parameters MUST be registered with IANA to prevent namespace collisions and provide interoperability. [...] Documents defining new parameters (or new values for existing parameters) MUST register them with IANA, as explained in Section 8.2. Section 8.2.1: When registering a new 'geo' URI Parameter, the following information MUST be provided: o Name of the Parameter. o Whether the Parameter accepts no value ("No value"), values from a predefined set ("Predefined"), or values constrained by a syntax only ("Constrained"). o Reference to the RFC or other permanent and readily available public specification defining the parameters and the new values. Unless specific instructions exist for a Parameter (like the definition of a Sub-registry), the following information MUST be provided when registering new values for existing "Predefined" 'geo' URI Parameters: o Name of the Parameter. o Reference to the RFC or other permanent and readily available public specification defining the new values. The following table provides the initial values for this registry: Parameter Name Value Restriction Reference(s) ---------------------------------------------------------- crs Predefined [RFC5870] u Constrained [RFC5870] No extra parameters outside of the RFC seem to be registered yet. We could do that. Or we could just make geocode understande an UTF-8 parameter called description. All valid geo URIs would still be parsable and it would be an extension for us.
(In reply to comment #18) > (In reply to comment #17) > > This option would probably be used to pass along locations from other > > applications. For example, open a map with the location of a business, > > similarly to what iOS does: > > https://developer.apple.com/library/mac/documentation/MapKit/Reference/MKMapItem_class/Reference/Reference.html#//apple_ref/occ/clm/MKMapItem/openMapsWithItems:launchOptions: > > > > Yeah, that would indeed be nice. > > > The pre-iOS6 version of it uses "maps:" as the URI scheme: > > http://schwiiz.org/?p=1552 > > > > We could add maps: as a scheme that geocode could parse and get it for free in > Maps. > > > Can additional information such as the name of the location be encoded in the > > geo: URI? > > It _could_, the RFC tells us how new parameters fits in the scheme, but there > are interoperability concerns, I guess. If we just add it it would be an > un-official gnome-extension and technically not 'geo' URI anymore. The offcial > way would be to try to register a parameter with IANA. Let's say "description" > or "name". Android and Google Maps support: geo:?q=lat,lon(label) http://developer.android.com/guide/components/intents-common.html#Maps Should we implement that extension to the protocol?
Yeah we could do that, I'll cook up a patch for geocode when I get some time and we can see how it looks.
(In reply to comment #20) > Yeah we could do that, I'll cook up a patch for geocode when I get some time > and we can see how it looks. bug #727861
*** This bug has been marked as a duplicate of bug 735215 ***