-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
Adds MSL Altitude Support #1029
Conversation
@mvanbeusekom please let me know if you are happy with this solution. It should have a smaller impact on all the other platforms compared to the current PR's that are up for NMEA. |
Codecov Report
@@ Coverage Diff @@
## master #1029 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 1 17 +16
Lines 30 245 +215
==========================================
+ Hits 30 245 +215
Continue to review full report at Codecov.
|
Hi @Wackymax, Thank you for the PR and taking an interest. The impact is indeed a lot smaller compared to the other implementations. However it will still expose a field in the Wouldn't it be better if we could add a separate Dart class (e.g. This is also more in line with the plans Google has for their plugins. I started updating the platform specific implementations inline with issue flutter/flutter#94224. Since I have some time available I can pick this up and see if I can get this implemented today and tomorrow. Please let me know how you feel? |
Sounds good. If you want to have a separate stream then #932 is probably a good candidate for you to work from. I think having the option of reporting altitude as MSL is still a good candidate case for this PR. What are your thoughts on keeping that in? |
eac1adc
to
2e1c643
Compare
@mvanbeusekom I've rebased on the latest master containing your changes. I've split things up a bit as well, still have the MSL option for altitude in Android settings and I've created a separate channel for NMEA messages but I am not sure how to go about using it with the approach you want to take. Do you want to take it from here? |
2e1c643
to
8cb59d7
Compare
@Wackymax, thanks. In general my idea would be to apply a similar architecture as done with the recent refactor for webview_flutter (see also issue flutter/flutter#93732). Where the idea is to duplicate the platform specific location API on the Dart side and create an additional cross-platform implementation that uses this API to implement the geolocator_platform_interface. This should enable users of the plugin to choose between using the cross-platform API (easy to use but limited) versus using the platform specific API (which is more work but removes the limitations). I understand if this is all a bit too much and I can definitely take over from here if you prefer. |
Closing this to bring in this changes with a new PR |
Yeah I would be very lost 😅. I think it best for you to take over. Seems like you have a much better idea of how to approach this.
… On 11 Apr 2022, at 16:43, Maurits van Beusekom ***@***.***> wrote:
@Wackymax, thanks. In general my idea would be to apply a similar architecture as done with the recent refactor for webview_flutter (see also issue flutter/flutter#93732). Where the idea is to duplicate the platform specific location API on the Dart side and create an additional cross-platform implementation that uses this API to implement the geolocator_platform_interface.
This should enable users of the plugin to choose between using the cross-platform API (easy to use but limited) versus using the platform specific API (which is more work but removes the limitations).
I understand if this is all a bit too much and I can definitely take over from here if you prefer.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Feature: Adds NMEA message support to position updates
Feature: Adds MSL altitude support
Altitude is reported as height above the ellipsoid but iOS reports altitude as height above the geoid. It's also not possible to get NMEA messages in the current version.
🆕 What is the new behavior (if this is a feature change)?
💥 Does this PR introduce a breaking change?
No
🐛 Recommendations for testing
📝 Links to relevant issues/docs
Coses #561, closes #605, closes #819, closes #932
Should help with: #987
🤔 Checklist before submitting
master
.