Review Request 109846: BR 259862, 222571: Add options to pause playback on suspend and inhibit automatic suspend if playing

[Matěj Laitl]

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


Thanks again for fixing more requested bugs/features. I like the idea with PowerManager, just please have a look at some suggestions below.


File Attachment: PlaybackConfig Dialog
<http://git.reviewboard.kde.org//r/109846/#fcomment37>
    I would name this "System Suspend Options" so that it is no doubt what "suspend" it is.


src/CMakeLists.txt
<http://git.reviewboard.kde.org/r/109846/#comment22648>

    I think this class deserves to be moved to src/playback/ - a place I'd like to move EngineController to, too.



src/PowerManager.h
<http://git.reviewboard.kde.org/r/109846/#comment22636>

    No need to create extra same-named namespace, please remove it.



src/PowerManager.h
<http://git.reviewboard.kde.org/r/109846/#comment22639>

    Please provide at least short documentation mentioning the purpose of this class.



src/PowerManager.h
<http://git.reviewboard.kde.org/r/109846/#comment22638>

    No need to make this a singleton, just create it in EngineController constructor and parent it to EngineController. (as it doesn't have sense without it)



src/PowerManager.h
<http://git.reviewboard.kde.org/r/109846/#comment22643>

    I don't understand why you need state tracking at all. Just call inhibitSuspend() in slotPlaying() and stopInhibitingSuspend() in slotNotPlaying()
    
    You can use m_cookieVal to track whether you're already inhibiting or not.



src/PowerManager.h
<http://git.reviewboard.kde.org/r/109846/#comment22646>

    I suggest m_inhibitionCookie name.



src/PowerManager.cpp
<http://git.reviewboard.kde.org/r/109846/#comment22640>

    own include is mentioned first and separated by an extra line, see HACKING/intro_and_style.txt



src/PowerManager.cpp
<http://git.reviewboard.kde.org/r/109846/#comment22641>

    Qt includes are separated by extra newline



src/PowerManager.cpp
<http://git.reviewboard.kde.org/r/109846/#comment22649>

    Coding style, correct: EngineController *engine



src/PowerManager.cpp
<http://git.reviewboard.kde.org/r/109846/#comment22644>

    I would call this slotResumingFromSuspend(), the original name doesn't suggest when the slot is called.



src/PowerManager.cpp
<http://git.reviewboard.kde.org/r/109846/#comment22642>

    I think you should call stopSuppressingSleep() if m_cookieVal is valid here.



src/PowerManager.cpp
<http://git.reviewboard.kde.org/r/109846/#comment22647>

    No need to include .moc files now, the buildsystem is intellingent enough.



src/configdialog/dialogs/PlaybackConfig.ui
<http://git.reviewboard.kde.org/r/109846/#comment22650>

    I wonder why you need QGridLayout, wouldn't a classic QVBoxLayout suffice?



src/configdialog/dialogs/PlaybackConfig.ui
<http://git.reviewboard.kde.org/r/109846/#comment22651>

    I think KConfig reads the values on show(), no need to specify them (please reset to default in Qt Designer)



src/configdialog/dialogs/PlaybackConfig.ui
<http://git.reviewboard.kde.org/r/109846/#comment22652>

    Ditto please reset to default in Qt Designer.


- Matěj Laitl


On April 4, 2013, 4:03 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109846/
> -----------------------------------------------------------
> 
> (Updated April 4, 2013, 4:03 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> 1. Added "Suspend Options" to the PlaybackConfig Dialog, with two options:
>      Pause playback on suspend
>      Inhibit suspend if playing
> 2. Created a PowerManager class to handle suspend behavior
> 
> Fixes:
> BR 259862 - Amarok does not inhibit suspend while playing
> BR 222571 - Amarok pause on suspend
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 990f313 
>   src/EngineController.cpp 52bfd90 
>   src/PowerManager.h PRE-CREATION 
>   src/PowerManager.cpp PRE-CREATION 
>   src/amarokconfig.kcfg fbe5497 
>   src/configdialog/dialogs/PlaybackConfig.ui 3a79e43 
> 
> Diff: http://git.reviewboard.kde.org/r/109846/diff/
> 
> 
> Testing
> -------
> 
> Works as expected
> 
> 
> File Attachments
> ----------------
> 
> PlaybackConfig Dialog
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/04/03/snapshot11.png
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

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