-
Notifications
You must be signed in to change notification settings - Fork 3
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
Align nullness with other SDKs #63
base: main
Are you sure you want to change the base?
Conversation
Methods that return nullable types were returning placeholder values. This was not aligned with our other mobile SDKs. This also allows simplifying the glue layer. PFL-69
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 like the changes 👍
await loadSourceConfig(Sources.sintel); | ||
await callPlayer((player) async { | ||
final subtitles = await player.availableSubtitles; | ||
subtitleToSelect = subtitles.last.id; | ||
subtitleToSelect = subtitles?.last.id; |
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.
do we also need the nullcheck on last
? (subtitles?.last?.id
)
or is this handled inside dart? 🤔
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.
Contrary to Kotlin, in a?.b.c
, .c
will only be called is a
is not null. It would be equivalent to a?(.b.c)
if such syntax was allowed.
Future<SubtitleTrack> get subtitle async => | ||
await _invokeObjectMethod<SubtitleTrack>( | ||
Future<SubtitleTrack?> get subtitle async => | ||
_invokeObjectMethod<SubtitleTrack>( |
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.
why was the await
removed?
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 don't believe it has any effect 🤔
What do you expect it to do?
/// `null` if there are no active [Source]. | ||
Future<List<SubtitleTrack>?> get availableSubtitles; |
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 was already aligned with the iOS and Android SDK. Both SDKs return an empty list. Why did you make the list nullable?
/// `null` if there is no currently selected [SubtitleTrack]. | ||
Future<SubtitleTrack?> get subtitle; |
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 was aligned with iOS where we return the off-item in case there is no subtitle selected. I see that on Android this returns null
in this case (which feels more right to me), so fine for me to align this with Android instead of iOS 👍 (no action required)
Problem (PFL-69)
Methods that return nullable types on our native SDK were returning
placeholder values. This was not aligned with our other mobile
SDKs.
Changes
Return null instead. This also allows simplifying the glue layer.
Checklist
CHANGELOG
entry if applicable