Skip to content
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

Fix the issue that the empty polyline cannot be updated #3046

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

grab-liyanjin
Copy link
Contributor

An example code to reproduce the issue is

Polyline line = mapLibreMap.addPolyline(new PolylineOptions());
// Anything to update the polyline will fail
line.setPoints(...);
line.setColor(...);

The reason is that PolylineContainer will ignore the empty polyline the first time.

@hactar
Copy link
Collaborator

hactar commented Nov 27, 2024

Hmm, I wonder why that check was there in the first place...

Did you test what happens if you add an empty polyline and don't add any points/colors etc later in the code?

@louwers
Copy link
Collaborator

louwers commented Nov 27, 2024

Thanks for the PR!!

We should probably make this behavior consistent across other types of containers. For example:

if (!polygon.getPoints().isEmpty()) {
long id = nativeMap != null ? nativeMap.addPolygon(polygon) : 0;
polygon.setId(id);
polygon.setMapLibreMap(maplibreMap);
annotations.put(id, polygon);
}

Also there is a AnnotationManagerTest.kt where a test can be added.

@grab-liyanjin
Copy link
Contributor Author

Hmm, I wonder why that check was there in the first place...

Did you test what happens if you add an empty polyline and don't add any points/colors etc later in the code?

This issue was found by our developer during map integration, nothing will happen with empty point, but cannot update the polyline later since it's not really added into the map

@grab-liyanjin
Copy link
Contributor Author

Thanks for the PR!!

We should probably make this behavior consistent across other types of containers. For example:

if (!polygon.getPoints().isEmpty()) {
long id = nativeMap != null ? nativeMap.addPolygon(polygon) : 0;
polygon.setId(id);
polygon.setMapLibreMap(maplibreMap);
annotations.put(id, polygon);
}

Also there is a AnnotationManagerTest.kt where a test can be added.

Hi louwers, I checked all the containers including PolygonContainer, PolylineContainer, MarkerContainer, AnnotationContainer, and ShapeAnnotationContainer, only PolygonContainer and PolylineContainer have this kind of logic, so made changes to these two classes and UT as well

@louwers louwers enabled auto-merge (squash) November 28, 2024 11:36
@louwers louwers merged commit f547907 into maplibre:main Nov 28, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants