Review Request 122555: knotifications: Add optional dependency on Qt5TextToSpeech for speech notifications.

[Martin Klapetek]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150213/ac71393e/attachment.html>