Conversation
- Adopt Pigeon message channel suffix instead of own custom proxy binary messenger
maios
force-pushed
the
MAPSFLT-213_drop-proxy-binary-messenger
branch
from
f2ff4b7 to
593a58a
Compare
maios
changed the title
evil159
left a comment
evil159
left a comment
There was a problem hiding this comment.
There some aspects to improve, mainly when it comes to multi-instance scenarios, but looking good overall.
| style = StyleManager(binaryMessenger: messenger); | ||
| _mapEvents = _MapEvents(binaryMessenger: messenger); | ||
| _snapshotterMessenger = | ||
| _SnapshotterMessenger(messageChannelSuffix: _suffix.toString()); |
There was a problem hiding this comment.
I noticed a pattern where binary messenger is specified explicitly, for consistency it should be specified here as well.
There was a problem hiding this comment.
I don't think it is necessary, as stated by the doc, so we can leave it as optional null
The [binaryMessenger] named argument is available for dependency injection. If it is left null, the default BinaryMessenger will be used which routes to the host platform.
There was a problem hiding this comment.
Not having it works for me as well, as long as it is consistent through the code base.
Currently, we still have binaryMessenger parameter specified in some places.
| _MapEvents({BinaryMessenger? binaryMessenger}) { | ||
| _channel = MethodChannel('com.mapbox.maps.flutter.map_events', | ||
| const StandardMethodCodec(), binaryMessenger); | ||
| _MapEvents({BinaryMessenger? binaryMessenger, String channelSuffix = ''}) { |
There was a problem hiding this comment.
Additionally binary messenger should be a required parameter as well, since in all other places binary messenger is specified explicitly, having it as a requirement will help with consistency.
| arguments: args, | ||
| channelSuffix: args["channelSuffix"] as? Int ?? 0, | ||
| registrar: registrar, | ||
| pluginVersion: pluginVersion, | ||
| eventTypes: eventTypes | ||
| pluginVersion: args["mapboxPluginVersion"] as? String ?? "", | ||
| eventTypes: args["eventTypes"] as? [Int] ?? [] |
There was a problem hiding this comment.
What will happen if these fallback values (0, "", []) are used?
There was a problem hiding this comment.
it's the same as before 😅 this one is only clean up the code a bit
pjleonard37
left a comment
pjleonard37
left a comment
There was a problem hiding this comment.
Looks good from my side. I think ready to go once we get @evil159 's approval
evil159
left a comment
evil159
left a comment
There was a problem hiding this comment.
@maios thank you for addressing the feedback. I think we are almost there, I have left a couple of nits(in my opinion the binary messenger parameter should be removed), and I noticed there is one method channel in _MapboxMapsPlatform lacking the channel suffix.
Otherwise, it's looking very good!
P.S. This is an internal change, but its impact is broad. Should we note it in the changelog?
| style = StyleManager(binaryMessenger: messenger); | ||
| _mapEvents = _MapEvents(binaryMessenger: messenger); | ||
| _snapshotterMessenger = | ||
| _SnapshotterMessenger(messageChannelSuffix: _suffix.toString()); |
There was a problem hiding this comment.
Not having it works for me as well, as long as it is consistent through the code base.
Currently, we still have binaryMessenger parameter specified in some places.
|
@maios -- I think the Should this be |
3 participants
What does this pull request do?
Drop
ProxyBinaryMessenger, instead, setup channel with a messageChannelSuffix which is now supported by PigeonWhat is the motivation and context behind this change?
Pull request checklist: