----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
