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

Google imagery can be used as background in ID #3623

Closed
dmgroom opened this issue Nov 27, 2016 · 7 comments
Closed

Google imagery can be used as background in ID #3623

dmgroom opened this issue Nov 27, 2016 · 7 comments
Labels
bug A bug - let's fix this!

Comments

@dmgroom
Copy link

dmgroom commented Nov 27, 2016

see changeset https://www.openstreetmap.org/changeset/43920440 which shows Google imagery being use as a custom background layer

@tmcw
Copy link
Contributor

tmcw commented Nov 27, 2016

Thanks for the report @dmgroom ! Would you be interested in contributing a fix? This would be a very straightforward one: here's the part of the iD code where we check for google.com, google.ru, and googleapis.com: the fix would be adding another like, just like the one for googleapis.com, below that line, with google.cn.

@bhousel bhousel added the bug A bug - let's fix this! label Nov 27, 2016
@pnorman
Copy link
Contributor

pnorman commented Nov 27, 2016

iD needs to be using the <blacklist> from capabilities

A short-term solution would be to manually use the same regular expression as the API currently returns, .*\.google(apis)?\..*/(vt|kh)[\?/].*([xyz]=.*){3}.*.

@bhousel
Copy link
Member

bhousel commented Nov 28, 2016

Hmm, @pnorman that <blacklist> regular expression seems to have an error

It seems dangerous to blindly apply a regular expression that we fetch from the capabilities endpoint. e.g. It could contain PCRE and might not work in JavaScript. I guess we could wrap it in a try/catch and fallback to a known good regexp pattern.

anyway:

>  re = new RegExp('.*\.google(apis)?\..*/(vt|kh)[\?/].*([xyz]=.*){3}.*')
<  /.*.google(apis)?..*\/(vt|kh)[?\/].*([xyz]=.*){3}.*/
>  re.test('google.com')
<  false

😕

@pnorman
Copy link
Contributor

pnorman commented Nov 28, 2016

Hmm, @pnorman that regular expression seems to have an error

It's assuming that / is a special character, which is wrong, unless you happen to be using sed and / as a delimiter. Switch it to @ or something in the drop-down and it works fine.

re.test('google.com')
< false

google.com isn't an imagery URL, so it shouldn't match. http://www.google.cn/maps/vt?lyrs=s@190&gl=com&x={x}&y={y}&z={z} does match.

$ echo 'http://www.google.cn/maps/vt?lyrs=s@190&gl=com&x={x}&y={y}&z={z}' > imagery_test
$ grep -E '.*\.google(apis)?\..*/(vt|kh)[\?/].*([xyz]=.*){3}.*' imagery_test
http://www.google.cn/maps/vt?lyrs=s@190&gl=com&x={x}&y={y}&z={z}

No PCRE here.

@bhousel
Copy link
Member

bhousel commented Dec 5, 2016

Closed with a bunch of weekend commits:
aa3c1c8 All sources have id now, so compare by id
a7ac44f Refactor imageryBlacklists so it can be called without triggering a GET
c353684 Add imagery blacklist tests to rendererBackground#baseLayerSource
b9888e4 Add imageryBlacklists function to get blacklists from OSM API (for #3623)

This is working pretty solidly now because we:

  1. try to use the current regex list from the imagery blacklist
  2. fallback to a good regex that we know works
  3. replace blacklisted custom imagery with rendererBackgroundSource.None()
  4. actually test when setting the source, not when clicking the button
  5. ^ which means we also test custom imagery passed in via the url hash (we didn't before)

@bhousel bhousel closed this as completed Dec 5, 2016
@SomeoneElseOSM
Copy link

SomeoneElseOSM commented Dec 13, 2016

Just checking the status of this, since https://www.openstreetmap.org/changeset/44369979 appears to be using google.cn imagery. Is it just awaiting rollout to the main site?

@bhousel
Copy link
Member

bhousel commented Dec 14, 2016

Is it just awaiting rollout to the main site?

Yes it's fixed now and will be included in the next release. I haven't set a date for this and iD is currently stable enough to release anytime, though it would be nice to get some recent pull requests merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug - let's fix this!
Projects
None yet
Development

No branches or pull requests

5 participants