Re: Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/#review76136 --- Ship it! Looks good, thank you! src/knotifyconfig.cpp https://git.reviewboard.kde.org/r/122555/#comment52524 Remove this, I've pushed it separately src/kstatusnotifieritem.h https://git.reviewboard.kde.org/r/122555/#comment52525 (don't forget to push this separately too) - Martin Klapetek On Feb. 14, 2015, 12:01 a.m., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/ --- (Updated Feb. 14, 2015, 12:01 a.m.) Review request for KDE Frameworks and Frederik Gladhorn. Repository: knotifications Description --- Add optional dependency on Qt5TextToSpeech for speech notifications. Diffs - CMakeLists.txt 208fd02153a0607e4cfbc02e4b289ef835cedbfd src/CMakeLists.txt 6a3d81707a0e27e2d7bbfbf7f3924852ab737bf9 src/knotification.h dc0c975e261f1a03b8b4875bc1069417cf8ea094 src/knotificationmanager.cpp affb6a673468bf6585cbda6fafdd008beb445cd9 src/knotifyconfig.cpp af6be92bd320eaa881d8420cadf175edf6bf41aa src/kstatusnotifieritem.h 74b97ba7c63d52cae8ee80326daa9f24ce03a331 src/notifybyktts.h 43756f776678bd7700a77a3357577363b36d2542 src/notifybyktts.cpp a2a15a9c77089527f54dfc63f13699d44336dda1 src/notifybytts.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122555/diff/ Testing --- As I said in the knotifyconfig review something at runtime isn't refreshing/reloading the config when it is changed. Otherwise this works fine when QtSpeech is available. QtSpeech is still in development, so this change is added as an optional dependency. Thanks, Jeremy Whiting ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.
On Feb. 13, 2015, 5:11 a.m., Martin Klapetek wrote: src/notifybyspeech.cpp, line 65 https://git.reviewboard.kde.org/r/122555/diff/1/?file=348637#file348637line65 Is there any way to know when the say() has finished? Because the finished() below will delete the notification object if it's the only plugin, which may just interrupt the speech in the middle We could watch the tts object for state change, but we wouldn't know which speech job it's finishing. Anyway, I don't think it matters, we pass the text to speak then the knotification object can get deleted with no effect on the speech itself from what I've heard (with my ears, not through the grape vine). - Jeremy --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/#review75986 --- On Feb. 12, 2015, 8:11 p.m., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/ --- (Updated Feb. 12, 2015, 8:11 p.m.) Review request for KDE Frameworks and Frederik Gladhorn. Repository: knotifications Description --- Add optional dependency on Qt5TextToSpeech for speech notifications. Diffs - CMakeLists.txt 208fd02153a0607e4cfbc02e4b289ef835cedbfd src/CMakeLists.txt 6a3d81707a0e27e2d7bbfbf7f3924852ab737bf9 src/knotification.h c85621699793436442090b7f94ea82ef10c45b89 src/knotificationmanager.cpp affb6a673468bf6585cbda6fafdd008beb445cd9 src/kstatusnotifieritem.h 113dad513c320ef97f59b221b3541ca2f388693e src/notifybyktts.h 43756f776678bd7700a77a3357577363b36d2542 src/notifybyktts.cpp a2a15a9c77089527f54dfc63f13699d44336dda1 src/notifybyspeech.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122555/diff/ Testing --- As I said in the knotifyconfig review something at runtime isn't refreshing/reloading the config when it is changed. Otherwise this works fine when QtSpeech is available. QtSpeech is still in development, so this change is added as an optional dependency. Thanks, Jeremy Whiting ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/ --- (Updated Feb. 13, 2015, 4:01 p.m.) Review request for KDE Frameworks and Frederik Gladhorn. Changes --- Fixed issues noted. Repository: knotifications Description --- Add optional dependency on Qt5TextToSpeech for speech notifications. Diffs (updated) - CMakeLists.txt 208fd02153a0607e4cfbc02e4b289ef835cedbfd src/CMakeLists.txt 6a3d81707a0e27e2d7bbfbf7f3924852ab737bf9 src/knotification.h dc0c975e261f1a03b8b4875bc1069417cf8ea094 src/knotificationmanager.cpp affb6a673468bf6585cbda6fafdd008beb445cd9 src/knotifyconfig.cpp af6be92bd320eaa881d8420cadf175edf6bf41aa src/kstatusnotifieritem.h 74b97ba7c63d52cae8ee80326daa9f24ce03a331 src/notifybyktts.h 43756f776678bd7700a77a3357577363b36d2542 src/notifybyktts.cpp a2a15a9c77089527f54dfc63f13699d44336dda1 src/notifybytts.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122555/diff/ Testing --- As I said in the knotifyconfig review something at runtime isn't refreshing/reloading the config when it is changed. Otherwise this works fine when QtSpeech is available. QtSpeech is still in development, so this change is added as an optional dependency. Thanks, Jeremy Whiting ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/#review76006 --- src/notifybyktts.h https://git.reviewboard.kde.org/r/122555/#comment52450 I have no idea how this is used, should it be i18n'ed? And Text to Speech - Frederik Gladhorn On Feb. 13, 2015, 3:11 a.m., Jeremy Whiting wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/ --- (Updated Feb. 13, 2015, 3:11 a.m.) Review request for KDE Frameworks and Frederik Gladhorn. Repository: knotifications Description --- Add optional dependency on Qt5TextToSpeech for speech notifications. Diffs - CMakeLists.txt 208fd02153a0607e4cfbc02e4b289ef835cedbfd src/CMakeLists.txt 6a3d81707a0e27e2d7bbfbf7f3924852ab737bf9 src/knotification.h c85621699793436442090b7f94ea82ef10c45b89 src/knotificationmanager.cpp affb6a673468bf6585cbda6fafdd008beb445cd9 src/kstatusnotifieritem.h 113dad513c320ef97f59b221b3541ca2f388693e src/notifybyktts.h 43756f776678bd7700a77a3357577363b36d2542 src/notifybyktts.cpp a2a15a9c77089527f54dfc63f13699d44336dda1 src/notifybyspeech.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122555/diff/ Testing --- As I said in the knotifyconfig review something at runtime isn't refreshing/reloading the config when it is changed. Otherwise this works fine when QtSpeech is available. QtSpeech is still in development, so this change is added as an optional dependency. Thanks, Jeremy Whiting ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122555/#review75986
---
Looks good, thanks for that! I'm wondering if using TTS in the name rather than
Speech would be more appropriate? TTS is an abbreviation that is well known
while Speech can mean many things...
src/kstatusnotifieritem.h
https://git.reviewboard.kde.org/r/122555/#comment52414
This is unrelated to this patch, but please do commit it (separately)
src/notifybyspeech.cpp
https://git.reviewboard.kde.org/r/122555/#comment52415
CamelCaseHeaders?
src/notifybyspeech.cpp
https://git.reviewboard.kde.org/r/122555/#comment52416
KNotifyConfig * config ) -- KNotifyConfig *config)
src/notifybyspeech.cpp
https://git.reviewboard.kde.org/r/122555/#comment52420
Could you add some comments on what this is and what it's for?
src/notifybyspeech.cpp
https://git.reviewboard.kde.org/r/122555/#comment52417
No spaces inside ()s
src/notifybyspeech.cpp
https://git.reviewboard.kde.org/r/122555/#comment52418
No spaces in ()s and add {}
src/notifybyspeech.cpp
https://git.reviewboard.kde.org/r/122555/#comment52419
Is there any way to know when the say() has finished? Because the
finished() below will delete the notification object if it's the only plugin,
which may just interrupt the speech in the middle
- Martin Klapetek
On Feb. 13, 2015, 4:11 a.m., Jeremy Whiting wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122555/
---
(Updated Feb. 13, 2015, 4:11 a.m.)
Review request for KDE Frameworks and Frederik Gladhorn.
Repository: knotifications
Description
---
Add optional dependency on Qt5TextToSpeech for speech notifications.
Diffs
-
CMakeLists.txt 208fd02153a0607e4cfbc02e4b289ef835cedbfd
src/CMakeLists.txt 6a3d81707a0e27e2d7bbfbf7f3924852ab737bf9
src/knotification.h c85621699793436442090b7f94ea82ef10c45b89
src/knotificationmanager.cpp affb6a673468bf6585cbda6fafdd008beb445cd9
src/kstatusnotifieritem.h 113dad513c320ef97f59b221b3541ca2f388693e
src/notifybyktts.h 43756f776678bd7700a77a3357577363b36d2542
src/notifybyktts.cpp a2a15a9c77089527f54dfc63f13699d44336dda1
src/notifybyspeech.cpp PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/122555/diff/
Testing
---
As I said in the knotifyconfig review something at runtime isn't
refreshing/reloading the config when it is changed. Otherwise this works fine
when QtSpeech is available.
QtSpeech is still in development, so this change is added as an optional
dependency.
Thanks,
Jeremy Whiting
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122555/ --- Review request for KDE Frameworks and Frederik Gladhorn. Repository: knotifications Description --- Add optional dependency on Qt5TextToSpeech for speech notifications. Diffs - CMakeLists.txt 208fd02153a0607e4cfbc02e4b289ef835cedbfd src/CMakeLists.txt 6a3d81707a0e27e2d7bbfbf7f3924852ab737bf9 src/knotification.h c85621699793436442090b7f94ea82ef10c45b89 src/knotificationmanager.cpp affb6a673468bf6585cbda6fafdd008beb445cd9 src/kstatusnotifieritem.h 113dad513c320ef97f59b221b3541ca2f388693e src/notifybyktts.h 43756f776678bd7700a77a3357577363b36d2542 src/notifybyktts.cpp a2a15a9c77089527f54dfc63f13699d44336dda1 src/notifybyspeech.cpp PRE-CREATION Diff: https://git.reviewboard.kde.org/r/122555/diff/ Testing --- As I said in the knotifyconfig review something at runtime isn't refreshing/reloading the config when it is changed. Otherwise this works fine when QtSpeech is available. QtSpeech is still in development, so this change is added as an optional dependency. Thanks, Jeremy Whiting ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
