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 #13737: Voiceover does not repeat when user submits answer before voiceover has finished #14367
Conversation
Hi @kevintab95, @DubeySandeep -- could one of you please add the appropriate changelog label to this pull request? Thanks! |
Important context: In this PR I make a fundamental change to the stop audio functionality: instead of simply stopping the track, I mimic the behavior of the on audio track ended callback function and set the currentTrack, filename, and position in file to null and call clearSecondaryAudioTranslations. I think having Stop be similar to 'on end' makes sense but it also may make the distinction between the Stop and Cancel functions irrelevant. Currently, the cancel function calls the stop function and then sets currentTrackFilename and currentTrack to be null, so in the new implementation it never makes sense to call cancel. Please look to these changes: https://github.com/oppia/oppia/pull/3818/files to see the old implementation of the stop, clear, and on ended functions, which used to look a lot like what my new edits are (stop didn't actually Stop the file, it just set it to null). I am not sure why this change was made, and have made one of my commits just so you can see where Stop is called and why. It is my opinion that none of the current uses of Stop actually require it to behave differently than cancel so it might actually just be a good idea to get rid of cancel. |
Hi @murraytay, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
@kevintab95 Can you please review this PR as a primary reviewer? |
Hi @murraytay, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
@kevintab95 would you mind reviewing this PR? |
Hi @murraytay, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
@DubeySandeep please see my comments to your message! |
Hi @murraytay, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
…tion of stopping the track
PTAL @DubeySandeep |
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
|
||
this._currentTrack = null; | ||
this._currentTrackFilename = null; | ||
this._lastPauseOrSeekPos = 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.
Ok, so on further inspection it seems to just need the currentTrack.stop() function which I think makes sense without a comment. Let me know what you think!
|
||
this._currentTrack = null; | ||
this._currentTrackFilename = null; | ||
this._lastPauseOrSeekPos = 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.
done
Unassigning @DubeySandeep since they have already approved the PR. |
Hi @aks681, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks! |
Overview
Essential Checklist
Proof that changes are correct
No voiceover for secondary text (make sure first text does not repeat)
no_secondary_audio.mov
Voiceover for secondary text
secondary_audio.mov
PR Pointers