After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 706726 - Add a context menu
Add a context menu
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on: 706786
Blocks:
 
 
Reported: 2013-08-25 01:23 UTC by Zeeshan Ali
Modified: 2013-08-31 18:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mainWindow: Minor indentation fixes (1.55 KB, patch)
2013-08-25 01:23 UTC, Zeeshan Ali
committed Details | Review
mapView: Minor simplification of code (1.23 KB, patch)
2013-08-25 01:23 UTC, Zeeshan Ali
rejected Details | Review
MapView: Separate showNGotoLocation method (1.44 KB, patch)
2013-08-25 01:23 UTC, Zeeshan Ali
committed Details | Review
Add a context menu (6.26 KB, patch)
2013-08-25 01:23 UTC, Zeeshan Ali
none Details | Review
Add a context menu (6.43 KB, patch)
2013-08-25 13:54 UTC, Zeeshan Ali
none Details | Review
Add a context menu (6.42 KB, patch)
2013-08-25 23:13 UTC, Zeeshan Ali
committed Details | Review
MapView: Make Geoclue property public (1.52 KB, patch)
2013-08-26 01:21 UTC, Zeeshan Ali
committed Details | Review
Geoclue: Allow overriding location (3.49 KB, patch)
2013-08-26 01:22 UTC, Zeeshan Ali
none Details | Review
ContextMenu: Add 'I am here!' menu item (3.46 KB, patch)
2013-08-26 01:22 UTC, Zeeshan Ali
accepted-commit_now Details | Review
Geoclue: Allow overriding location (3.54 KB, patch)
2013-08-26 01:25 UTC, Zeeshan Ali
accepted-commit_now Details | Review
Maps screenshot of weird effect. (758.78 KB, image/png)
2013-08-26 07:28 UTC, Jonas Danielsson
  Details
Geoclue: Allow overriding location (7.01 KB, patch)
2013-08-27 17:02 UTC, Zeeshan Ali
committed Details | Review
ContextMenu: Add 'I am here!' menu item (3.46 KB, patch)
2013-08-27 17:03 UTC, Zeeshan Ali
committed Details | Review
geoclue: Make findLocation() public (1.82 KB, patch)
2013-08-27 17:03 UTC, Zeeshan Ali
none Details | Review
MainWindow: Remove some dead code (1.14 KB, patch)
2013-08-27 17:03 UTC, Zeeshan Ali
committed Details | Review
MainWindow: Allow switching back to auto location (1.25 KB, patch)
2013-08-27 17:03 UTC, Zeeshan Ali
committed Details | Review
geoclue: Make findLocation() public (2.11 KB, patch)
2013-08-27 18:56 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2013-08-25 01:23:22 UTC
See patches.
Comment 1 Zeeshan Ali 2013-08-25 01:23:25 UTC
Created attachment 253035 [details] [review]
mainWindow: Minor indentation fixes
Comment 2 Zeeshan Ali 2013-08-25 01:23:34 UTC
Created attachment 253036 [details] [review]
mapView: Minor simplification of code
Comment 3 Zeeshan Ali 2013-08-25 01:23:38 UTC
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.
Comment 4 Zeeshan Ali 2013-08-25 01:23:43 UTC
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.
Comment 5 Jonas Danielsson 2013-08-25 02:19:36 UTC
Review of attachment 253036 [details] [review]:

Ack.
Comment 6 Jonas Danielsson 2013-08-25 02:19:41 UTC
Review of attachment 253037 [details] [review]:

Looks fine.
Comment 7 Jonas Danielsson 2013-08-25 02:19:44 UTC
Review of attachment 253038 [details] [review]:

Mixed tabs and spaces in indenting this file.
Comment 8 Jonas Danielsson 2013-08-25 02:23:09 UTC
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
Comment 9 Zeeshan Ali 2013-08-25 13:47:44 UTC
(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.
Comment 10 Zeeshan Ali 2013-08-25 13:54:54 UTC
Created attachment 253056 [details] [review]
Add a context menu

v2: 

* Remove tabs from .ui file
* Add GLib.Application.*mark_busy() calls.
Comment 11 Jonas Danielsson 2013-08-25 20:06:38 UTC
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?
Comment 12 Zeeshan Ali 2013-08-25 23:08:53 UTC
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.
Comment 13 Zeeshan Ali 2013-08-25 23:13:15 UTC
Created attachment 253078 [details] [review]
Add a context menu

v3: Pass MapView to ContextMenu constructor.
Comment 14 Zeeshan Ali 2013-08-26 01:21:57 UTC
Created attachment 253085 [details] [review]
MapView: Make Geoclue property public
Comment 15 Zeeshan Ali 2013-08-26 01:22:06 UTC
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:
Comment 16 Zeeshan Ali 2013-08-26 01:22:10 UTC
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.
Comment 17 Zeeshan Ali 2013-08-26 01:25:22 UTC
Created attachment 253088 [details] [review]
Geoclue: Allow overriding location

v2: Re-added the accidently removed URL to geoclue bug.
Comment 18 Jonas Danielsson 2013-08-26 07:02:29 UTC
Review of attachment 253085 [details] [review]:

Ack.
Comment 19 Jonas Danielsson 2013-08-26 07:23:30 UTC
Review of attachment 253087 [details] [review]:

Ack.
Comment 20 Jonas Danielsson 2013-08-26 07:23:52 UTC
Review of attachment 253088 [details] [review]:

Ack.
Comment 21 Jonas Danielsson 2013-08-26 07:28:07 UTC
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.
Comment 22 Jonas Danielsson 2013-08-26 07:36:33 UTC
No I pressed 'What is here'" of couse, not 'I am Here'
Comment 23 Jonas Danielsson 2013-08-26 07:44:13 UTC
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.
Comment 24 Jonas Danielsson 2013-08-26 08:29:15 UTC
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.
Comment 25 Jonas Danielsson 2013-08-26 09:02:19 UTC
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.
Comment 26 Jonas Danielsson 2013-08-26 09:16:59 UTC
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.
Comment 27 Jonas Danielsson 2013-08-26 09:59:31 UTC
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?
Comment 28 Zeeshan Ali 2013-08-27 00:43:40 UTC
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 29 Zeeshan Ali 2013-08-27 00:44:23 UTC
Comment on attachment 253087 [details] [review]
ContextMenu: Add 'I am here!' menu item

Setting patch status on behalf of Jonas.
Comment 30 Zeeshan Ali 2013-08-27 00:44:40 UTC
Comment on attachment 253088 [details] [review]
Geoclue: Allow overriding location

Setting patch status on behalf of Jonas.
Comment 31 Zeeshan Ali 2013-08-27 17:02:58 UTC
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
Comment 32 Zeeshan Ali 2013-08-27 17:03:19 UTC
Created attachment 253278 [details] [review]
ContextMenu: Add 'I am here!' menu item

v2: Rebased
Comment 33 Zeeshan Ali 2013-08-27 17:03:32 UTC
Created attachment 253279 [details] [review]
geoclue: Make findLocation() public
Comment 34 Zeeshan Ali 2013-08-27 17:03:40 UTC
Created attachment 253280 [details] [review]
MainWindow: Remove some dead code
Comment 35 Zeeshan Ali 2013-08-27 17:03:51 UTC
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.
Comment 36 Zeeshan Ali 2013-08-27 18:56:27 UTC
Created attachment 253288 [details] [review]
geoclue: Make findLocation() public

v2: Update call to findLocation() as well.
Comment 37 Mattias Bengtsson 2013-08-28 01:20:18 UTC
Review of attachment 253035 [details] [review]:

ACK
Comment 38 Mattias Bengtsson 2013-08-28 01:21:09 UTC
Review of attachment 253036 [details] [review]:

ACK
Comment 39 Mattias Bengtsson 2013-08-28 01:22:37 UTC
Review of attachment 253037 [details] [review]:

ACK
Comment 40 Mattias Bengtsson 2013-08-28 01:33:08 UTC
Review of attachment 253277 [details] [review]:

ACK
Comment 41 Mattias Bengtsson 2013-08-28 02:46:21 UTC
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?
Comment 42 Mattias Bengtsson 2013-08-28 03:24:36 UTC
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).
Comment 43 Mattias Bengtsson 2013-08-28 03:25:17 UTC
Review of attachment 253280 [details] [review]:

ACK
Comment 44 Mattias Bengtsson 2013-08-28 03:30:04 UTC
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. :)
Comment 45 Mattias Bengtsson 2013-08-28 03:30:18 UTC
Review of attachment 253280 [details] [review]:

ACK
Comment 46 Mattias Bengtsson 2013-08-28 03:31:09 UTC
Review of attachment 253288 [details] [review]:

ACK
Comment 47 Jonas Danielsson 2013-08-28 07:08:07 UTC
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.
Comment 48 Zeeshan Ali 2013-08-28 14:04:56 UTC
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?
Comment 49 Zeeshan Ali 2013-08-28 14:07:36 UTC
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?
Comment 50 Zeeshan Ali 2013-08-28 14:10:33 UTC
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. :)
Comment 51 Mattias Bengtsson 2013-08-29 02:26:16 UTC
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.
Comment 52 Zeeshan Ali 2013-08-29 12:17:27 UTC
(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. :)