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

Store selected TTS voice in localStorage #1241

Merged
merged 5 commits into from
Sep 21, 2023

Conversation

sbwhitt
Copy link
Contributor

@sbwhitt sbwhitt commented Sep 18, 2023

Closes #1131

TTS voice selection is stored after user selects from voice menu. On load, the last selected voice will attempt to be selected from localStorage and loaded as the active voice. If no voice is stored, the default will be selected.

Voice is stored under the localStorage key 'playback-voice'.

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #1241 (5e5fdb8) into master (650c9f7) will increase coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 5e5fdb8 differs from pull request most recent head d4e6c89. Consider uploading reports for the commit d4e6c89 to get more accurate results

@@            Coverage Diff             @@
##           master    #1241      +/-   ##
==========================================
+ Coverage   69.22%   69.33%   +0.11%     
==========================================
  Files          60       60              
  Lines        5095     5101       +6     
  Branches     1067     1070       +3     
==========================================
+ Hits         3527     3537      +10     
+ Misses       1541     1537       -4     
  Partials       27       27              
Files Changed Coverage Δ
src/plugins/tts/AbstractTTSEngine.js 55.00% <100.00%> (+7.12%) ⬆️

@cdrini
Copy link
Contributor

cdrini commented Sep 18, 2023

Hi, nice thank you @sbwhitt ! This is a great first issue 😊 This would better live inside AbstractTTSEngine.setVoice for the setting, and AbstractTTSEngine.getBestBookVoice for the reading. In getBestBookVoice, you should at the very start of the function check if the book language has preferred voice set in local storage, and output that before all the other logic. Note browsers sometimes change the languages available, so check to make sure the voice is actually available in the voices array!

Overall:

  • Move localStorage.getItem to AbstractTTSEngine.getBestBookVoice
  • Move localStorage.setItem to AbstractTTSEngine.setVoice
  • Make localStorage entry book-language specific (eg BRplayback-voice-en: Foo Bar)
  • Handle voice in localStorage missing from voices array
  • Add some unit tests for this one in tests/jest/plugins/tts/AbstractTTSEngine.test.js

@sbwhitt
Copy link
Contributor Author

sbwhitt commented Sep 18, 2023

Hi, nice thank you @sbwhitt ! This is a great first issue 😊 This would better live inside AbstractTTSEngine.setVoice for the setting, and AbstractTTSEngine.getBestBookVoice for the reading. In getBestBookVoice, you should at the very start of the function check if the book language has preferred voice set in local storage, and output that before all the other logic. Note browsers sometimes change the languages available, so check to make sure the voice is actually available in the voices array!

Overall:

  • Move localStorage.getItem to AbstractTTSEngine.getBestBookVoice
  • Move localStorage.setItem to AbstractTTSEngine.setVoice
  • Make localStorage entry book-language specific (eg BRplayback-voice-en: Foo Bar)
  • Handle voice in localStorage missing from voices array
  • Add some unit tests for this one in tests/jest/plugins/tts/AbstractTTSEngine.test.js

Ok, great, I will be working on this. Thank you for the tips!

@sbwhitt sbwhitt force-pushed the feat/tts-voice branch 2 times, most recently from ec35c9a to 49d4615 Compare September 21, 2023 14:01
Copy link
Contributor

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great @sbwhitt ! Testing it out now...

src/plugins/tts/AbstractTTSEngine.js Outdated Show resolved Hide resolved
src/plugins/tts/AbstractTTSEngine.js Outdated Show resolved Hide resolved
@cdrini
Copy link
Contributor

cdrini commented Sep 21, 2023

Works perfectly! Tested books in different languages, tested that the voice is remembered across languages :) Thank you @sbwhitt !

@cdrini cdrini merged commit 5aa78e2 into internetarchive:master Sep 21, 2023
7 checks passed
@sbwhitt sbwhitt deleted the feat/tts-voice branch September 21, 2023 20:53
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.

Remember selected voice for language
2 participants