Skip to content
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

[voicerss] Add supported voices #10167

Closed
wants to merge 3 commits into from
Closed

[voicerss] Add supported voices #10167

wants to merge 3 commits into from

Conversation

curlyboi
Copy link
Contributor

Added all supported voices for the individual languages taken from API docs. When asked about voices list without locale, it will return all of them, when asked about particular locale, only voices for that locale are returned.

Signed-off-by: cURLy bOi [email protected]

Added all supported voices for the individual languages taken from API docs. When asked about voices list without locale, it will return all of them, when asked about particular locale, only voices for that locale are returned.

Signed-off-by: cURLy bOi <[email protected]>
@lolodomo
Copy link
Contributor

lolodomo commented Feb 15, 2021

Additionnally, is it a change that will break all existing setup ?
For example, my current setup is with that:

org.openhab.voice:defaultVoice=voicerss:frFR

What should be the new value ?
Will it still work with my current value ?

Did you check the result with the existing console command voice voices ?

@curlyboi
Copy link
Contributor Author

Additionnally, is it a change that will break all existing setup ?
For example, my current setup is with that:

org.openhab.voice:defaultVoice=voicerss:frFR

What should be the new value ?
Will it still work with my current value ?

Did you check the result with the existing console command voice voices ?

i did not because i don't even have java devenv. my commit is simply a suggestion how to implement the full api. i don't think it is possible to endlessly keep backwards compatibility with old systems and this would be a fix that's easily solved anyway.

@lolodomo
Copy link
Contributor

lolodomo commented Feb 15, 2021

That means you propose a PR you even did not test !!! Very dangerous. The strict minimum would be to mention it.

@curlyboi
Copy link
Contributor Author

That means you propose a PR you even did not test !!! Very dangerous. The strict minimum would be to mention it.

Fair point, I will remember it. I have 20 years of programming experience and had my Java pro friend code-review it to make sure it's not absolute rubbish :)

Added the suggested lowercase conversion to catch more input

Signed-off-by: cURLy bOi <[email protected]>
@lolodomo
Copy link
Contributor

lolodomo commented Feb 16, 2021

I suggest to not merge this PR as it was not tested, even not by his author.
I will test it myself but it is not in my priorities.

@lolodomo
Copy link
Contributor

By the way, the PR is not compiling.

Signed-off-by: cURLy bOi <[email protected]>
@curlyboi
Copy link
Contributor Author

By the way, the PR is not compiling.

Fixed the typo. Assuming it will compile, can I test this by grabbing an artifact somewhere, putting it in /var/lib/wherever and test myself?

@lolodomo
Copy link
Contributor

My feeling is that you could simply keep backward compatibility by keeping VoiceRSS in all your list of voices.

Regarding locale.toLanguageTag(), I don't know if this clear for you what value it returns but it is not for me.

@lolodomo
Copy link
Contributor

locale.toLanguageTag() returns for example fr-FR so the toLowerCase() was necessary.
By the way, your code is not sufficient as each voice must have a unique ID and this is not the case with your current code.
I will propose my version that will be backward compatible.

@curlyboi
Copy link
Contributor Author

My feeling is that you could simply keep backward compatibility by keeping VoiceRSS in all your list of voices.

So you propose that each locale's map will contain the individual voices, and then also VoiceRSS which will translate to "nothing" (not calling a specific voice)? This seems like adding unnecessary clutter just because your particular system has it like this.

@curlyboi
Copy link
Contributor Author

By the way, your code is not sufficient as each voice must have a unique ID and this is not the case with your current code.

Here you are right. Furthermore, I just realized that the voice is not taken into account in the createURL, so even though the voice is selectable, it would make no difference in the actual API call. So thank you for making me realize this.

@lolodomo
Copy link
Contributor

I have done the required changes (backward compatibility + new voice UID) but I always get the same voice. I imagine the voice has to be passed to the request.

@curlyboi
Copy link
Contributor Author

I imagine the voice has to be passed to the request.

Exactly. The createURL needs to take the voice as another argument, that is passed from getTextToSpeech (so that needs to be added there as well and passed to the createURL) and finally, this is called from VoiceRSSTTSService.java in synthesize, where you have the Voice object with the available voice name.

In the API URL, the parameter is simply v, so &v=Iva for non-default French voice.

@lolodomo
Copy link
Contributor

This was clearly not a change that can be done without compiling and without any testing !!!!
I am on it.

@curlyboi
Copy link
Contributor Author

This was clearly not a change that can be done without compiling and without any testing !!!!

You are right. Maybe this will finally get me to create a dev environment so I can properly dev plugins.

@lolodomo
Copy link
Contributor

Ok, I have something working.
This was even more complicated as the cache has to be enhaced too.

Considering the number of changes, the best solution would be that I create my own PR. Are you ok ?

@lolodomo
Copy link
Contributor

lolodomo commented Feb 16, 2021

Regarding the backward compatibility, it consists only in adding a "default" voice with the same UID as before (the UID is what every user has set). And I also take care to reuse the same cached files when this "default" voice is set.
This is clean in my point of view.
I will publish my PR tomorrow.

PS: I switched to Zola voice ;)

@lolodomo
Copy link
Contributor

To be closed (very partial implementation not working).

@curlyboi
Copy link
Contributor Author

curlyboi commented Feb 17, 2021

Agreed to migrate to different PR.

@curlyboi curlyboi closed this Feb 17, 2021
@curlyboi
Copy link
Contributor Author

PS: I switched to Zola voice ;)

I only have this "Josef" dude :D Would prefer a girl too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants