[android] - derived source attribution by tobrun · Pull Request #8630 · mapbox/mapbox-gl-native

tobrun · 2017-04-04T13:41:26Z

tobrun · 2017-04-04T15:38:31Z

It seems we need some additional logic here, currently attribution for digital globe when using a satellite map shows up under Improve this map while on iOS it's placed above.

1ec5 · 2017-04-04T18:12:21Z

It seems we need some additional logic here, currently attribution for digital globe when using a satellite map shows up under Improve this map while on iOS it's placed above.

“Improve This Map” should appear before DigitalGlobe. (This screenshot is outdated.)

1ec5 · 2017-04-04T18:32:03Z

+
+  private void parseAttribution(String attributionSource) {
+    if (!TextUtils.isEmpty(attributionSource)) {
+      SpannableStringBuilder htmlBuilder = (SpannableStringBuilder) Html.fromHtml(attributionSource);


Could you characterize the HTML parser being used here? fromHtml(String) doesn’t say anything about which tags it supports, what encoding it uses, whether it attempts to load external images, etc. fromHtml(String, int) mentions TagSoup, but I don’t know if it’s also the case with this older API.

They use TagSoup underneath. With current design there isn't any possibility of icon placement though the API support it with an ImageGetter https://developer.android.com/reference/android/text/Html.html#fromHtml(java.lang.String,%20android.text.Html.ImageGetter,%20android.text.Html.TagHandler.

No external icon support is perfectly fine. I just wanted to double-check because the use of HTML in attribution has been problematic for GL JS and mapbox.js, mostly concerning XSS risk.

1ec5 · 2017-04-04T18:36:46Z

+  private void rebuildAttributionMap() {
+    attributionMap = new LinkedHashMap<>();
+    for (Source source : mapboxMap.getSources()) {
+      parseAttribution(source.getAttribution());


This implementation doesn’t attempt to deduplicate copyright notices that appear redundantly in multiple sources. For example, the Mapbox Streets and Mapbox Satellite sources both credit OpenStreetMap, so “© OpenStreetMap” would probably appear twice in the dialog when viewing the Mapbox Satellite Streets style.

You may want to adapt the logic used on iOS and macOS or alternatively the GL JS implementation. Both implementations come with plenty of unit tests as well.

A map collection cannot contain duplicate keys so this solves the issues of duplicate entries. Is there a use-case that I'm a not supporting with not implementing // remove any entries that are substrings of another entry.?

That logic was originally implemented in GL JS in mapbox/mapbox-gl-js#2453. I guess it’s meant to avoid “© OpenStreetMap contributors © OpenStreetMap” or somesuch, not sure.

I think what you’ve got should be fine when it comes to the Mapbox default styles at least. None of the sources ultimately loaded by Mapbox Streets or Mapbox Satellite Streets have any links whose texts differ but overlap.

1ec5 · 2017-04-04T19:10:37Z

-    <array name="mapbox_attribution_links" formatted="false" translatable="false">
-        <item>https://www.mapbox.com/about/maps/</item>
-        <item>http://www.openstreetmap.org/about/</item>
-        <item>https://www.mapbox.com/map-feedback/#/%1$f/%2$f/%3$d</item>


The iOS, macOS, and JS implementations automatically append this hash to the feedback URL. That way, the Map Feedback tool starts out at the right location, which makes their feedback much more actionable. The feedback link has a class attribute of mapbox-improve-map.

That got lost, will readd

tobrun · 2017-04-11T09:31:54Z

Fixed up @1ec5 comment about improving this map, did a cleanup and is ready for review.

ivovandongen

👍

ivovandongen · 2017-04-11T13:09:07Z

Minor thing, but maybe invoke source.getAttribution() only once. Now it doesn't really do much, but if the implementation gets more complex it's a bit wasteful.

done in 527ffbe#diff-8a5300c6fbc51b88f2e971461adcb513R33. re-review that part of the code?

ivovandongen · 2017-04-11T13:12:24Z

Nit; some class level javadoc?

done in 527ffbe#diff-8a5300c6fbc51b88f2e971461adcb513R33

1ec5 · 2017-04-11T21:55:12Z

+ */
+class AttributionDialogManager implements View.OnClickListener, DialogInterface.OnClickListener {
+
+  private static final String MAP_FEEDBACK_URL = "https://www.mapbox.com/map-feedback";


We shouldn’t hard-code this URL, because sources other than Mapbox Streets are free to provide their own map feedback URLs. Look for <a> tags with class set to mapbox-improve-map.

I ticketed this out as tail work in #8730, since custom feedback links are admittedly quite an edge case.

Thank you for ticketing that out, will fix it separately from this PR.

tobrun added the Android Mapbox Maps SDK for Android label Apr 4, 2017

tobrun added this to the android-v5.1.0 milestone Apr 4, 2017

tobrun self-assigned this Apr 4, 2017

tobrun requested a review from ivovandongen April 4, 2017 13:41

tobrun mentioned this pull request Apr 4, 2017

Derive attribution from sources in current style #2723

Closed

tobrun force-pushed the 2723-derived-source-attribution branch from b4bbbb3 to 3688cf2 Compare April 4, 2017 14:01

1ec5 added the GL JS parity For feature parity with Mapbox GL JS label Apr 4, 2017

1ec5 reviewed Apr 4, 2017

View reviewed changes

tobrun force-pushed the 2723-derived-source-attribution branch from 3688cf2 to 9111e42 Compare April 11, 2017 09:28

ivovandongen approved these changes Apr 11, 2017

View reviewed changes

tobrun force-pushed the 2723-derived-source-attribution branch from 9111e42 to 527ffbe Compare April 11, 2017 15:22

1ec5 suggested changes Apr 11, 2017

View reviewed changes

1ec5 mentioned this pull request Apr 12, 2017

Append camera position to custom feedback links in attribution #8730

Closed

1ec5 approved these changes Apr 12, 2017

View reviewed changes

tobrun force-pushed the 2723-derived-source-attribution branch from 527ffbe to a70febf Compare April 13, 2017 12:09

[android] - derived source attribution

4e01377

tobrun force-pushed the 2723-derived-source-attribution branch from a70febf to 4e01377 Compare April 13, 2017 14:12

tobrun merged commit f045559 into master Apr 13, 2017

tobrun deleted the 2723-derived-source-attribution branch April 13, 2017 15:24

tobrun mentioned this pull request May 2, 2017

Release Android v5.1.0-beta1 #8856

Closed

12 tasks

This was referenced May 18, 2017

Release Android v5.1.0-beta2 #8975

Closed

Release Android v5.1.0-beta.3 #9114

Closed

tobrun mentioned this pull request Jun 9, 2017

Release Android v5.1.0-beta.4 #9231

Closed

12 tasks

tobrun mentioned this pull request Jun 21, 2017

Release Android v5.1.0-beta.5 #9315

Closed

11 tasks

tobrun mentioned this pull request Jun 30, 2017

Release Android v5.1.0 #9404

Closed

16 tasks

Conversation

tobrun commented Apr 4, 2017

Uh oh!

tobrun commented Apr 4, 2017

Uh oh!

1ec5 commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobrun Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobrun commented Apr 11, 2017

Uh oh!

ivovandongen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tobrun Apr 4, 2017 •

edited

Loading

1ec5 Apr 11, 2017 •

edited

Loading