Put state in Future instead of using Future<isInit> #65
Conversation
| if (tNull is T) { // T is nullable | ||
| final jsonStringOrNull = await _invokeMethod<String?>(methodName, data); | ||
| if (jsonStringOrNull == null) return tNull; | ||
| jsonString = jsonStringOrNull; | ||
| } else { | ||
| jsonString = await _invokeMethod<String>(methodName, data); | ||
| } |
There was a problem hiding this comment.
Unrelated to the async stuff. Made the function generic over nullness.
There was a problem hiding this comment.
I'm not super fond of that change. Kind of hard to understand and mentally decode IMO. Why do we need it in the first place? In which case was the old implementation not working as expeced?
|
|
||
| _completer.complete(true); |
There was a problem hiding this comment.
Completer is used to bridge a callback based API to Future. Given that we are chaining Futures, it is not needed.
| }, | ||
| ), | ||
| ) | ||
| .then((value) { |
There was a problem hiding this comment.
Migrated to async method rather than Future chaining.
| return EventChannel(name); | ||
| static EventChannel registerEventChannel({ | ||
| required String name, | ||
| void Function(dynamic event)? onEvent, |
There was a problem hiding this comment.
Align with registerMethodChannel: the callback is passed to the register* function rather than registered in a following call.
| throw Exception('Error initializing player on native platform side.'); | ||
| final String jsonString; | ||
| const T? tNull = null; | ||
| if (tNull is T) { // T is nullable |
There was a problem hiding this comment.
this if condition is always false, or do I misunderstand something here?
(you create tNull = null and in the next line you check for tNull is T 🤔
| /// Private method channel for this player instance | ||
| MethodChannel methodChannel; | ||
|
|
||
| /// Private method channel for this player instance to receive events |
There was a problem hiding this comment.
Nit:
| /// Private method channel for this player instance to receive events | |
| /// Private event channel for this player instance |
| if (value == null || value == false) { | ||
| _completer.complete(false); | ||
| return; | ||
| final success = await mainChannel.invokeMethod<bool>( |
There was a problem hiding this comment.
Starting from here, the indentation is off. My VSCode is auto-fixing this when I edit this file. Is your IDE maybe configured to indent using 4 spaces?
| if (tNull is T) { // T is nullable | ||
| final jsonStringOrNull = await _invokeMethod<String?>(methodName, data); | ||
| if (jsonStringOrNull == null) return tNull; | ||
| jsonString = jsonStringOrNull; | ||
| } else { | ||
| jsonString = await _invokeMethod<String>(methodName, data); | ||
| } |
There was a problem hiding this comment.
I'm not super fond of that change. Kind of hard to understand and mentally decode IMO. Why do we need it in the first place? In which case was the old implementation not working as expeced?
| static EventChannel registerEventChannel({ | ||
| required String name, | ||
| void Function(dynamic event)? onEvent, | ||
| }) { |
There was a problem hiding this comment.
Incorrect indentation. VSCode auto-fixed that when I touched the file
The native player was previously initialized and the communication channels set in a separate async context, with a
Future<bool>guarding when this was done. This is very similar to how it would be done in C with a condition variable.The safer, and more common in Dart, way is to embed the async result in the future itself. The channels can be safely accessed by
awaiting on the newFuture<NativePlayerHandle>.This PR depends on: #63