-
-
Notifications
You must be signed in to change notification settings - Fork 662
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
NMEA Updates #1490
NMEA Updates #1490
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Wackymax,
Thanks for yet another great PR, I like the update however I have one concern about the update of the com.google.android.gms:play-services-location
and androidx.core:core
dependencies.
It would be great to hear your thoughts about it.
implementation 'com.google.android.gms:play-services-location:21.2.0' | ||
implementation 'androidx.core:core:1.13.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit afraid updating these dependencies introduce a breaking change. I have done an upgrade to version 21.1.0 of the com.google.android.gms:play-services-location
which forced users to update AGP to version 8.0.0 and Kotlin to 1.9.0.
I have reverted the change as we didn't need any of the new functionality, if we do need it to support the MSL altitude we should probably update the version number to 5.0.0
and clearly mention in the CHANGELOG that users should update their Android Gradle Plugin to version 8.0.0 and Kotlin to version 1.9.0.
I believe we do need it to support the new MSL functionality. I also don't like to let versions fall behind to much for security concerns etc. Id rather update to a new major version then?On 16 May 2024, at 09:32, Maurits van Beusekom ***@***.***> wrote:
@mvanbeusekom commented on this pull request.
Hi @Wackymax,
Thanks again for this PR, I like the update however I have one concern about the update of the com.google.android.gms:play-services-location and androidx.core:core dependencies.
It would be great to hear your thoughts about it.
In geolocator_android/android/build.gradle:
+ implementation 'com.google.android.gms:play-services-location:21.2.0'
+ implementation 'androidx.core:core:1.13.0'
I am a bit afraid updating these dependencies introduce a breaking change. I have done an upgrade to version 21.1.0 of the com.google.android.gms:play-services-location which forced users to update AGP to version 8.0.0 and Kotlin to 1.9.0.
I have reverted the change as we didn't need any of the new functionality, if we do need it to support the MSL altitude we should probably update the version number to 5.0.0 and clearly mention in the CHANGELOG that users should update their Android Gradle Plugin to version 8.0.0 and Kotlin to version 1.9.0.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
That sounds great to me. Personally I also don't like to fall behind, so if you can update to a new major release and clearly mention that users need to update to AGP version 8.0.0 or higher in the CHANGELOG that would be great. One interesting thing I noticed, the example application is still using version 7.6.1 of Gradle (see gradle-wrapper.properties) and version 7.4.2 of the Android Gradle Plugin (see settings.gradle). So maybe Google fixed something between version 21.1.0 and 21.2.0 not forcing users to upgrade anymore. Here is the original issues people submitted when I did the update: #1441 and #1442 |
Yeah I also haven't updated in my personal app and haven't run into any problems running on this commit. Should I still update to a newer version in this case?On 16 May 2024, at 10:03, Maurits van Beusekom ***@***.***> wrote:
I believe we do need it to support the new MSL functionality. I also don't like to let versions fall behind to much for security concerns etc. Id rather update to a new major version then?
That sounds great to me. Personally I also don't like to fall behind, so if you can update to a new major release and clearly mention that users need to update to AGP version 8.0.0 or higher in the CHANGELOG that would be great.
One interesting thing I noticed, the example application is still using version 7.6.1 of Gradle (see gradle-wrapper.properties) and version 7.4.2 of the Android Gradle Plugin (see settings.gradle). So maybe Google fixed something between version 21.1.0 and 21.2.0 not forcing users to upgrade anymore.
Here is the original issue people submitted when I did the update: #1442
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Let me do a few tests, see if I can reproduce the original error with version 21.1.0 and if it still occurs when I update to 21.2.0. I will get back to you soon ;). Thanks for your patience. |
TL;DR: Updating to version 21.2.0 is safe. No need to do a major version update. Long(er) version: To ensure the update of the
dependencies:
geolocator_android:
path: /Users/maurits/sources/Baseflow/Internal/Flutter/geolocator/geolocator_android The Next I updated the
Next I updated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @Wackymax for another great contribution.
@mvanbeusekom Could this change be responsible for #1517 ? |
This update includes the following changes:
Fixes: #1473
Fixes: #1479
Pre-launch Checklist
pubspec.yaml
with an appropriate new version according to the [pub versioning philosophy], or this PR is does not need version changes.CHANGELOG.md
to add a description of the change.///
).main
.dart format .
and committed any changes.flutter analyze
and fixed any errors.