Review Request 109695: JJ#241066: Added a prepareToQuit() signal to amarokWindowScript

[Matěj Laitl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109695/#review29953
-----------------------------------------------------------


Looks good, just please resolve the remarks and please add entry to ChangeLog under CHANGES section so that Script developers know and the bug is mentioned.

You may want to for example wait a couple of seconds in your example script to check that scripts can postpone Amarok exit.


src/App.cpp
<http://git.reviewboard.kde.org/r/109695/#comment22322>

    No. Adding arbitrary waits is simply wrong. Scripts have an opportunity to clean up in prepareToQuit() (note that signal->slot delivery is blocking except when the receiver is in another thread)



src/scriptengine/AmarokWindowScript.h
<http://git.reviewboard.kde.org/r/109695/#comment22323>

    Please document this signal so that script developer know its use and semantics.


- Matěj Laitl


On March 24, 2013, 8:56 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109695/
> -----------------------------------------------------------
> 
> (Updated March 24, 2013, 8:56 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Bug 241066 - JJ: Add Signals trackStop and amarokShutdown to Amarok scripting interface
> 
> Changes:
> 1. Added a prepareToQuit() signal to amarokWindowScript
> 2. Delayed the app's exit to 1 second after the signal is emitted
> 2. Replaced kapp macro calls with App::instance() because quit() is not virtual
> 
> Note:
> Signal trackFinished()[trackStop] already exists, so only added prepareToQuit()[amarokShutdown]
> 
> 
> Diffs
> -----
> 
>   src/App.h 97dfdf2 
>   src/App.cpp fdb4255 
>   src/MainWindow.cpp 66f4f76 
>   src/dbus/mpris1/RootHandler.cpp e60eb1b 
>   src/dbus/mpris2/MediaPlayer2.cpp f86ccb3 
>   src/scriptengine/AmarokScript.cpp 922e71d 
>   src/scriptengine/AmarokWindowScript.h 5407579 
>   src/scriptengine/AmarokWindowScript.cpp 897e2da 
> 
> Diff: http://git.reviewboard.kde.org/r/109695/diff/
> 
> 
> Testing
> -------
> 
> Tested the new signal in a script
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130327/54a65920/attachment-0001.html>