Skip to content

fix(KmlLayer): Update constructor to take in a Context instead #631

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

Merged
merged 6 commits into from
Feb 22, 2020

Conversation

arriolac
Copy link
Contributor

Updating KmlLayer, KmlRenderer, and Renderer, to accept a Context instead of a FragmentActivity. The former signature and solution presents a couple of issues mentioned in #629.

This change does break configuration change handling for caching images in a way that
retaining state across these changes is now handled by the caller (i.e. by passing in
an ImagesCache instance via KmlLayer constructor).

  • Make sure to open a GitHub issue as a bug/feature request before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #629 🦕

@arriolac arriolac requested a review from barbeau February 20, 2020 23:35
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 20, 2020
@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #631 into master will decrease coverage by <.01%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage      19%   18.99%   -0.01%     
==========================================
  Files          71       71              
  Lines        4056     4037      -19     
  Branches      610      607       -3     
==========================================
- Hits          771      767       -4     
+ Misses       3251     3238      -13     
+ Partials       34       32       -2
Impacted Files Coverage Δ
...ava/com/google/maps/android/data/kml/KmlLayer.java 0% <0%> (ø) ⬆️
.../com/google/maps/android/data/kml/KmlRenderer.java 2.15% <100%> (ø) ⬆️
...in/java/com/google/maps/android/data/Renderer.java 10.12% <40%> (-0.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f0c22...31a4b94. Read the comment docs.

Copy link
Collaborator

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

LGTM! Two comments in-line.

@@ -128,7 +128,7 @@ public void onFeatureClick(Feature feature) {
KmlLayer kmlPolygonLayer;
try {
// KML Polyline
kmlPolylineLayer = new KmlLayer(getMap(), R.raw.south_london_line_kml, this, markerManager, polygonManager, polylineManager, groundOverlayManager);
kmlPolylineLayer = new KmlLayer(getMap(), R.raw.south_london_line_kml, this, markerManager, polygonManager, polylineManager, groundOverlayManager, null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion - in this or KmlDemoActivity, include an example of passing in the cache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. I'll update KmlDemoActivity and reuse the RetainFragment + ImagesCache strategy there

@@ -127,29 +129,23 @@
* Creates a new Renderer object for KML features
*
* @param map map to place objects on
* @param activity activity needed to add info windows and retain bitmap cache fragment
* @param context the Context
* @param markerManager marker manager to create marker collection from
* @param polygonManager polygon manager to create polygon collection from
* @param polylineManager polyline manager to create polyline collection from
* @param groundOverlayManager ground overlay manager to create ground overlay collection from
Copy link
Collaborator

Choose a reason for hiding this comment

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

imagesCache should be added to Javadoc with a description

@arriolac arriolac merged commit 731f7e6 into master Feb 22, 2020
@arriolac arriolac deleted the chris/fix/629 branch February 22, 2020 00:08
@arriolac arriolac added this to the 1.0 milestone Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing in a Context instead of a FragmentActivity when constructing KmlLayer
3 participants