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

Implement QtSpeech-based text-to-speech backend #2939

Merged
merged 1 commit into from Mar 16, 2017

Conversation

@davidebeatrici
Copy link
Member

commented Mar 13, 2017

Qt 5.8 introduced Qt Speech, a module that contains a TTS system which works on every OS.

This commit provides a new file called "TextToSpeech.cpp", without any suffix, indicating that it is OS-independent.
If the "qtspeech" CONFIG option is present and the "texttospeech" Qt module is available, the new file is included instead of the OS-specific one.


class TextToSpeechPrivate {
public:
QTextToSpeech *mSpeech;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

s/mSpeech/m_tts/

class TextToSpeechPrivate {
public:
QTextToSpeech *mSpeech;
QVector<QVoice> mVoices;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

s/mVoices/m_voices/

};

TextToSpeechPrivate::TextToSpeechPrivate() {
mSpeech = NULL;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

Drop. We assign to mSpeech below.


TextToSpeechPrivate::~TextToSpeechPrivate() {
if (mSpeech)
delete mSpeech;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

You should not need to delete mSpeech, since it is a QObject and we're its parent.
If we are destroyed, our children will be destroyed, too.

(Leave an empty destructor)

}

void TextToSpeechPrivate::say(const QString &text) {
if (mSpeech)

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

mSpeech is never null.

}

void TextToSpeechPrivate::setVolume(int volume) {
if (mSpeech)

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

mSpeech is never null.

@mkrautz
Copy link
Member

left a comment

A few coding style comments...

Also:
I'm OK with merging this, but it should not be on by default for 1.3.0.
Please add a "qtspeech" CONFIG option and lock the feature behind it. And document t in INSTALL. Remember to note the requirements (Qt 5.8?)...

@@ -64,6 +64,11 @@ CONFIG(static) {
QT *= network sql xml svg
isEqual(QT_MAJOR_VERSION, 5) {
QT *= widgets

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

Wrap in CONFIG(qtspeech) {
...
}


qtHaveModule(texttospeech) {
QT *= texttospeech
QT_TTS = "true"

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

Drop.

@@ -198,6 +203,10 @@ SOURCES *= BanEditor.cpp \
widgets/MUComboBox.cpp \
DeveloperConsole.cpp

!isEmpty(QT_TTS) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

CONFIG(qtspeech) { .... } instead of QT_TTS.

SOURCES *= GlobalShortcut_win.cpp TextToSpeech_win.cpp Overlay_win.cpp SharedMemory_win.cpp Log_win.cpp os_win.cpp TaskList.cpp ../../overlay/ods.cpp UserLockFile_win.cpp
SOURCES *= GlobalShortcut_win.cpp Overlay_win.cpp SharedMemory_win.cpp Log_win.cpp os_win.cpp TaskList.cpp ../../overlay/ods.cpp UserLockFile_win.cpp

isEmpty(QT_TTS) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

CONFIG(qtspeech) { .... } instead of QT_TTS.

OBJECTIVE_SOURCES *= TextToSpeech_macx.mm GlobalShortcut_macx.mm os_macx.mm Log_macx.mm AppNap.mm
OBJECTIVE_SOURCES *= GlobalShortcut_macx.mm os_macx.mm Log_macx.mm AppNap.mm

isEmpty(QT_TTS) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

CONFIG(qtspeech) { .... } instead of QT_TTS.

SOURCES *= os_unix.cpp GlobalShortcut_unix.cpp TextToSpeech_unix.cpp Overlay_unix.cpp SharedMemory_unix.cpp Log_unix.cpp
SOURCES *= os_unix.cpp GlobalShortcut_unix.cpp Overlay_unix.cpp SharedMemory_unix.cpp Log_unix.cpp

isEmpty(QT_TTS) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

CONFIG(qtspeech) { .... } instead of QT_TTS.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qt-speech-tts branch from 378a4c7 to e0f1c11 Mar 14, 2017

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 14, 2017

@davidebeatrici You should update the INSTALL file with docs about the new qtspeech option.
Also, you need to update the commit message/subject and PR to reflect the new state of your code. (It's no longer "instead of OS-specific TTS sytems")

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2017

Oh, I didn't push, sorry.

@davidebeatrici davidebeatrici changed the title Use Qt Speech if available, instead of OS-specific TTS systems Use Qt Speech if available Mar 14, 2017

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qt-speech-tts branch from e0f1c11 to ff897b5 Mar 14, 2017

INSTALL Outdated
@@ -189,3 +189,7 @@ CONFIG+=g15-emulator (Mumble, Win32)
CONFIG+=no-manual-plugin (Mumble)
Don't include the built-in 'manual' positional
audio plugin.

CONFIG+=qtspeech (Mumble)
Use Qt's TTS system, part of the Qt Speech

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 14, 2017

Member

How about:

Use Qt's text-to-speech system (part of the experimental QtSpeech module) instead of Mumble's own OS-specific text-to-speech implementations.

?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 14, 2017

Author Member

Sounds good.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 14, 2017

Author Member

Use Qt's text-to-speech system (part of the experimental Qt Speech module), if available, instead of Mumble's own OS-specific text-to-speech implementations.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qt-speech-tts branch from ff897b5 to 5de9c58 Mar 14, 2017

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

This part of your commit message is outdated now:

It also changes the project file so that it checks if the "texttospeech" Qt module is available before including the new file and avoid including the OS-specific one.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qt-speech-tts branch from 5de9c58 to 6d6dbd1 Mar 15, 2017

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

The commit message is still not 100% true. It says this new implementation will only be used if you have CONFIG+=qtspeech AND have the module available.

However, if you use CONFIG+=qtspeech, and the module is not available -- it will not build correctly, and fail during the build, because the Qt module isn't available.

I suggest you add an else clause to the Qt module check, and error() if the module is not available.
That way, CONFIG+=qtspeech will not work (it will fail hard) on systems w/o the proper module.

How about this commit message instead?

Use Qt Speech if specified and available

Qt 5.8 introduced Qt Speech, a module that contains a TTS system which works on every OS.

This commit implements a new OS-independent Text-To-Speech engine using Q Speech.

It can be enabled with CONFIG+=qtspeech, if the Qt version being used has the module available.

Use Qt Speech if specified and available
Qt 5.8 introduced Qt Speech, a module that contains a TTS system which works on every OS.

This commit implements a new OS-independent Text-To-Speech engine using Qt Speech.

It can be enabled with CONFIG+=qtspeech, if the Qt version being used has the module available.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qt-speech-tts branch from 6d6dbd1 to 08e2d0a Mar 16, 2017

@mkrautz mkrautz changed the title Use Qt Speech if available Implement QtSpeech-based text-to-speech backend Mar 16, 2017

@mkrautz mkrautz merged commit b9165ae into mumble-voip:master Mar 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.