Try to add support for device's default TTS and decouple the languages supported by TTS engine and Whisper #29
Conversation
Decouple the languages supported by TTS engine and Whisper. The app will translate all text it heard and speak only available language in its TTS engine.
…dd_hiai # Conflicts: # app/src/main/java/nie/translator/rtranslator/tools/TTS.java
Keep filtering low-quality voices; Add some comments.
…nguage; Show if the language is available in TTS in language list directly; Remove a duplicate "getLanguages".
JingziC
changed the title
|
Hi again JingziC, In these days I will review the pull request (don't make any changes in the meantime, If you have other ideas, leave a comment). I'll tell you in advance that I won't be very fast in the review because I don't have much free time and I'm not used to receiving and reviewing pull requests, so I still need more practice to become faster. Thank you again! |
|
Hi niedev, thanks for the prompt reply. I understand and will not change this request and corresponding branch. Please take you time. |
There was a problem hiding this comment.
There are some fixes you should do to get merged, but otherwise great job!
Things to fix regarding code form and comments:
-
From line 56 to 87 of TTS.java the indentation needs to be fixed, also the commented code needs to use multi-line comment (/* */) instead of single-line comment (//).
-
The comments indicating changes made in this PR should be removed, so the ones on lines 57, 184 of TTS.java and on line 132 of Global.java (I appreciate that you want to communicate the changes you made but it is not necessary to do it inside the code, since this code will be inserted in the app, so these comments would lose meaning for future readers).
-
On line 214 of TTS.java the variable "found_language" should be renamed to "foundLanguage", to be consistent with the style used for the names of the other variables in the code.
-
On lines 184 and 211 of WalkieTalkieService.java the comments should be removed (there is already a comment above that explains what the code does, and the two comments are not very clear).
Things to fix regarding the functionality of the code:
-
On line 160 of CustomLocale.java " (No Text-to-Speech)" should be replaced with " (no TTS)" (shortening it makes it look nicer and, above all, causes fewer problems with phones with small screens or with very large text set).
-
On lines 184 and 211 of WalkieTalkieService.java, the control "TTS.ttsLanguages.contains(languageOfText)" should be replaced with "CustomLocale.containsLanguage(TTS.ttsLanguages, languageOfText)", which is the same one used in the control above to decide whether to speak (this is because "containsLanguage" is less restrictive than "contains", so if you don't use "containsLanguage" below, the microphone is often reactivated even if the TTS speaks, while using "contains" in both cases would lead to not speaking even when in reality you could).
After this fixes I will merge the pull request, I'll do some more tests and then I will do a new release of RTranslator based on these changes (where I will obviously quote you).
- Remove redundant TTS engine comments and adjust indentation. - Shorten "No Text-to-Speech" to "No TTS" in language list. - Change language availability check in WalkieTalkieService to use CustomLocale.containsLanguage(). - Correct boolean variable naming for consistency (found_language -> foundLanguage).
There was a problem hiding this comment.
Thank you for the fast, detailed and helpful review on both format and functionality. I agree with all the comments and made the following changes according to this review:
Things to fix regarding code form and comments:
From line 56 to 87 of TTS.java the indentation needs to be fixed, also the commented code needs to use multi-line comment (/* */) instead of single-line comment (//).
The comments are multi-line comments now and the indentation has been adjusted.
The comments indicating changes made in this PR should be removed, so the ones on lines 57, 184 of TTS.java and on line 132 of Global.java (I appreciate that you want to communicate the changes you made but it is not necessary to do it inside the code, since this code will be inserted in the app, so these comments would lose meaning for future readers).
The comments at these lines have been removed.
On line 214 of TTS.java the variable "found_language" should be renamed to "foundLanguage", to be consistent with the style used for the names of the other variables in the code.
Their name are foundLanguage now.
On lines 184 and 211 of WalkieTalkieService.java the comments should be removed (there is already a comment above that explains what the code does, and the two comments are not very clear).
The comments in these lines has been removed.
Things to fix regarding the functionality of the code:
On line 160 of CustomLocale.java " (No Text-to-Speech)" should be replaced with " (no TTS)" (shortening it makes it look nicer and, above all, causes fewer problems with phones with small screens or with very large text set).
It has been changed to "No TTS".
On lines 184 and 211 of WalkieTalkieService.java, the control "TTS.ttsLanguages.contains(languageOfText)" should be replaced with "CustomLocale.containsLanguage(TTS.ttsLanguages, languageOfText)", which is the same one used in the control above to decide whether to speak (this is because "containsLanguage" is less restrictive than "contains", so if you don't use "containsLanguage" below, the microphone is often reactivated even if the TTS speaks, while using "contains" in both cases would lead to not speaking even when in reality you could).
Changed to "containsLanguage()".
I tested the modifed version by Github Actions and also on my own devices. This version could work as expectation.
I hope these changes addressed all the concerns mentioned in this review.
Thank you again for maintaining this project.
Add support for Huawei's TTS and decouple the languages supported by TTS engine and Whisper
Hello,
Thank you for maintaining this project. The project has been very useful for me. I installed the latest version to a Huawei device with HarmonyOS 4.2.0 but it could not detect Huawei's TTS engine as what happened on Samsung previously. Besides, I noticed that the list of languages only included the languages supported by both TTS and Whisper when it detected the TTS engine. However, users including me may expect to use only the Whisper's translation even the Google, Samsung or Huawei's TTS are not available because the supported languages in TTS sometimes are limited.
To address these issues, I have made the following modifications:
The modifications were tested on a Huawei device with HarmonyOS 4.2.0 (com.huawei.hiai) and and a Samsung device with Android 14 (com.samsung.SMT). The built app looks good for me on both devices. However I did not have a chance to test on other platforms. The .apk prerelease of this version is avaliable and can also be built from the branch.
The updated language list displayed on Samsung with Samsung's TTS is as follows.