[annotation] Update DraggableAnnotationController for multiple draggable managers (#863, #883)

[Ph0tonic]

This is a proposal for fixing #863 and #883.

They are kind of linked together and I foud the solution of the second one during my work so I added it in this PR.

I know that it's not perfect but at least is a simple solution which allow multiple manager to be draggable.

@LukasPaczos and @langsmith I would be happy to have your feedback. 👍

[LukasPaczos]

Hey @Ph0tonic, thanks for your contribution again!

Unfortunately, I don't think this is an approach that we'd like to take. Having multiple detectors running is going to get inefficient.

To resolve the issue, I'd rather explore the option of having one, public instance of the draggable controller into which we can inject multiple annotation managers, instead of each annotation manager creating its own draggable controller.

[Ph0tonic]

Ok, I see, I will try to turn draggable controller into a singleton such has to minimize the number of detectors.

[Ph0tonic]

@LukasPaczos Here is a completely new implementation which turns DraggableController into a singleton. Would this be closer to an approach you would take ?

The tests are failing because I'm not able to run make generate-annotation-code locally.

[Ph0tonic]

I had a look at the make generate-annotation-code and the underlying scripts and I think that some of the .ejs needs to be updated but as those are annotated with:

// This file is generated.

I let those like they currently are. Please let me know if I can do anything else @langsmith and @LukasPaczos. Thanks

[Ph0tonic]

The limitations resolved by this PR would really help me for one of my side project. If we find a viable solution, do you think we might be able to do a release for those next weeks ?
@LukasPaczos and @langsmith Thank you very much for your time.

[kikothemaster]

When can we expect this fix in production?

[Ph0tonic]

Any updates about this PR ? @LukasPaczos and @langsmith

[LukasPaczos]

Sorry for a delayed response, this looks great @Ph0tonic!

Would you also be able to add one test to the DraggableAnnotationControllerTest that runs multiple managers at once? When this and the last comment below is addressed I can rerun the code generation for you to make the CI pass.

[LukasPaczos]

An issue with this singleton approach might occur when the map is recreated, for example in a recycler view, or when transitioning between fragments/activites.

We might need to do something like this, to additionally ensure that we're using the correct instance:

Suggested change

	public static synchronized DraggableAnnotationController getInstance(MapView mapView, MapboxMap mapboxMap) {
	if(INSTANCE == null){
	INSTANCE = new DraggableAnnotationController(mapView, mapboxMap);
	}
	return INSTANCE;
	}
	public static synchronized DraggableAnnotationController getInstance(MapView mapView, MapboxMap mapboxMap) {
	if(INSTANCE == null || this.mapView != mapview || this.mapboxMap != mapboxMap){
	INSTANCE = new DraggableAnnotationController(mapView, mapboxMap);
	}
	return INSTANCE;
	}

The strong references to mapView and mapboxMap should be dropped when the last removeAnnotationManager is called and the instance is nullified so we shouldn't leak. It'd be great to test this scenario though.

[Ph0tonic]

You're right I did not think about that 👍. I updated the code to reflect what you said. Regarding the testing of the last part of your remark, I'm not sure how I can do it.

[LukasPaczos]

Amazing work here, thanks @Ph0tonic! Let's continue in #1147.

[Ph0tonic]

Amazing work here, thanks @Ph0tonic! Let's continue in #1147.

You are welcome