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.

Sign up for GitHub

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

Jump to bottom

Get rid of MapboxGlPlatform.getInstance #710

Merged
merged 1 commit into from
Oct 24, 2021
Merged

Get rid of MapboxGlPlatform.getInstance #710

merged 1 commit into from
Oct 24, 2021

Conversation

shroff
Copy link
Collaborator

@shroff shroff commented Oct 9, 2021

This was an unnecessary layer of indirection, potentially causing a memory leak by holding on to the platform instance.

@shroff shroff force-pushed the platform-getinstance branch from 5dc4e29 to db64074 Compare October 9, 2021 18:38
@shroff shroff had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 9, 2021 18:38 Failure
@shroff shroff had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 9, 2021 18:38 Failure
@felix-ht
Copy link
Collaborator

felix-ht commented Oct 15, 2021 •
edited
Loading

Might there be any reason to hold onto more than one platform instance?

Could this cause issues if multiple maps are shown at the same time?

@felix-ht
Copy link
Collaborator

felix-ht commented Oct 15, 2021 •
edited
Loading

@shroff i checked and and we need multiple MapboxGlPlatforms to support multiple simultaneous maps. For example it tracks the callbacks and those need to be different for different instances. So going with the dispose approach of @AAverin seems like the way to go!

Well i got it wrong as you still create a new instance of MapboxGlPlatform for each new MapboxMap. So this is fine! LGTM

@AAverin
Copy link
Contributor

AAverin commented Oct 15, 2021

PR looks good to me too, I just didn't yet have the time to test if it resolves the leaks completely.
You are welcome to merge, I can only do the testing end of next week.

@felix-ht
Copy link
Collaborator

@AAverin it would be great if you could check it out and post your results

@AAverin AAverin mentioned this pull request Oct 18, 2021
Open
@AAverin
Copy link
Contributor

AAverin commented Oct 21, 2021

Pointing to this branch directly, I can still see many instances of MapboxMap sitting in memory when there shouldn't be any. Probably the solution would be a combination of #706 and this.
I will try to merge this PR into #706 and clean it up

@AAverin AAverin mentioned this pull request Oct 21, 2021
Merged
@AAverin
Copy link
Contributor

AAverin commented Oct 21, 2021

I have merged this PR into #706 and added some more fixes for memory leaks on the top. If #706 is merged this PR can be closed.

@shroff shroff merged commit b28e8c6 into master Oct 24, 2021
@shroff shroff deleted the platform-getinstance branch October 24, 2021 17:09
@m0nac0 m0nac0 mentioned this pull request May 13, 2022
Merged
m0nac0 added a commit to maplibre/flutter-maplibre-gl that referenced this pull request May 13, 2022
@m0nac0 @shroff @felix-ht
Cherry-pick upstream#710, #852, #904 + small fixes (#69)
d23b229 
* 710 (Get rid of MapboxGlPlatform.getInstance) flutter-mapbox-gl/maps#710
* 852( Android embedding fixes - migrate to maven) flutter-mapbox-gl/maps#852 was also required to fix compile time errors
* a small fix for the platform dependency
* Update CI flutter version
* cherry-pick upstream#904 (fix android build issues)

Co-authored-by: shroff <502644+shroff@users.noreply.github.com>
Co-authored-by: Felix Horvat <24698503+felix-ht@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felix-ht felix-ht felix-ht approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shroff @felix-ht @AAverin