Adding Static Maps API#413
Adding Static Maps API#413domesticmouse merged 21 commits intogooglemaps:masterfrom domesticmouse:master
Conversation
| private static final long serialVersionUID = 1L; | ||
|
|
||
| /** The image data from the Photos API call. */ | ||
| public final byte[] imageData; |
There was a problem hiding this comment.
I don't think this is really what you want, in that byte[] is still mutable. But I can't think of a better approach.
There was a problem hiding this comment.
This ImageResult is an output value object. If a client of this library alters the image data, it has no impact on the library.
| * brown, green, purple, yellow, blue, gray, orange, red, white}. | ||
| */ | ||
| public void color(String color) { | ||
| if (!colorPattern.matcher(color).matches()) { |
There was a problem hiding this comment.
is it worth enforcing this locally? We might allow more colors in future.
There was a problem hiding this comment.
Fair call, ripped out regex
| private String label; | ||
| private String customIconURL; | ||
| private CustomIconAnchor anchorPoint; | ||
| private List<String> locations = new ArrayList<>(); |
There was a problem hiding this comment.
private final, you don't ever change this ref
| private String color; | ||
| private String fillcolor; | ||
| private boolean geodesic; | ||
| private List<String> points = new ArrayList<>(); |
| * appear in its default thickness (5 pixels). | ||
| */ | ||
| public void weight(int weight) { | ||
| this.weight = weight; |
There was a problem hiding this comment.
should setting this to zero clear the Integer?
There was a problem hiding this comment.
Good idea. Implemented in toUrlValue()
| this.height = height; | ||
| } | ||
|
|
||
| /** Serialisation constructor. */ |
| import com.google.maps.model.Size; | ||
| import java.awt.image.BufferedImage; | ||
| import java.io.ByteArrayInputStream; | ||
| import javax.imageio.ImageIO; |
There was a problem hiding this comment.
Is this library only pulled in for testing? Do we have it for the regular lib?
There was a problem hiding this comment.
I only use it in the test library to make sure we can round trip an image. I'm not decoding the image in the main library to keep up throughput.
| protected A param(String key, int val) { | ||
| params.put(key, Integer.toString(val)); | ||
|
|
||
| @SuppressWarnings("unchecked") // safe by specification - A is the actual class of this instance |
There was a problem hiding this comment.
minor nit: could you replace these three returns with
private A getInstance() {
@SuppressWarnings("unchecked");
return (A) this;
}
// later
return this.getInstance();There was a problem hiding this comment.
I like it, done. Ish.
samthor
left a comment
samthor
left a comment
There was a problem hiding this comment.
Just let me know your thoughts on the 'set' vs 'add' problem you have now.
|
|
||
| protected A param(String key, String val) { | ||
| params.put(key, val); | ||
| if(params.get(key) == null) { |
| protected A param(String key, String val) { | ||
| params.put(key, val); | ||
| if(params.get(key) == null) { | ||
| params.put(key, new ArrayList<String>()); |
There was a problem hiding this comment.
This looks naïvely fine but I wonder if anyone is using this class to overwrite previous values being set via .param().
Would it be worth having an explicit 'add' flag somehow? I know you already have the three param methods, so a matrix of those would be awkward. Let me know.
There was a problem hiding this comment.
In fact, yes I was being naive. There was logic in optimise waypoints that implicitly relied on that underlying hashmap behavior of the parameters.
New implementation now enforces singleton parameters (as implicitly required by non Static Maps API surfaces), and just uses the multiple parameters for Static Maps paths and markers.
Now the tests pass...
| paramsFound++; | ||
| } | ||
| } | ||
| assertTrue(paramsFound == expectedValues.size()); |
There was a problem hiding this comment.
Good catch, fixed.
2 participants
Fixes #233.
Also contains random clean ups and fixes, e.g. #412