-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix videoplayer status after pause&finish #15782
Fix videoplayer status after pause&finish #15782
Conversation
if (this._ignorePause) { | ||
this._ignorePause = false; | ||
return; | ||
} | ||
this._playing = false; |
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.
There is _ignorePause
. I don't quite understand its usage. But from its name, it seems to ignore stop. And if want to ignore stop, then should not change the status?
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.
As you can see, when call stop(), it will set _ignorePause
to ture, which is reasonable.
But then it call video.pause()
then trigger onPause
in web.
Q: Why? It should be call video.stop()
then trigger onStop
, like on other platform:
A: Because on web, we don't have such video.stop()
interface. So, we use pause()
instead of stop()
.
Based on the above, I think we should set _playing
to false to make sure, in this condition, the video status is not playing. (obviously, when trigger onPause
, _playing
status shouldn't be true).
Otherwise, we got wrong isplaying
value when click stop
in web.
But from its name, it seems to ignore stop. And if want to ignore stop, then should not change the status?
By the way, as far as i'm concerned, _ignorePause
is just to prevent to pause
repeatly when video already pause
or stop
and emit extra Event.PAUSE
. So it is safe to set _playing = false
in advance.
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.
How about add stopped
status on web? Then can just remove _ignorePause
. The stop
implementation may be like this
public stop (): void {
if (this._status == STOPPED) {
return;
}
this.pause();
this._status = STOPPED;
...
}
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.
And i think only _status variable is needed.
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.
How about add
stopped
status on web? Then can just remove_ignorePause
. Thestop
implementation may be like thispublic stop (): void { if (this._status == STOPPED) { return; } this.pause(); this._status = STOPPED; ... }
Right now , _ignorePause
is not only use on web but also on other platform. So I do not recommand change implementation in small version like 3.8.1; In fact, there are other implementations also not reasonable enough. Maybe we can do some refactory work all together later on.
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.
Sounds reasonable.
Re: # related PR: cocos/cocos-test-projects#824
Changelog
Continuous Integration
This pull request:
Compatibility Check
This pull request: