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.

Sign up for GitHub

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

Jump to bottom

Feat: Added speed control option for Text-to-Speech module #1317

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Feat: Added speed control option for Text-to-Speech module #1317

wants to merge 1 commit into from
+104 −2

Conversation

heropj
Copy link
Contributor

@heropj heropj commented Jan 1, 2025 •
edited
Loading

fix #1299

  • Made a function to change rate of speech based on user's choice.
  • User can select speed of their choice from the ui.
  • User can also use keyboard shortcut >/< to increase/decrease speed.
  • When a user changes speed, the speech is stopped and 'restarted' with new speed.

image

@heropj heropj force-pushed the feat/speed-control-for-tts branch from 06e0d55 to 6688ec8 Compare January 1, 2025 22:56
kelson42
kelson42 requested changes Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@heropj Thank you again very much for this PR. To me it works fine, but could you please save in the preferences (beside the chosen voice) the speed, so the user does not have always to change back the TTS speed to his prefered speed?

veloman-yunkan
veloman-yunkan reviewed Jan 5, 2025
View reviewed changes
src/texttospeechbar.cpp
@@ -54,6 +58,16 @@ void TextToSpeechBar::setLocale(const QLocale& locale)
}
}

void TextToSpeechBar::setupSpeedOptionsComboBox()
{
ui->speedLabel->setText(gt("speed"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to include the change to resources/i18n/en.json. Please also don't forget to update resources/i18n/qqq.json too.

src/texttospeechbar.cpp Outdated
ui->speedComboBox->setMaxVisibleItems(10);
ui->speedComboBox->setLineEdit(new ComboBoxLineEdit(ui->speedComboBox));

QStringList speedOptions = {"0.25x","0.50x","0.75x","1.00x","1.25x","1.50x","1.75x","2.00x"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we include x in the speed rate value? Won't internationalization be slightly affected? In youtube there is no x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍👍i'll remove it.

@heropj
Copy link
Contributor Author

heropj commented Jan 5, 2025 •
edited
Loading

hello @kelson42 , @veloman-yunkan
There are shortcuts to select language and voice...
eg CREATE_ACTION_SHORTCUT(ToggleTTSLanguageAction, gt("select-read-language"), QKeySequence(Qt::ALT | Qt::SHIFT | Qt::Key_L)); to select language

do we need a shortcut for choosing speed also?? if yes then what should be the key-combination..
ALT+SHIFT+S is already selected as shortcut for "reading selected"

@heropj heropj force-pushed the feat/speed-control-for-tts branch from 6688ec8 to af93a92 Compare January 9, 2025 05:06
@kelson42
Copy link
Collaborator

@heropj Ready for a new review?

@heropj
Copy link
Contributor Author

heropj commented Jan 10, 2025

@kelson42 yes👍👍

veloman-yunkan
veloman-yunkan requested changes Jan 12, 2025
View reviewed changes
src/texttospeechbar.cpp Outdated
Comment on lines 35 to 45
//keyboard shortcuts to control speed of tts
QAction* increaseSpeedAction = new QAction(this);
increaseSpeedAction->setShortcut(QKeySequence("Shift+."));
connect(increaseSpeedAction, &QAction::triggered, this, &TextToSpeechBar::increaseSpeed);

QAction* decreaseSpeedAction = new QAction(this);
decreaseSpeedAction->setShortcut(QKeySequence("Shift+,"));
connect(decreaseSpeedAction, &QAction::triggered, this, &TextToSpeechBar::decreaseSpeed);

this->addAction(increaseSpeedAction);
this->addAction(decreaseSpeedAction);
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Fix indentation
  2. Shouldn't at least some of this code be moved to kiwixapp.cpp (see ToggleTTSLanguageAction as an example)?

src/texttospeechbar.cpp Outdated

//keyboard shortcuts to control speed of tts
QAction* increaseSpeedAction = new QAction(this);
increaseSpeedAction->setShortcut(QKeySequence("Shift+."));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why "Shift+." rather than ">"? Same for the decrease speed action shortcut.

Copy link
Contributor Author

@heropj heropj Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

shift+. is clearer for users?? it's clear that they have to press shift and .
while for '>' they might get confused..??

in description of this PR i wrote "shift + >/<" as keyboard shortcut for speed control, that was wrong..i have edited it to "shift + ./,"

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question is whether you want the shortcut to be bound to the "Shift + ." key sequence (which is equivalent to ">" on my keyboard but may resolve to something else on different keyboard layouts)¹? Or the underlying reason for selecting this key combination is mnemonics - increase with ">" and decrease with "<"?

¹ BTW, what if there is a keyboard layout where "." itself has to be entered via a Shift key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay..i got it.., we should go with mnemonics... i'll change it to '>' for speed up and '<' for speed down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done..
image

src/texttospeechbar.cpp Outdated
void TextToSpeechBar::resetSpeedComboBox()
{
double savedSpeed = KiwixApp::instance()->getSavedTtsSpeed(m_speech.locale().name());
int index = (savedSpeed == 1.0) ? 3 : (savedSpeed - 0.25) * 4; //default speed: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the special handling of the savedSpeed == 1.0 case needed? Don't you trust the (savedSpeed - 0.25) * 4 formula always working correctly?

src/texttospeechbar.cpp Outdated
int currentIndex = ui->speedComboBox->currentIndex();
if (currentIndex < ui->speedComboBox->count() - 1) {
ui->speedComboBox->setCurrentIndex(currentIndex + 1);
onSpeedChanged(currentIndex + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't ui->speedComboBox->setCurrentIndex() result in onSpeedChanged() being called automatically due to the correctly setup signal handling? Same for decreaseSpeed()

@heropj heropj force-pushed the feat/speed-control-for-tts branch from af93a92 to c749921 Compare January 17, 2025 21:14
@heropj heropj force-pushed the feat/speed-control-for-tts branch from c749921 to afced9b Compare January 18, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veloman-yunkan veloman-yunkan veloman-yunkan requested changes

@kelson42 kelson42 kelson42 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to speed-up Text To Speech
3 participants
@heropj @kelson42 @veloman-yunkan