-
Notifications
You must be signed in to change notification settings - Fork 513
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
Workaround for disposal crash with flutter3 #1172
Workaround for disposal crash with flutter3 #1172
Conversation
Nice job! Can you only use the This avoids any risk for old versions! |
I don't think there's a way to get flutter version runtime, only dart. I suppose we could introduce a breaking change with a parameter and let the user decice, which kind of embedding they want? |
huh i didn’t know that you could not get the flutter version. So adding a parameter would be a good solution. We can start by defaulting it to the current embedding Adding some lines to the README to indicate that this should be set someone wants to use flutter 3.x, would be great. |
I'll add the parameter, the corresponding warning and fix the failed checks. I can also update the README and you can later polish it up. |
I added two parameters. One for delayed disposal and one for overriding hybrid composition. Both act per instance of map. Side note: In the project I'm working on we've come to the conclusion that hybrid mode is better (faster and without crashes) for Android 9 devices since we manufacture one - that contradicts observations that were commented in this repo. Hence I added an override parameter, so that you can have a mixed mode and decide whatever you want - run time. Aside from this there's not much I can do - I tested your proposed solution and commented in #1119. If you have any more suggestions, please let me know. Otherwise please review this and consider merging. |
@srmanc I try to add this branch into my project by using this
but I got the error
You have any idea? |
@samderlust Didn't try it out, but AFAIK you have to make dependency overrides for mapbox_gl_platform_interface and mapbox_gl_web as well, not just mapbox_gl. |
@srmanc I figure so, let me try that |
it is compiled with the following. just put it here in case anyone needs it
|
@samderlust please note that Tobrun's repository is deprecated, https://github.com/flutter-mapbox-gl/maps.git is the official fork we all should be using. If you want to use this PR, I suggest to use the same commit for all three dependencies, since they all come from the same repo. Like this:
|
@srmanc Thanks for correcting. |
I'm having an issue while setting up my project to use this PR. I tried setting my pubspec.yaml up just like @srmanc stated. Though I'm getting an error while compiling:
This is my pubspec.yaml:
Could you provide me with some feedback what I'm doing wrong? |
@dawid-niedzwiecki This PR works with the latest stable flutter, 3.3.0. Which version of flutter are you using? |
It works <3 Had to use flutter 3.3.0 and set my MapboxMap properties like this:
|
Unfortunately I'm still having the same problem [✓] Flutter (Channel stable, 3.3.3, on macOS 12.0.1 21A559 darwin-x64, locale en-TR)
|
@GULERTOLGA Did you use useDelayedDisposal: true? What steps did you use exactly to reproduce this issue? |
Yes I have set useDelayedDisposal=true. There seems to be no definite reproducing steps. I often encounter this error when I go back to the previous screen after pan and zoom operations on the map. my pubspec
|
This PR seems to fix the issue for some users while some still experience the same issue but haven't responded all inquiries. Do you @felix-ht think we could merge this since it's opt-in anyway or do you think we could somehow improve this further? |
@srmanc my only concern is that this might indicate to the users that it works while it really for lots of users right now. If you can extend the message to indicate that this is not ready for production and has some residual issues - im fine with merging it. your take @AAverin ? |
I am personally reluctant to merge. Unfortunately I didn't yet have time to look deeper into the issue myself. Too much work. |
@AAverin i do agree that this isn't perfect - but the thing is that with the addition into the readme is certainly better than the current state of master. It has a clear indication what to expect with flutter 3.X and makes it easier for users to start testing, potentially giving us more awareness for the flutter and upstream mapbox issues. |
@srmanc can you fix Flutter CI / Static code analysis (pull_request) |
I can see all the valid points so I am fine with merging. Although, as this is not a fix, I think a better way would be to keep this workaround as a separate branch and add to main readme instructions on how people can use it via |
@felix-ht Locally for me it works, I'm using flutter 3.3.3. Linter says it's missing Uint8List which is in dart:typed_data, but my local linter says it's also flutter/services package. Which flutter does CI use? @AAverin I agree and I see your point, it's not a fix. One argument though for why we benefit from this - flutter web has a lot of improvements and bug fixes after 3.x and there are users, who use this package cross-platform completely - myself included. |
The Ci is on 2.10 - (which is still the main version used for the package) |
@AAverin i'am not in favor the extra branch, as this will cause additional maintenance effort to keep this branch in sync with master. And we are not talking about a release but just the current master. |
@felix-ht Linter errors and formatting fixed. |
@srmanc i decided to revert this merge commit as it causes issues with old flutter versions - apparently the interface of TextureAndroidViewController changed - this causes to no longer build for flutter 2.X Might be quite tricky to get this working for 2.x and 3.x |
This PR addresses native crashes after Mapbox disposal, since flutter3 seems to dispose resources faster than its 2.X.X predecessors.
It is an ugly workaround and by no means a fix, so be advised to use it cautiously.
It works for both hybrid and virtual device mode.