Sorry, mispelled the link [1] reference.

Created attachment 285071 [details] [review]
placeEntry: Add support for plain coordinates

This adds support for parsing plain coordinates with the format
"<latitude>, <longitude>" in PlaceEntry class. When PlaceEntry
detects a valid plain coordinate (valid format, between valid ranges)
being introduced, it will skip the place search, also, no reverse
search is performed.

Plain coordinates places are internally represented by a GeocodePlace
object whose "name" property is null.

Created attachment 285072 [details] [review]
turnPointMarker: Adapt to PlaceEntry's plain coordinates support

Created attachment 285073 [details] [review]
searchResultBubble: Show plain coordinates based places properly

Please note that this patch-set must be applied after #731068 patch-set.

Review of attachment 285071 [details] [review]:

Thanks!

::: src/placeEntry.js
@@ +124,3 @@
 
+    _parseCoordinates: function(text) {
+        let match = /^\s*(\-?[0-9]+(?:\.[0-9]+)?)\s*,\s*(\-?[0-9]+(?:\.[0-9]+)?)\s*$/g.exec(text);

When grepping in gnome-shell I think that the construct:

let matches = text.match(...) is more common.

Also if you find a way to shorten that line it would be nice :)
And maybe a common somewhere on what it is matching? Parsing regexp is not always the most fun you can have before drinking your first cup of coffee.

Is regex called for? Could it be parsed with simpler means?

@@ +145,3 @@
         }
 
+        let parsedCoordinates = this._parseCoordinates(this.text);

Maybe a comment above about what we are doing here?

Also, maybe let location = ...? And return a Geocode.Location?

Review of attachment 285072 [details] [review]:

Looks fine.

The commit shortlog is a bit long, maybe:

turnPointerMarker: Remove custom name

And a sentence in the message about it is no longer needed.

Review of attachment 285073 [details] [review]:

When is this ever needed?

Review of attachment 285071 [details] [review]:

::: src/placeEntry.js
@@ +123,3 @@
     },
 
+    _parseCoordinates: function(text) {

Also is coords or similar more descriptive than text?

Review of attachment 285071 [details] [review]:

::: src/placeEntry.js
@@ +123,3 @@
     },
 
+    _parseCoordinates: function(text) {

We don't know if they are coords before parsing :), we want to parse a piece of "text" to get coordinates.

@@ +124,3 @@
 
+    _parseCoordinates: function(text) {
+        let match = /^\s*(\-?[0-9]+(?:\.[0-9]+)?)\s*,\s*(\-?[0-9]+(?:\.[0-9]+)?)\s*$/g.exec(text);

Maybe moving the regex to a constant, add a comment to the constant and replace [0-9] for \d can keep the code more clean.

Yes, parse an unowned regex is not an easy task... you usually need to trust on it ;)

@@ +145,3 @@
         }
 
+        let parsedCoordinates = this._parseCoordinates(this.text);

Yeah, maybe that'd be more clean.

Review of attachment 285073 [details] [review]:

When you search for a plain coordinate in the main search entry, what should we show? without this change we would show a bubble with a pin image and no text, I tried to show something neater.

Review of attachment 285071 [details] [review]:

::: src/placeEntry.js
@@ +124,3 @@
 
+    _parseCoordinates: function(text) {
+        let match = /^\s*(\-?[0-9]+(?:\.[0-9]+)?)\s*,\s*(\-?[0-9]+(?:\.[0-9]+)?)\s*$/g.exec(text);

How about the possibility of an optional label/description? So that for instance user loation can set "current location" ? And for when we get uri support with the geouri extension of setting a label?

"label: lat, lon" or something like that.

Review of attachment 285073 [details] [review]:

::: src/searchResultBubble.js
@@ +51,3 @@
 
+        if (!place.name) {
+            title = _("Unknown location");

Could we do this without introducing a translatable string? (if we want to sneak this in). We could show a label if there is one, or just the coordinates if not.

Also maybe this should be a more general bubble? So that it could also be used for uri and other non-named locations. The searchResultMarker could show that bubble if !place.name.

Review of attachment 285073 [details] [review]:

::: src/searchResultBubble.js
@@ +51,3 @@
 
+        if (!place.name) {
+            title = _("Unknown location");

You're right on the translatable string, I noticed that too.

From the choices that you mention, I do prefer to not show any bubble when !place.name, showing coordinates in the bubble as a title may look ugly.

Actually yes... SearchResultBubble has some reusable code to generate bubble content for different kind of places. About the uri feature, what about fill the search bar with opened location and fire the search?

(In reply to comment #12)
> How about the possibility of an optional label/description? So that for
> instance user loation can set "current location" ? And for when we get uri
> support with the geouri extension of setting a label?
> 
> "label: lat, lon" or something like that.

After discuss it a little bit with Jonas, we conclude that it'd be a good idea to make some little parsing in PlaceEntry to support geo: URIs too.

This will affect #735215, where we plan to use search bar and SearchResultMarker,Bubble directly to show opened locations via --uri command line option, substantially simplifying #735215 code.

Created attachment 285429 [details] [review]
placeEntry: Add support for RFC5870 URIs and plain coordinates

The URIs supported are the same as for GeocodeGlib:

The URI should be in the geo scheme (RFC 5870) which in its simplest
form looks like:
    geo:latitude,longitude

An Android extension to set a description is also supported in the form
of:
    geo:0,0?q=latitude,longitude(description)

Since we can have unnamed places with this approach, we need to support
these in PlaceEntry to show a proper text in the entries for these
cases like the plain coordinates. As plain coordinates are now valid
values for PlaceEntry we may interpret it as well.

------

I don't know if is a good idea to add URIs and plain coordinates
support in the same commit, but since we add the chance to have
unnamed places by adding URI support and we need a way to represent
these kind of places in PlaceEntry's, don't add support for plain
coordinates as valid input as well seemed inconsistent.

Created attachment 285430 [details] [review]
turnPointMarker: Remove custom name

Created attachment 285431 [details] [review]
searchResultMarker: Disable bubble for unnamed places

Review of attachment 285429 [details] [review]:

I can see a UX problem with this, let say that I have 2 coordinates and I want to copy into clipboard and then paste it to search the route between these two points, so, the most logic steps are:

- Copy first coordinate
- Paste in From entry
- Copy second coordinate
- Paste in To entry
- Press INTRO (I have focus in To entry)

Since I've never pressed INTRO when having focus in From entry, the _onActivate method was never executed, so the place property wasn't updated for this entry. So the result are an inconsistent search (From was filled before paste) or no search being executed (From wasn't filled before paste).

An easy way to fix it maybe is force string parsing on focus out (if we cannot parse as an URI or plain coordinate, do nothing as we are doing now), that maybe can look weird when interacting with main search entry (search triggered on focus out), we can add an option to PlaceEntry to avoid this or different classes for main search entry and route point search entry.

I also was thinking about update the place when user is writing, but right now the patch update the entry text on place change and it can be a problem if the user want to search for a place that is similar to a geographic coordinate.

Review of attachment 285429 [details] [review]:

Thanks! Looks fine.

But. Don't include the uri parsing in this. Make it a patch of it's own. Maybe it should go in at the same time we introduce --uri.
But at least make it a patch of it's own.

About the UX bit:
Could you cook up a follow-up patch that triggers the parsing on focus-out, and introduce a property that enables it?
So that we can test the behavior. You could also hook up to the paste-clipboard signal?

https://developer.gnome.org/gtk3/stable/GtkEntry.html#GtkEntry-paste-clipboard

And the property would be "parse-on-paste" ?

::: src/placeEntry.js
@@ +149,3 @@
+        } catch(e) {
+            Utils.debug('Geo URI parsing failed: ' + e);
+            return null;

This message should be user visiable I think. Maybe with Application.notificationManager.showMessage.

Review of attachment 285430 [details] [review]:

Looks fine.

Review of attachment 285431 [details] [review]:

So this makes so that a search-result with no name shows no bubble.

Andreas: Is there a nice way to show a bubble that is coordinates only, or is this better?

(In reply to comment #22)
> Review of attachment 285431 [details] [review]:
> 
> So this makes so that a search-result with no name shows no bubble.
> 
> Andreas: Is there a nice way to show a bubble that is coordinates only, or is
> this better?

I think no bubble sounds good. Just the pin.
You could kind of try and find the nearest address, but I feel it's a bit too error-prone.

Created attachment 285531 [details] [review]
placeEntry: Add support for plain coordinates

This adds support for parsing plain coordinates with the format
"<latitude>, <longitude>" in PlaceEntry class. When PlaceEntry
detects a valid plain coordinate (valid format, between valid ranges)
being introduced, it will skip the place search, also, no reverse
search is performed.

Plain coordinates places are internally represented by a GeocodePlace
object whose "name" property is null.

Created attachment 285532 [details] [review]
turnPointMarker: Remove custom name

Created attachment 285533 [details] [review]
searchResultMarker: Disable bubble for unnamed places

Created attachment 285534 [details] [review]
placeEntry: Add parseOnFocusOut option

This fix a usability issue related to plain coordinates support and
sidebar place entries, where user writes a plain coordinate in
the entry and jump to the next entry without pressing INTRO to
confirm the place.

Attachment 285531 [details] pushed as d6f283b - placeEntry: Add support for plain coordinates
Attachment 285532 [details] pushed as 0f1e4ac - turnPointMarker: Remove custom name
Attachment 285534 [details] pushed as b839089 - placeEntry: Add parseOnFocusOut option