Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

MapSnapshot attribution #10362

Merged
merged 7 commits into from
Nov 14, 2017
Merged

MapSnapshot attribution #10362

merged 7 commits into from
Nov 14, 2017

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Nov 2, 2017 •
edited
Loading

ezgif com-video-to-gif

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Nov 2, 2017
@tobrun tobrun added this to the android-v5.2.0 milestone Nov 2, 2017
@tobrun tobrun self-assigned this Nov 2, 2017
@tobrun tobrun added the release blocker Blocks the next final release label Nov 3, 2017
@tobrun tobrun force-pushed the tvn-mapsnapshot-attribution branch 4 times, most recently from 8c99bc1 to 5b686e5 Compare November 13, 2017 10:36
Guardiola31337
Guardiola31337 reviewed Nov 13, 2017
View reviewed changes
...id/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/AttributionMeasure.java Outdated
}

public AttributionLayout measure() {
float logoContainerWidth = logo.getWidth() + (2 * margin);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

measure() is kinda long. What about extracting each block of code into a private method?

...id/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/AttributionMeasure.java Outdated
boolean shortText = textViewShortContainerWidth + margin <= maxSizeShort;
shorterText = fullLogoShortText || smallLogoShortText || shortText;

if (fullLogoText) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about removing the if else if chain using a map/dictionary?

...oid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/AttributionParser.java Outdated
* @return if the url is valid for improve this map
*/
private boolean isValidForImproveThisMap(String url) {
return withImproveMap || !url.equals("https://www.mapbox.com/map-feedback/");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number

...oid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/AttributionParser.java Outdated
* @return if the url is valid for Mapbox
*/
private boolean isValidForMapbox(String url) {
return withMapboxAttribution || !url.equals("https://www.mapbox.com/about/maps/");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number

...oid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/AttributionParser.java Outdated
*/
private void addAdditionalAttributions() {
if (withTelemetryAttribution) {
String telemetryKey = "Telemetry Settings";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic numbers

...ndroid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/snapshotter/MapSnapshotter.java Outdated

AttributionLayout layout = measure.measure();

// draw logo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addOverlay is getting long.
What about extracting these blocks of code into private methods and give them a name based on the comments? This way addOverlay will be easier to read and understand and comments will become superfluous so they won't be necessary.

...estApp/src/androidTest/java/com/mapbox/mapboxsdk/maps/attribution/AttributionLayoutTest.java Outdated
}

@Test
@Ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the tests are ignored, could we remove AttributionLayoutTest?

.../MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/attribution/AttributionParseTest.java
for (Attribution attribution : attributionList) {
switch (counter) {
case 0:
assertEquals("URL mapbox should match", "https://www.mapbox.com/about/maps/", attribution.getUrl());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about extracting the different assertions into different tests and give them a name based on the messages?
This way I guess that the for loop won't be necessary and tests will be more maintainable and readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have bandwith to pick this up. feel free to ticket out any follow up work.

@Guardiola31337 Guardiola31337 merged commit f0f113b into release-agua Nov 14, 2017
@Guardiola31337 Guardiola31337 deleted the tvn-mapsnapshot-attribution branch November 14, 2017 10:13
This was referenced Nov 14, 2017
Closed
Closed
mappy-mobile pushed a commit to Mappy/mapbox-gl-native that referenced this pull request Nov 29, 2017
Merge branch 'release-android-v5.2.0' into mappy_mergre_mapbox_5.2
52978a8 
* release-android-v5.2.0: (788 commits)
  release android v5.2.0
  release v5.2.0-beta.5 (mapbox#10464)
  [ios] Update puck arrow stroke color when tint changes
  Monkey crashes (mapbox#10440) (mapbox#10472)
  Deploy macosapp as part of releases (mapbox#10191)
  [ios] Update podspecs to v3.7.0-rc.1
  [ios] Updated Spanish, Vietnamese translations
  [ios] Fix toCamera.centerCoordinate in shouldChangeFromCamera  (mapbox#10433)
  MapSnapshot attribution (mapbox#10362)
  Downgrade min sdk to 14 (mapbox#10355)
  Update MGLMapSnapshotter docs (mapbox#10438)
  [android] - harden deselection mechanism for markers (mapbox#10403)
  [android] Cherry picks to agua (mapbox#10442)
  [ios] Silence smart invert warnings (mapbox#10425)
  [ios] Doc fixes for "Adding Points to a Map" guide
  [ios, macos] Cleanup duplicated snapshotter frame code.
  [android] release 5.2.0-beta.4 (mapbox#10384)
  [android] - add config file for excluding generated tests, refactor generation script output
  [ios] Bump podspec to beta.4
  [ios, macos] Add attribution to snapshots.
  ...

# Conflicts:
#	.gitignore
#	cmake/core-files.cmake
#	include/mbgl/annotation/annotation.hpp
#	platform/android/CHANGELOG.md
#	platform/android/MapboxGLAndroidSDK/build.gradle
#	platform/android/MapboxGLAndroidSDK/gradle.properties
#	platform/android/MapboxGLAndroidSDK/src/main/AndroidManifest.xml
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationSource.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AttributionDialogManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapGestureDetector.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/camera/LatLngBoundsActivity.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/SimpleMapActivity.java
#	platform/android/build.gradle
#	platform/android/config.cmake
#	platform/android/dependencies.gradle
#	platform/android/settings.gradle
#	platform/android/src/map/camera_position.cpp
#	platform/android/src/native_map_view.cpp
#	platform/android/src/native_map_view.hpp
#	src/mbgl/algorithm/generate_clip_ids_impl.hpp
#	src/mbgl/annotation/line_annotation_impl.cpp
#	src/mbgl/renderer/layers/render_line_layer.hpp
#	src/mbgl/renderer/painter.cpp
#	src/mbgl/renderer/painter_line.cpp
#	src/mbgl/renderer/render_item.hpp
#	src/mbgl/renderer/render_line_layer.cpp
#	src/mbgl/sprite/sprite_atlas.cpp
#	src/mbgl/sprite/sprite_atlas.hpp
#	src/mbgl/sprite/sprite_parser.cpp
#	src/mbgl/sprite/sprite_parser.hpp
#	src/mbgl/storage/resource.cpp
#	src/mbgl/style/layers/line_layer.cpp
#	src/mbgl/style/layers/line_layer_impl.hpp
#	src/mbgl/style/style.cpp
#	src/mbgl/style/style.hpp
#	src/mbgl/tile/tile_loader_impl.hpp
#	test/algorithm/generate_clip_ids.test.cpp
#	test/sprite/sprite_atlas.test.cpp
#	test/storage/resource.test.cpp
mappy-mobile pushed a commit to Mappy/mapbox-gl-native that referenced this pull request Dec 12, 2017
Merge branch 'mapbox_release_5.2' into mappy_merge_5.2
537fb3d 
* mapbox_release_5.2: (788 commits)
  release android v5.2.0
  release v5.2.0-beta.5 (mapbox#10464)
  [ios] Update puck arrow stroke color when tint changes
  Monkey crashes (mapbox#10440) (mapbox#10472)
  Deploy macosapp as part of releases (mapbox#10191)
  [ios] Update podspecs to v3.7.0-rc.1
  [ios] Updated Spanish, Vietnamese translations
  [ios] Fix toCamera.centerCoordinate in shouldChangeFromCamera  (mapbox#10433)
  MapSnapshot attribution (mapbox#10362)
  Downgrade min sdk to 14 (mapbox#10355)
  Update MGLMapSnapshotter docs (mapbox#10438)
  [android] - harden deselection mechanism for markers (mapbox#10403)
  [android] Cherry picks to agua (mapbox#10442)
  [ios] Silence smart invert warnings (mapbox#10425)
  [ios] Doc fixes for "Adding Points to a Map" guide
  [ios, macos] Cleanup duplicated snapshotter frame code.
  [android] release 5.2.0-beta.4 (mapbox#10384)
  [android] - add config file for excluding generated tests, refactor generation script output
  [ios] Bump podspec to beta.4
  [ios, macos] Add attribution to snapshots.
  ...

# Conflicts:
#	.gitignore
#	.gitmodules
#	cmake/core-files.cmake
#	include/mbgl/annotation/annotation.hpp
#	platform/android/CHANGELOG.md
#	platform/android/MapboxGLAndroidSDK/build.gradle
#	platform/android/MapboxGLAndroidSDK/gradle.properties
#	platform/android/MapboxGLAndroidSDK/src/main/AndroidManifest.xml
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationSource.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AnnotationManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AttributionDialogManager.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapGestureDetector.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMap.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/NativeMapView.java
#	platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Transform.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/camera/LatLngBoundsActivity.java
#	platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/SimpleMapActivity.java
#	platform/android/build.gradle
#	platform/android/config.cmake
#	platform/android/dependencies.gradle
#	platform/android/settings.gradle
#	platform/android/src/map/camera_position.cpp
#	platform/android/src/native_map_view.cpp
#	platform/android/src/native_map_view.hpp
#	src/mbgl/algorithm/generate_clip_ids_impl.hpp
#	src/mbgl/annotation/line_annotation_impl.cpp
#	src/mbgl/renderer/layers/render_line_layer.hpp
#	src/mbgl/renderer/painter.cpp
#	src/mbgl/renderer/painter_line.cpp
#	src/mbgl/renderer/render_item.hpp
#	src/mbgl/renderer/render_line_layer.cpp
#	src/mbgl/sprite/sprite_atlas.cpp
#	src/mbgl/sprite/sprite_atlas.hpp
#	src/mbgl/sprite/sprite_parser.cpp
#	src/mbgl/sprite/sprite_parser.hpp
#	src/mbgl/storage/resource.cpp
#	src/mbgl/style/layers/line_layer.cpp
#	src/mbgl/style/layers/line_layer_impl.hpp
#	src/mbgl/style/style.cpp
#	src/mbgl/style/style.hpp
#	src/mbgl/tile/tile_loader_impl.hpp
#	test/algorithm/generate_clip_ids.test.cpp
#	test/sprite/sprite_atlas.test.cpp
#	test/storage/resource.test.cpp
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

@ivovandongen ivovandongen Awaiting requested review from ivovandongen

@osana osana Awaiting requested review from osana

Assignees

@tobrun tobrun

Labels
Android Mapbox Maps SDK for Android release blocker Blocks the next final release
Projects
None yet
Milestone
android-v5.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@tobrun @Guardiola31337