Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Geo URI to Share panel, take 2. #994

Closed
wants to merge 3 commits into from
Closed

Introduce Geo URI to Share panel, take 2. #994

wants to merge 3 commits into from

Conversation

erictheise
Copy link
Contributor

A second attempt at addressing #799. Abandons the previous idea of adding a geo: button in the Link or HTML section of the Share panel. Adds a small block between Link or HTML and Image,

Makes #884 obsolete.

@HolgerJeromin
Copy link
Contributor

Do you have a screenshot? From reading your code it should be nice and usable.

@HolgerJeromin
Copy link
Contributor

Osmand (and probably others) adds ?z=20 to the url. Seems to be non standard and ignored by Vespucci. But perhaps worth to add.

@simonpoole
Copy link
Contributor

@HolgerJeromin I applied the PR as a patch here, there is an issue with the original string not being found, but I suspect that is just a minor glitch: glitch fixed, operator error on my behalf.

geo2

@HolgerJeromin
Copy link
Contributor

I like the order, so even on small screens this is probably usable (ref #849)

@erictheise
Copy link
Contributor Author

Thanks, @simonpoole, for sharing a screenshot. Here's how the panel looks on an iPhone.

image

@eighthave
Copy link

adding z=20 to a geo: URI is definitely part of the standard rfc5870. A geo: URI includes a full query string, I don't think there are any restrictions on the key/values allowed in a geo: URI's query string (except for the standard URI restrictions, of course). But it does look like the standard separator for the query string has recently become ; rather than ?, according to the RFC. The older draft RFC shows using ? . Android's geo: support is based on the draft RFC: https://developer.android.com/guide/appendix/app-intents.html

FYI, I've been doing work on supporting geo: URIs in lots of Android apps, including Osmand, K-9, ChatSecure, etc.

@simonpoole
Copy link
Contributor

OT but just so that there is no misunderstanding: I ignore the z parameter on purpose in Vespucci. Its a safeguard against trying to download the world (which isn't funny on a mobile device :-)). Naturally that wouldn't happen in any case, but if you are in a high density area you are likely better off setting the default download area to something reasonable than to rely on every user to zoom in first.

@erictheise
Copy link
Contributor Author

@eighthave, @HolgerJeromin, is there any reason not to send along the browser's current zoom level in the query string?

@simonpoole
Copy link
Contributor

@erictheise no reason I can think of, that vespucci ignores it is really very very specific.

@eighthave
Copy link

I think as much info should be included in the query string as possible. Apps are supposed to ignore key/values in the query string that they do not understand, according to the RFC:

@erictheise
Copy link
Contributor Author

Added current zoom to the query string.

@tomhughes
Copy link
Member

So the standard says that a query string is valid, but it doesn't actually define any specific parameters? So z= being zoom just becomes an OSM specific standard of some kind by this logic?

@erictheise
Copy link
Contributor Author

It's described in that Google Android Reference of Available Intents and iOS honors it as well.

@eighthave
Copy link

z=20 does not seem to be in an RFC but basically all of the major map services respect it:

@erictheise
Copy link
Contributor Author

@tomhughes, the people who requested this feature seem okay with the implementation and the ?z= is certainly more consistent across the services @eighthave listed than the keys for lat/lon. I need to return the Android tablet I borrowed in order to evaluate my work and am in the dark about what I'd need to do in order for you to accept this pull request.

@tomhughes
Copy link
Member

Oh I sure it's fine technically, or if not is easily fixable, so I wouldn't worry about whether you have access to a tablet.

The only reason is I haven't looked at this is that I don't like the UI very much (having something labelled "Geo URI" is very geeky and will be meaningless to most people) but I am equally struggling to come up with any better ideas...

@t-8ch
Copy link

t-8ch commented Jun 25, 2015

Maybe "Coordinates link"?

@HolgerJeromin
Copy link
Contributor

Perhaps we could expand the topic to "coordinates" and add multiple formats for copy and paste like

50.77172 6.07763
N 50° 46.303 E 006° 04.658
N 50° 46' 18.192" E 6° 4' 39.468"

Is this useful or overkill? I don't want to block merging this PR.

@erictheise
Copy link
Contributor Author

A geeky label for a somewhat geeky feature. The intent of the link is Launch External Mapping App, but that takes up too much screen space and will not even be true for, say, a Windows desktop user. The geo uri is not new, although it was new to me, and explicitly labeling it as such may help popularize it. That seems within the mission of openstreetmap-website.

@tomhughes
Copy link
Member

I've merged this as is in 6403cb9. We can always change things later if we come up with better ideas of how to present it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants