Add LineString and Polygon parsing #707
Conversation
| return type; | ||
| } | ||
| private String type; | ||
| private ArrayList<Object> coordinates; |
There was a problem hiding this comment.
I don't think there's a better way to parse properties that can have multiple types, unfortunately.
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
|
|
||
| public class GeojsonGeometryTest { |
There was a problem hiding this comment.
We could also add a test (and implementation) for validating that Polygons always have identical first and last points. I'm not sure that that makes sense however: Collect will have to dynamically detect a polygon by checking for a match anyway, so invalid polygons would just end up being represented as traces.
There was a problem hiding this comment.
Does Collect check that first and last match before dropping last? If not, it probably should.
There was a problem hiding this comment.
There's no implementation for showing shapes (it's a separate issue from showing traces/lines), but yes it'll need to do that.
There was a problem hiding this comment.
Meant the implementation that shows existing shapes (defaults or previously-selected) and it sounds like that's on your radar and you'll consolidate all that anyway.
| @@ -32,7 +32,7 @@ public class GeojsonGeometry { | |||
| public String getOdkCoordinates() throws IOException { | |||
| if (type.equals("Point")) { | |||
| return coordinates.get(1) + " " + coordinates.get(0) + " 0 0"; | |||
| } else if (type.equals("LineString")) { | |||
| } else if (type.equals("LineString") || type.equals("MultiPoint")) { | |||
There was a problem hiding this comment.
It occurs to me that MultiPoint is semantically distinct from LineString and we probably shouldn't support it (yet). It's intended to be displayed without the lines in between so we don't have a way of representing that in the ODK geo notation.
There was a problem hiding this comment.
It's intended to be displayed without the lines in between so we don't have a way of representing that in the ODK geo notation.
It was pretty unclear to me in the spec whether that was the case or not. I guess it's an array of points that can be empty or just one point so that implies no "line". Is the "0-dimensional Point and MultiPoint" comment what suggests that to you or is there another part that lays that out in a clearer way?
Either way, part of me feels like there wouldn't be a problem in showing it with a line now and enhancing it later? Maybe there are cases where the line would be problematic that I haven't thought about.
There was a problem hiding this comment.
Agreed the spec is not super clear but it was seeing the name again in context that made me realize the difference in intent. The Internet agrees: https://stackoverflow.com/questions/36685195/whats-the-difference-between-linestring-and-multipoint-in-geojson
wouldn't be a problem in showing it with a line now and enhancing it later
It just doesn't make sense. Let's say the feature is "nurse stations" and there's a multipoint geometry. Drawing it with lines would not be useful I think it's more helpful to users to let them know we don't support what they're trying to do. In that case, they should disaggregate into one point per station.
That said, if you really feel strongly about keeping it in, this is not the hill I want to die on!
There was a problem hiding this comment.
It just doesn't make sense. Let's say the feature is "nurse stations" and there's a multipoint geometry. Drawing it with lines would not be useful I think it's more helpful to users to let them know we don't support what they're trying to do. In that case, they should disaggregate into one point per station.
That makes a lot of sense. I'll remove the support here.
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
|
|
||
| public class GeojsonGeometryTest { |
There was a problem hiding this comment.
Does Collect check that first and last match before dropping last? If not, it probably should.
lognaturel
changed the title
Largely based on getodk/javarosa#707. The biggest divergence besides obvious language/API differences is the validation approach. And `@ts-check` was added to ensure the validation and types actually match the expected runtime values.
Largely based on getodk/javarosa#707. The biggest divergence besides obvious language/API differences is the validation approach. And `@ts-check` was added to ensure the validation and types actually match the expected runtime values.
Closes #706
What has been done to verify that this works as intended?
New tests.
Why is this the best possible solution? Were any other approaches considered?
Comments inline.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This is all additional, so not a lot of risk here, hopefully.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
getodk/docs#1541