GNOME Bugzilla – Bug 706726
Add a context menu
Last modified: 2013-08-31 18:34:14 UTC
See patches.
Created attachment 253035 [details] [review] mainWindow: Minor indentation fixes
Created attachment 253036 [details] [review] mapView: Minor simplification of code
Created attachment 253037 [details] [review] MapView: Separate showNGotoLocation method MapView now provides a separate method for showing a location and going to it while retaining the showLocation for only showing the location on the map.
Created attachment 253038 [details] [review] Add a context menu Currently this menu has only one item: "What's here?". If user activates this item, a reverse geocoding is performed for the coordinates where user right clicked and place information (currently only the name) is shown in bubble (just like for forward geocoding search results). Next would be to add a "I'm here" menu item to provide user the ability to correct (or improve the accuracy) of his/her location.
Review of attachment 253036 [details] [review]: Ack.
Review of attachment 253037 [details] [review]: Looks fine.
Review of attachment 253038 [details] [review]: Mixed tabs and spaces in indenting this file.
Review of attachment 253038 [details] [review]: Looks fine code wise otherwise. What I'm wondering is more behavior. When you press What is here, and start async reverse geocode the map becomes unresponsive until the location is found. You can't scroll or zoom. Or rather you can, but all such events will happen after the place is shown. Is there a way around that? And also it would be nice to have some kind of indication of that there is a search going on. If it takes some time to reveres you start to wonder if it has failed. Would it be possible to have some indicator, could we set the cursor? ::: src/context-menu.ui @@ +10,3 @@ + <property name="visible">True</property> + </object> + </child> Mixed tabs and spaces
(In reply to comment #8) > Review of attachment 253038 [details] [review]: > > Looks fine code wise otherwise. > > What I'm wondering is more behavior. When you press What is here, and start > async reverse geocode the map becomes unresponsive until the location is found. > You can't scroll or zoom. Or rather you can, but all such events will happen > after the place is shown. Is there a way around that? Oh? It shouldn't be like that. Here i can reproduce it but it typically only takes <=2 seconds from clicking "What's here" to result getting shown so the issue doesn't seem big. > And also it would be nice to have some kind of indication of that there is a > search going on. If it takes some time to reveres you start to wonder if it has > failed. > Would it be possible to have some indicator Yeah, I agree. > , could we set the cursor? The new way of doing this is: https://developer.gnome.org/gio/unstable/GApplication.html#g-application-mark-busy. Although I don't see any spinners when i call that, I'm hoping its because I'm running gnome-shell < 3.9. However, I'll update the patch to include these calls.
Created attachment 253056 [details] [review] Add a context menu v2: * Remove tabs from .ui file * Add GLib.Application.*mark_busy() calls.
Review of attachment 253056 [details] [review]: (Forgot to say Cool Stuff! last round) I see the spinner from the mark busy, looks good! (should we do the same for search instead of spinner in popup?) One small nit below, otherwise Ack. ::: src/contextMenu.js @@ +34,3 @@ + _init: function(mainWindow) { + this._mapView = mainWindow.mapView; + Is there a reason to pass in mainWindow to the constructor, and not just the mapView?
Review of attachment 253056 [details] [review]: >(Forgot to say Cool Stuff! last round) Thanks. >I see the spinner from the mark busy, looks good! Ah ok, thanks for confirming. (should we do the same for search instead of spinner in popup?) I don't think thats a good idea. This _mark_busy() is for situations where you'd previously use the spinning cursor. In fact, I'm not sure if my usage is appropriate for this API. I'd say lets do it this way for now and talk to designers to change it to something better in 3.12. ::: src/contextMenu.js @@ +34,3 @@ + _init: function(mainWindow) { + this._mapView = mainWindow.mapView; + Good question! I think this is coming from a previous approach where I did need the MainWindow itself. I can change this now.
Created attachment 253078 [details] [review] Add a context menu v3: Pass MapView to ContextMenu constructor.
Created attachment 253085 [details] [review] MapView: Make Geoclue property public
Created attachment 253086 [details] [review] Geoclue: Allow overriding location Currently this only updates the current concept of user location within Maps and hence user location will be reset on next Maps run. Making this permanent would probably involve implementing new API in Geoclue that allows (some specific white-listed) apps and administrators to set geoip (and later wlan) mapping overrides:
Created attachment 253087 [details] [review] ContextMenu: Add 'I am here!' menu item When launched, it overrides Maps's concept of user location. We also do a reverse geocoding to set a proper name on the user location pin.
Created attachment 253088 [details] [review] Geoclue: Allow overriding location v2: Re-added the accidently removed URL to geoclue bug.
Review of attachment 253085 [details] [review]: Ack.
Review of attachment 253087 [details] [review]: Ack.
Review of attachment 253088 [details] [review]: Ack.
Created attachment 253096 [details] Maps screenshot of weird effect. So, codewise this looks fine. But the ux isn't great at all for me. Which might be a geocode-reverse or nominatim issue. I tried this out around where I'm raised: Varberg. I lived outside of Varberg in Tvååker. So try this: Search for Varberg (Sweden, Hallands Län). Press Tvååker and choose "I am Here". Not only does it not find "Tvååker" (which will popup if you search får Tvååker and give correct location), it reverse geocodes to "Varberg". And it doesn't place the popup on Varberg, but out in the ocean. On the SS I pressed "I am Here" on Himle and that was the result.
No I pressed 'What is here'" of couse, not 'I am Here'
So it's two issues really. 1. The reverse isn't that good. I saw this when I played around with geo-uri as well. I found the lat/lon for Tvååker via Google Maps and ran it through geocode-reverse and it gave me Varberg. Even tho geocode-forward can find Tvååker fine. => So we do not always get the place name we expect. 2. The location given back from Reverse is not the one for Varberg in Forward. => So the marker for what is here does not popup where we expect. Both these might be geocode bugs, or limitations in nominatim. 2 could be fixed by using the lat/lon from x/y instead of the one in the geocode-place given back from Reverse, but that has its own downside I guess.
Not that 1) above have the same Reverse-naming issue even tho the placement works. Because the patch uses the x/y=>lat/lon to place the Pin. It will popup where we expect. But if we press on it it will have a name that is not what we expect. For instance it can popup in Tvååker but have the name Varberg.
So what is here founding the nearest thing it can find and plopping a pin there might be fine. But since we only use mapLocation.show on it in your patch, it might popup outside the visible view. So we do not see that anything poped up. And we will not know if it failed or poped up somewhere else, outside of view. This happend to me while playing around with it around Paris.
The above issue can be solved by moving the ensureVisible call from goTo to show in mapLocation. Which might make more sense, if we want to show something, we should ensure it is visible.
Ah. It's an locale issue! This is a bug on geocode-glib I guess. The problem aboce is that the constructor of geocode-reverse does: g_strdup_printf ("%g", geocode_location_get_latitude (location))); And for my swedish locale the double decimal-point is "," and this messes up everything. So this should be fixed in geocode-glib. Will create the bug. When on that subject, do you know why %g and not %f is used, it will give less decimal points with g, yes?
Review of attachment 253085 [details] [review]: Thanks for reviews. One request: Please also set commit status as part of reviews so its clear which patch needs work and which is good etc.
Comment on attachment 253087 [details] [review] ContextMenu: Add 'I am here!' menu item Setting patch status on behalf of Jonas.
Comment on attachment 253088 [details] [review] Geoclue: Allow overriding location Setting patch status on behalf of Jonas.
Created attachment 253277 [details] [review] Geoclue: Allow overriding location v2: * Initialize geoclue client and keep it around even if user overrides the location. * Start/Stop geoclue client & connect/disconnect to location update signals according to direction of location info flow (from/to user). https://bugs.freedesktop.org/show_bug.cgi?id=68536
Created attachment 253278 [details] [review] ContextMenu: Add 'I am here!' menu item v2: Rebased
Created attachment 253279 [details] [review] geoclue: Make findLocation() public
Created attachment 253280 [details] [review] MainWindow: Remove some dead code
Created attachment 253281 [details] [review] MainWindow: Allow switching back to auto location After user him/herself overrides location, allow him/her to switch back to automatic location detection through geoclue. We do this through the 'Goto my location' button.
Created attachment 253288 [details] [review] geoclue: Make findLocation() public v2: Update call to findLocation() as well.
Review of attachment 253035 [details] [review]: ACK
Review of attachment 253036 [details] [review]: ACK
Review of attachment 253037 [details] [review]: ACK
Review of attachment 253277 [details] [review]: ACK
Review of attachment 253278 [details] [review]: The code looks fine. ::: src/context-menu.ui @@ +14,3 @@ + <object class="GtkMenuItem" id="i-am-here-item"> + <property name="name">i-am-here-item</property> + <property name="label" translatable="yes">I'm here!</property> This label feels a bit too cheerful. Maybe we should ask a designer for a better label? @@ +17,3 @@ + <property name="visible">True</property> + </object> + </child> Why isn't this made using the new GMenu-stuff?
Review of attachment 253078 [details] [review]: I'm pretty sure we need to talk to designers about this at some point. I'm not sure what use case "What's here?" is trying to solve. If it's "I want to look for interesting POI's nearby" we need to take a harder look at that for 3.12. At GUADEC we talked about maybe doing POI search from the searchbar (restricted by the bbox of the currently visible map and some min zoom-level) and at the hackfest in June we talked about doing it via interacting with a pin (bubble). If it's on the other hand "I want to know what's exactly here, directly underneath my mouse pointer" I'm not sure when a user would want to know that? Please explain what you are trying to solve here. :) The code looks fine except for the GMenu comment. ::: src/context-menu.ui @@ +12,3 @@ + </child> + </object> +</interface> Why is this not made using the new GMenu stuff? (Like in the other bug).
Review of attachment 253280 [details] [review]: ACK
Review of attachment 253281 [details] [review]: ACK (except for small code style comment) ::: src/mainWindow.js @@ +276,3 @@ + (function() { + this.mapView.gotoUserLocation(true); + }).bind(this)); I prefer code like > this.mapView.gotoUserLocation.bind(this.mapView,true) instead of > (function() { > this.mapView.gotoUserLocation(true); > }).bind(this) but I don't think we agree on that, in which case I'm ok with either. :)
Review of attachment 253288 [details] [review]: ACK
Review of attachment 253036 [details] [review]: The spinner patch in bug#706891 needs the callback to be run even if the search fails, in order to stop/hide the spinner.
Review of attachment 253078 [details] [review]: > Please explain what you are trying to solve here. :) Its pretty simple I think: user be able to find out what a particular place is. Currently we are only showing the name but later we can show all kinds of details. >If it's "I want to look for interesting POI's nearby" Not at all. That would be a separate menu item if it goes into this menu. Jimmac found at least this item useful: <jimmac_> the "what is in here" seems like a much more interesting action ::: src/context-menu.ui @@ +12,3 @@ + </child> + </object> +</interface> I don't know how to. :( What other bug btw?
Review of attachment 253281 [details] [review]: ::: src/mainWindow.js @@ +276,3 @@ + (function() { + this.mapView.gotoUserLocation(true); + }).bind(this)); I'm fine with your way but that would make it even more harder to fit this under 80 char as per your other coding-style preference. :)
Review of attachment 253078 [details] [review]: Hm, I believe I understand now what you want to achieve. I think it might make more sense if the action of refining your location would be initiated from the location pin itself. Maybe by just dragging it. I also wonder if the "What's here?"-thing could be bound to just simple left-click? Would it make sense as a default action? ::: src/context-menu.ui @@ +12,3 @@ + </child> + </object> +</interface> I meant "other patch" (I mentioned this in the review of another patch). :) I'm ok with this btw.
(In reply to comment #51) > Review of attachment 253078 [details] [review]: > > Hm, I believe I understand now what you want to achieve. > > I think it might make more sense if the action of refining your location would > be initiated from the location pin itself. Maybe by just dragging it. Interesting idea! However I already got approval for this change in the break so as temporary measure I'll go with this if its not too bad? > I also wonder if the "What's here?"-thing could be bound to just simple > left-click? Would it make sense as a default action? That would overload nominatim for sure if we generate a reverse geocoding request for every click. :) Jimmac said that it might be a good idea to do that on long-press IIRC. > ::: src/context-menu.ui > @@ +12,3 @@ > + </child> > + </object> > +</interface> > > I meant "other patch" (I mentioned this in the review of another patch). :) > > I'm ok with this btw. Good, then please update the patch status. :)