-
-
Notifications
You must be signed in to change notification settings - Fork 0
Wrong TTS language locale conversion #16
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
Conversation
7ae5280
to
06ce844
Compare
WalkthroughThis pull request updates the library version information in the Gradle build configuration and refines the language selection logic in the TextToSpeechWrapper class. The build file now uses Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TextToSpeechWrapper
participant TTS_Engine
Caller->>TextToSpeechWrapper: Initialize with language & contentProviderLanguage
TextToSpeechWrapper->>TextToSpeechWrapper: sttLanguage = language.ifEmpty(contentProviderLanguage)
TextToSpeechWrapper->>TTS_Engine: setLanguage(sttLanguage.toLowerCase())
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/src/main/java/ai/elimu/common/utils/di/TextToSpeechWrapper.kt (1)
38-42
: Improved language selection with misleading variable name.The language selection logic is now more concise with the
ifEmpty
function, which is a good improvement. However, the variable namesttLanguage
is misleading in a Text-to-Speech (TTS) context as "STT" typically refers to "Speech-to-Text".- val sttLanguage = language.ifEmpty { + val ttsLanguage = language.ifEmpty { contentProviderLanguage } - tts.setLanguage(sttLanguage.lowercase().toLanguage().toLocale()) + tts.setLanguage(ttsLanguage.lowercase().toLanguage().toLocale())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
utils/build.gradle
(2 hunks)utils/src/main/java/ai/elimu/common/utils/di/TextToSpeechWrapper.kt
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-latest, 17)
🔇 Additional comments (3)
utils/src/main/java/ai/elimu/common/utils/di/TextToSpeechWrapper.kt (1)
42-42
: Proper language locale handling for TTS.The addition of
lowercase()
before callingtoLanguage().toLocale()
ensures consistent language code formatting, which helps resolve locale conversion issues. This appears to directly address the "Wrong TTS language locale conversion" problem mentioned in the PR title.utils/build.gradle (2)
10-10
: Version incremented appropriately for bug fix.The library version has been properly incremented from '1.0.5-SNAPSHOT' to '1.0.6-SNAPSHOT' following semantic versioning conventions for a patch release.
21-21
: Version code updated consistently with semantic version.The version code has been updated from 1000005 to 1000006, maintaining consistency with the semantic version 1.0.6.
} else { | ||
tts.setLanguage(contentProviderLanguage.toLanguage().toLocale()) | ||
|
||
val sttLanguage = language.ifEmpty { |
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.
@tuancoltech The language
variable here should probably be removed as a parameter? (Since the apps using this utils library don't know which language to use.)
class TextToSpeechWrapper(context: Context, language: String, contentProviderId: String)
↓
class TextToSpeechWrapper(context: Context, contentProviderId: String)
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.
@jo-elimu The language
parameter here is for apps to be able to pass a custom TTS language to the TextToSpeech engine, disregard the one selected in ContentProvidre.
It's for the future.
Wrong TTS language locale conversion.
Related to elimu-ai/vitabu#177
Summary by CodeRabbit