-
-
Notifications
You must be signed in to change notification settings - Fork 659
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
Added a new feature to receive NMEA messages #605
base: master
Are you sure you want to change the base?
Conversation
@@ -1,3 +1,5 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
package="com.baseflow.geolocator"> | |||
|
|||
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" /> |
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.
This line should be removed. It will probably give a warning which can be suppressed in code.
Permissions are the responsibility of the developers using the plugin in their App and should not be something we determine for them.
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" /> |
@@ -21,11 +21,17 @@ | |||
|
|||
@Nullable private MethodCallHandlerImpl methodCallHandler; | |||
|
|||
@Nullable private StreamHandlerImpl streamHandler; | |||
@Nullable | |||
private PositionStreamImpl streamHandler; |
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 see you renamed the class StreamHandlerImpl
to PositionStreamImpl
, this name however suggests that the class is a stream dealing with positions. This is however not the case, the class implements all the logic necessary to create a stream but it is not the stream it self. So I suggest we rename it to PositionStreamHandlerImpl
.
Also it would be nice to rename the variable streamHandler
to positionStreamHandler
making it a bit more semantic and easier to read code for other developers.
private PositionStreamImpl streamHandler; | |
private PositionStreamHandlerImpl positionStreamHandler; |
|
||
@Nullable private Registrar pluginRegistrar; | ||
@Nullable | ||
private NmeaStreamImpl nmeaStream; |
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.
Same as the above PositionStreamImpl
. It would be better to rename this to:
private NmeaStreamImpl nmeaStream; | |
private NmeaStreamHandlerImpl nmeaStreamHandler; |
this.geolocationManager = geolocationManager; | ||
} | ||
|
||
public static Map<String, Object> toMap(String n, Long l) { |
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.
This method should be made private
.
It's intention is to convert the NMEA result into a format that the Flutter EventChannel
understands. Only the NmeaStreamHandlerImpl
(after you renamed the class) is allowed to send messages on the NMEA EventChannel, meaning other code should be protected from knowing how to map a NMEA object to a map.
public static Map<String, Object> toMap(String n, Long l) { | |
private static Map<String, Object> toMap(String n, Long l) { |
public void startNmeaUpdates(Context context, Activity activity, NmeaMessageaClient client, | ||
NmeaChangedCallback nmeaChangedCallback, ErrorCallback errorCallback) { | ||
|
||
handlePermissions( | ||
context, | ||
activity, | ||
() -> client.startNmeaUpdates(nmeaChangedCallback, errorCallback), | ||
errorCallback); | ||
} | ||
|
||
public void stopNmeaUpdates(NmeaMessageaClient client) { | ||
client.stopNmeaUpdates(); | ||
} | ||
|
||
public NmeaMessageaClient createNmeaClient(Context context) { | ||
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N ? new GnssNmeaMessageClient(context) | ||
: new GpsNmeaMessageClient(context); | ||
} |
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.
It would be better to create a whole new class (in it's own sub folder, together with the NmeaMessageaClient
, GnssNmeaMessageClient
and GpsNmeaMessageClient
classes) NmeaManager
and put this logic there. It is not really related to location information and currently bloats the GeolocationManager
class making it harder to understand what is going on in the GeolocationManager
class.
The current implementation violates the Single-responsibility principle which says (more info here):
A class should have one and only one reason to change, meaning that a class should have only one job.
Currently this class has two reasons two change:
- When the implementation of requesting location information changes;
- When the implementation of requesting NMEA information changes.
I think it would be good to move the handlePermissions
method into the PermissionManager
class (and make it public
). This way we don't have to duplicate this code in the GeolocationManager
and NmeaManager
class and we also don't need a reference from the NmeaManager
class to the GeolocationManager
class.
return _nmeaMessageStream; | ||
} | ||
|
||
var nmeaStream = nmeaChannel.receiveBroadcastStream(); |
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.
The nmeaStream
variable can be made final
, once it is initialized it does not change (and is should not change), by making it a final
variable we can protect against this.
var nmeaStream = nmeaChannel.receiveBroadcastStream(); | |
final nmeaStream = nmeaChannel.receiveBroadcastStream(); |
|
||
@immutable | ||
|
||
///nmea message |
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.
Comments should be written as sentences (see here and here for more details). You can also have a look at the Position
class for inspiration.
|
||
///nmea message | ||
class NmeaMessage { | ||
///nmea message |
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.
Comments should be written as sentences (see here and here for more details). You can also have a look at the Position
class for inspiration.
///nmea message | ||
NmeaMessage(this.message, this.timestamp); | ||
|
||
///message |
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.
Comments should be written as sentences (see here and here for more details). You can also have a look at the Position
class for inspiration.
///message | ||
final String message; | ||
|
||
///timestamp |
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.
Comments should be written as sentences (see here and here for more details). You can also have a look at the Position
class for inspiration.
geolocator/lib/geolocator.dart
Outdated
/// StreamSubscription<NmeaMessage> nmeaStream = getNmeaStream() | ||
/// .listen((NmeaMessage nmea) { | ||
/// // Handle NMEA changes | ||
/// }); |
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.
Code example should include static class:
/// StreamSubscription<NmeaMessage> nmeaStream = getNmeaStream() | |
/// .listen((NmeaMessage nmea) { | |
/// // Handle NMEA changes | |
/// }); | |
/// StreamSubscription<NmeaMessage> nmeaStream = Geolocator.getNmeaStream() | |
/// .listen((NmeaMessage nmea) { | |
/// // Handle NMEA changes | |
/// }); |
0dad5da
to
efe1501
Compare
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.
Thank you for updating your PR. I think the code now looks good but still have some minor requests that should be fixed.
StreamHandlerImpl streamHandler = new StreamHandlerImpl(geolocatorPlugin.geolocationManager); | ||
PositionStreamHandlerImpl streamHandler = new PositionStreamHandlerImpl(geolocatorPlugin.geolocationManager); | ||
streamHandler.startListening(registrar.context(), registrar.messenger()); | ||
streamHandler.setActivity(registrar.activity()); | ||
|
||
NmeaStreamHandlerImpl nmeaStream = new NmeaStreamHandlerImpl(geolocatorPlugin.nmeaMessageManager); | ||
nmeaStream.startListening(registrar.context(), registrar.messenger()); | ||
nmeaStream.setActivity(registrar.activity()); | ||
} | ||
|
||
@Override | ||
public void onAttachedToEngine(@NonNull FlutterPluginBinding flutterPluginBinding) { | ||
methodCallHandler = new MethodCallHandlerImpl(this.permissionManager, this.geolocationManager); | ||
methodCallHandler.startListening( | ||
flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger()); | ||
streamHandler = new StreamHandlerImpl(this.geolocationManager); | ||
streamHandler.startListening( | ||
positionStreamHandler = new PositionStreamHandlerImpl(this.geolocationManager); | ||
positionStreamHandler.startListening( | ||
flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger()); | ||
nmeaStreamHandler = new NmeaStreamHandlerImpl(this.nmeaMessageManager); | ||
nmeaStreamHandler.startListening( | ||
flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger()); | ||
} |
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 think it is time to simplify this block of code and do some more code sharing. I think we can replace the registerWith
, onAttachedToEngine
and onAttachedToActivity
with the following methods:
Improved registerWith
// This static function is optional and equivalent to onAttachedToEngine. It supports the old
// pre-Flutter-1.12 Android projects. You are encouraged to continue supporting
// plugin registration via this function while apps migrate to use the new Android APIs
// post-flutter-1.12 via https://flutter.dev/go/android-project-migration.
//
// It is encouraged to share logic between onAttachedToEngine and registerWith to keep
// them functionally equivalent. Only one of onAttachedToEngine or registerWith will be called
// depending on the user's project. onAttachedToEngine or registerWith must both be defined
// in the same class.
public static void registerWith(Registrar registrar) {
GeolocatorPlugin geolocatorPlugin = new GeolocatorPlugin();
geolocatorPlugin.pluginRegistrar = registrar;
geolocatorPlugin.configureListeners(registrar.context(), registrar.messenger());
geolocatorPlugin.setActivity(registrar.activity());
}
Improved onAttachedToEngine
@Override
public void onAttachedToEngine(@NonNull FlutterPluginBinding flutterPluginBinding) {
configureListeners(flutterPluginBinding.getApplicationContext(), flutterPluginBinding.getBinaryMessenger());
}
Improved onAttachedToActivity
@Override
public void onAttachedToActivity(@NonNull ActivityPluginBinding binding) {
this.pluginBinding = binding;
setActivity(binding.getActivity());
}
Improved onDetachedFromActivity
@Override
public void onDetachedFromActivity() {
setActivity(null);
}
New method configureListeners
void configureListeners(Context applicationContext, BinaryMessenger messenger) {
methodCallHandler =
new MethodCallHandlerImpl(permissionManager, geolocationManager);
methodCallHandler.startListening(applicationContext, messenger);
positionStreamHandler = new PositionStreamHandlerImpl(geolocationManager);
positionStreamHandler.startListening(applicationContext, messenger);
nmeaStreamHandler = new NmeaStreamHandlerImpl(nmeaMessageManager);
nmeaStreamHandler.startListening(applicationContext, messenger);
}
New method setActivity
void setActivity(@Nullable Activity activity) {
if (methodCallHandler != null) {
methodCallHandler.setActivity(activity);
}
if (positionStreamHandler != null) {
positionStreamHandler.setActivity(activity);
}
if (nmeaStreamHandler != null) {
nmeaStreamHandler.setActivity(activity);
}
if (activity != null) {
registerListeners();
} else {
deregisterListeners();
}
}
This will remove some duplicated code and also solves confusion with variable names.
|
||
class NmeaStreamHandlerImpl implements EventChannel.StreamHandler { | ||
|
||
private static final String TAG = "NmeaStreamImpl"; |
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.
Rename TAG
to match class name:
private static final String TAG = "NmeaStreamImpl"; | |
private static final String TAG = "NmeaStreamHandlerImpl"; |
this.nmeaMessageManager = nmeaMessageManager; | ||
} | ||
|
||
private static Map<String, Object> toMap(String n, Long l) { |
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.
Provide clear self-explanatory variable names instead of using single characters:
private static Map<String, Object> toMap(String n, Long l) { | |
private static Map<String, Object> toMap(String message, long timestamp) { |
return null; | ||
} | ||
|
||
Map<String, Object> Nmea = new HashMap<>(); |
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.
Local variables should not start with a capital:
Map<String, Object> Nmea = new HashMap<>(); | |
Map<String, Object> nmeaMap = new HashMap<>(); |
context, | ||
activity, | ||
this.nmeaMessageaClient, | ||
(String n, long l) -> events.success(toMap(n, l)), |
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.
Provide clear self-explanatory variable name instead of a single character:
(String n, long l) -> events.success(toMap(n, l)), | |
(String message, long timestamp) -> events.success(toMap(message, timestamp)), |
|
||
@SuppressLint("MissingPermission") | ||
@Override | ||
public void onProviderDisabled(String s) { |
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.
Provide self-explanatory variable names instead of single character variable names:
public void onProviderDisabled(String s) { | |
public void onProviderDisabled(String provider) { |
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N ? new GnssNmeaMessageClient(context) | ||
: new GpsNmeaMessageClient(context); |
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.
nit: formatting can be little bit improved:
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N ? new GnssNmeaMessageClient(context) | |
: new GpsNmeaMessageClient(context); | |
return android.os.Build.VERSION.SDK_INT >= VERSION_CODES.N | |
? new GnssNmeaMessageClient(context) | |
: new GpsNmeaMessageClient(context); |
geolocator/lib/geolocator.dart
Outdated
/// Returns a stream emitting NMEA-0183 sentences when they are received from | ||
/// the GNSS engine. With devices running a Android API level lower than 24 | ||
/// NMEA-0183 sentences are received from the GPS engine. |
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.
nit: the explanation is very clean but Dart documentation guidelines prescribe the first sentence to be short and compact. So maybe we can update this to:
/// Returns a stream emitting NMEA-0183 sentences when they are received from | |
/// the GNSS engine. With devices running a Android API level lower than 24 | |
/// NMEA-0183 sentences are received from the GPS engine. | |
/// Returns a stream emitting NMEA-0183 sentences (Android only, no-op on iOS). | |
/// | |
/// With devices running a Android API level lower than 24 NMEA-0183 sentences | |
/// are received from the GPS engine. From API level 24 and up the GNSS engine | |
/// is used. |
/// Returns a stream emitting NMEA-0183 sentences when they are received from | ||
/// the GNSS engine. With devices running a Android API level lower than 24 | ||
/// NMEA-0183 sentences are received from the GPS engine. |
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.
nit: the explanation is very clean but Dart documentation guidelines prescribe the first sentence to be short and compact. So maybe we can update this to:
/// Returns a stream emitting NMEA-0183 sentences when they are received from | |
/// the GNSS engine. With devices running a Android API level lower than 24 | |
/// NMEA-0183 sentences are received from the GPS engine. | |
/// Returns a stream emitting NMEA-0183 sentences (Android only, no-op on iOS). | |
/// | |
/// With devices running a Android API level lower than 24 NMEA-0183 sentences | |
/// are received from the GPS engine. From API level 24 and up the GNSS engine | |
/// is used. |
EventChannel eventChannel = | ||
EventChannel('flutter.baseflow.com/geolocator_updates'); | ||
|
||
/// The event channel used to receive [NmeaMessage] updates from the native | ||
/// platform. | ||
@visibleForTesting | ||
EventChannel nmeaChannel = EventChannel('flutter.baseflow.com/nmea_updates'); |
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.
Similar to the implementation on the JAVA side, I think here we should also use clear naming to differentiate between the event channel broadcasting positions and the event channel broadcasting NMEA messages:
EventChannel eventChannel = | |
EventChannel('flutter.baseflow.com/geolocator_updates'); | |
/// The event channel used to receive [NmeaMessage] updates from the native | |
/// platform. | |
@visibleForTesting | |
EventChannel nmeaChannel = EventChannel('flutter.baseflow.com/nmea_updates'); | |
EventChannel positionEventChannel = | |
EventChannel('flutter.baseflow.com/geolocator_updates'); | |
/// The event channel used to receive [NmeaMessage] updates from the native | |
/// platform. | |
@visibleForTesting | |
EventChannel nmeaEventChannel = EventChannel('flutter.baseflow.com/nmea_updates'); |
…d under Marshmellow
…functionality to example app
…functionality to example app
…functionality to example app
…d tests to increase code coverage to 100%.
efe1501
to
6c5ca1a
Compare
Fix: issue #610 - requestPermission does not returning for Android 5.1 and below.
…functionality to example app
…functionality to example app
…functionality to example app
…d tests to increase code coverage to 100%.
version bump
…to issue#516 � Conflicts: � geolocator/CHANGELOG.md � geolocator/README.md � geolocator/example/lib/main.dart � geolocator/pubspec.yaml � geolocator_platform_interface/CHANGELOG.md
Any news about this feature please? is it ready to be used in Flutter? |
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
This PR introduces a new feature.
Currently you can not receive NMEA messages.
🆕 What is the new behavior (if this is a feature change)?
With this PR, you are able to recieve NMEA mesages
Example
This stream will continually give NMEA updates
💥 Does this PR introduce a breaking change?
No
🐛 Recommendations for testing
Use the example app ;)
📝 Links to relevant issues/docs
fixes #561
🤔 Checklist before submitting