-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[video_player] Migrate video player to null safety #3165
Conversation
| /// The total duration of the video. | ||
| /// | ||
| /// Is null when [initialized] is false. | ||
| /// The duration is zero if the video hasn't been initialized. |
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.
nits:
| /// The duration is zero if the video hasn't been initialized. | |
| /// The duration is [Duration.zero] if the video hasn't been initialized. |
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.
Done.
|
|
||
| /// Indicates whether or not the video has been loaded and is ready to play. | ||
| bool get initialized => duration != null; | ||
| final bool isInitialized; |
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 did we need to make this name change? Feels like we can make it as non-breaking as possible?
Although I do agree that isInitialized is a better name. So your call.
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.
Ack.
| } | ||
|
|
||
| @override | ||
| // ignore: must_call_super |
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 remove super?
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 bad merge. Fixed.
| } | ||
| _isDisposed = true; | ||
| super.dispose(); | ||
| _lifeCycleObserver.dispose(); |
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.
Looks like the logic here is changed, is it related to null-safety migration?
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.
Fixed.
| return; | ||
| } | ||
| _updatePosition(newPosition); | ||
| _updatePosition(newPosition!); |
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.
can newPosition here be null?
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.
No, but I changed the code, so it's more readable. The Analyzer can figure out that newPosition! can safely be newPosition if you have a null check earlier.
|
|
||
| void initialize() { | ||
| WidgetsBinding.instance.addObserver(this); | ||
| WidgetsBinding.instance!.addObserver(this); |
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.
Can WidgetsBinding.instance be null?
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.
no. runApp() calls WidgetsFlutterBinding.ensureInitialized().
| // ignore: undefined_prefixed_name | ||
| assetUrl = ui.webOnlyAssetManager.getAssetUrl(assetUrl); | ||
| ui.webOnlyAssetManager.getAssetUrl(assetUrl); | ||
| assetUrl = ''; |
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.
Not sure why we need this line, could you elaborate?
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.
my mistake. Fixed.
cyanglaz
left a comment
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.
lgtm
3454b4e to
50afefb
Compare
Migrates video player to null safety.
I made some API changes like the use of
Duration.zero/Size.zeroinstead ofnullfor defaults inVideoPlayerValueandCaption.