-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Fix some phones unable to play audio #2728
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
// 标签朗读 | ||
this.statusList[index] = AudioStatus.READY | ||
audioElement.pause() | ||
} | ||
} | ||
} | ||
getTextList(text: string, is_end: boolean) { |
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 are several issues and suggestions with the provided code:
-
Multiple Instances of Audio Elements: The line
<audio ref="audioPlayer" v-for="item in audioList" ...>
generates multiple audio elements on each render, which is inefficient and could lead to performance problems. -
Conditional Rendering: Ensure that only one instance of
div ref="audioCiontainer"
is used unless you need dynamic rendering based on some condition. -
Speech Synthesis vs HTML Audio Element: The logic is inconsistent between handling SpeechSynthesisUtterance and HTMLAudioElement instances. This leads to redundant checks and can be streamlined.
-
Error Handling: There's no error handling when creating a new
HTMLAudioElement
, which should usedocument.createElement('audio')
instead of assigning it directly to a ref using Vue.js syntax. -
State Management: The
statusList
management seems scattered around different methods without consistent usage guidelines. Consider consolidating state management for better readability and maintainability. -
Method Names and Signatures: Method names like
pause
and logic inside them might not work as expected because they don't handle certain states correctly (e.g.,PLAY_INT
and other enum values). -
Code Style: There are minor style improvements that could enhance readability and consistency throughout the file.
Here are cleaned-up version recommendations:
// Remove the repeated div element and single instance control
<div ref="audioPanel">
<!-- El button -->
</div>
<!-- Single container for all audio elements -->
<div ref="audioContainer"></div>
<script lang="ts">
import { defineComponent } from 'vue';
class AudioManage {
textList: string[] = [];
ttsType: 'TTS' | 'TAG_READING' = ''; // Add type annotation here
statusList: number[] = []; // Define statuses
async loadAudioItems() {
await createHtmlElements(this.textList.map(item =>
this.createAudioElement(item)
));
}
private createAudioElement(text: string): HTMLElement {
const audio = document.createElement('audio');
audio.controls = true;
audio.hidden = true;
const utterance = new SpeechSynthesisUtterance(text);
// Handle playback events if needed
return audio;
}
}
function createHtmlElements(elements: HTMLElement[]): Array<HTMLDivElement> {
const divContainer = document.createElement('div');
const parentEl = document.getElementById('parentElementId'); // Replace with actual ID
elements.forEach((element) => {
divContainer.appendChild(element);
});
parentEl.appendChild(divContainer);
}
</script>
Please adapt the above changes according to your project structure and requirements.
if (window.speechSynthesis) { | ||
window.speechSynthesis.cancel() | ||
} | ||
|
||
window.sendMessage = sendMessage | ||
bus.on('on:transcribing', (status: boolean) => { | ||
transcribing.value = 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.
The code snippet provided is almost correct, with only one minor improvement. The speechSynthesis
object might not be defined in some environments when the script runs, so adding a null check before calling its method helps prevent errors.
Optimization Suggestion:
Add an early return after checking if window.speechSynthesis
exists to avoid unnecessary code execution.
Here's the updated code:
@@ -523,7 +523,10 @@ onMounted(() => {
let userFormData = JSON.parse(localStorage.getItem(`${accessToken}userForm`) || '{}')
form_data.value = userFormData
}
+ if (window.speechSynthesis) {
window.speechSynthesis.cancel() // Ensure this happens safely
- }
+ window.sendMessage = sendMessage;
+ bus.on('on:transcribing', (status: boolean) => {
+ transcribing.value = status;
+ });
}
This change ensures that the rest of the function can proceed without relying implicitly on the presence of window.speechSynthesis
.
fix: Fix some phones unable to play audio