----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126650/#review90697 -----------------------------------------------------------
Nice! I'm wondering how could add additional platform support to this API later. autotests/fakePMServer.cpp (line 23) <https://git.reviewboard.kde.org/r/126650/#comment61980> PowerDevil never returns the same cookie more than once, ie. it's just ++m_cookieId; autotests/inhibition_test.cpp (line 60) <https://git.reviewboard.kde.org/r/126650/#comment61981> Does this test work in a normal Plasma session? There's already PowerDevil claiming those services autotests/inhibition_test.cpp (line 89) <https://git.reviewboard.kde.org/r/126650/#comment61982> Perhaps we should split the enum Requested into Activating and Deactivating, or at least "Busy"/BeingProcessed or so, Requsted for me implies an "activation requested" autotests/inhibition_test.cpp (line 134) <https://git.reviewboard.kde.org/r/126650/#comment61983> Please also add a "delete i;" test to verify that automatic unregistration on deletion also works src/inhibition.h (line 31) <https://git.reviewboard.kde.org/r/126650/#comment61986> I think we should reserve 0x00 as "no type" or similar src/inhibition.h (lines 32 - 34) <https://git.reviewboard.kde.org/r/126650/#comment61985> Please use proper doxygen comments for describing enum values: InhibitScreen = 0x01, ///< Prevents screen turning off, ... src/inhibition.h (line 33) <https://git.reviewboard.kde.org/r/126650/#comment61997> I think they should build upon each other Screen = Suspend | 0x02, PM = Suspend | Screen | 0x04, All = Suspend | Screen | PM src/inhibition.h (line 49) <https://git.reviewboard.kde.org/r/126650/#comment61989> Or just comment here that "this destroys the object and releases the inhibition" src/inhibition.h (line 54) <https://git.reviewboard.kde.org/r/126650/#comment61987> This is standard QObject behavior but I would still mention that when the object is destroyed, the inhibition will be released src/inhibition.h (line 66) <https://git.reviewboard.kde.org/r/126650/#comment61988> but deleting it will still release the inhibition, right? src/inhibition.h (lines 105 - 106) <https://git.reviewboard.kde.org/r/126650/#comment61990> Then you'll have two inhibitions, one of which will no longer be managed by this object. You need to deactivate first. src/inhibition.h (line 122) <https://git.reviewboard.kde.org/r/126650/#comment61991> Do we use Q_NULLPTR in Frameworks? src/inhibition.cpp (lines 38 - 40) <https://git.reviewboard.kde.org/r/126650/#comment61992> initializer list? Private() : status(Inhibition::Inactive) , ... Also, you're leaving most member variables uninitialized src/inhibition.cpp (line 83) <https://git.reviewboard.kde.org/r/126650/#comment61993> This blocks, doesn't it? src/inhibition.cpp (line 144) <https://git.reviewboard.kde.org/r/126650/#comment61994> If I didn't change anything but status was Revoked or Failed I would not be able to activate it again? src/inhibition.cpp (line 156) <https://git.reviewboard.kde.org/r/126650/#comment61995> "reason" is not a good name for this src/inhibition.cpp (line 160) <https://git.reviewboard.kde.org/r/126650/#comment61996> You would need to set both states 1 ("InterruptSession") and 4 ("ChangeScreenSettings") which would be 5 in the end. I think there's no point in allowing to keep the screen on but then have the system auto suspend after a while. src/inhibition.cpp (line 186) <https://git.reviewboard.kde.org/r/126650/#comment61998> What about PM? See above, if they were cumulative this check would be fine. Perhaps we need a way to communicate to the user whether fine-grained types control is supported or not, so I could refrain from posting an inhibition if I know it'll be overkill for the situation I want, or so. src/inhibition.cpp (line 211) <https://git.reviewboard.kde.org/r/126650/#comment61999> Oh, so this is a separate call? Then we should probably also split this in the API, so we have a "keep screen on" and "lock screen" type? Martin G? However if I watch a movie I want neither the screen to turn off nor the screen to lock. src/inhibition.cpp (line 228) <https://git.reviewboard.kde.org/r/126650/#comment62000> Depending on which call (the PM or ScreenSaver) call returns later this status will be returned by the API. src/inhibition.cpp (line 238) <https://git.reviewboard.kde.org/r/126650/#comment62001> We need to handle the case of "activate but then the object goes away", we will have a spurious inhibition in that case. PowerDevil tracks when an application closes but it obviously cannot detect this case. src/inhibition.cpp (line 274) <https://git.reviewboard.kde.org/r/126650/#comment62002> perhaps an error signal and have the status enum only reflect active/inactive - Kai Uwe Broulik On Jan. 6, 2016, 7:17 vorm., Martin Klapetek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126650/ > ----------------------------------------------------------- > > (Updated Jan. 6, 2016, 7:17 vorm.) > > > Review request for KDE Frameworks and Kai Uwe Broulik. > > > Repository: kidletime > > > Description > ------- > > This is a work-in-progress, but I'm putting it up for a feedback now > as most people are gone for the day when I wake up :) > > --- > > After some discussion in https://git.reviewboard.kde.org/r/126627/ > and then with Kai Uwe Broulik, I've added a PM/ScreenSaver inhibition > capabilities to KIdleItem. > > We decided with Kai that we want simple stuff, encapsulated away for > the client as much as possible. So the Inhibition class has a static > "constructors" which make the usage as easy as follows: > > KIdleTime::Inhibition::createInhibition(KIdleTime::Inhibition::InhibitScreen, > QStringLiteral("Playing Movie"), mainWindow); > > That call would return an Inhibition* object which has methods to set > the inhibition type and reason and to activate/deactivate the inhibition. > The static method above would activate() on the Inhibition right away; > this allows a simple fire-and-forget usage. The Inhibition object can > be parented to any other object; the inhibition will be deactivated when > the Inhibition object is destroyed. The user can also keep the pointer > around and call deactivate() whenever actually needed. > > The inhibition itself first looks for Solid and if present, it uses the > Solid interface. If not present, it goes for the FDO interfaces. > > It comes with an autotest. > > > Diffs > ----- > > autotests/qtest_dbus.h PRE-CREATION > src/CMakeLists.txt 23e5e29 > autotests/inhibition_test.cpp PRE-CREATION > autotests/CMakeLists.txt PRE-CREATION > autotests/fakePMServer.h PRE-CREATION > src/inhibition.cpp PRE-CREATION > src/inhibition.h PRE-CREATION > autotests/fakePMServer.cpp PRE-CREATION > CMakeLists.txt ed5dc0c > > Diff: https://git.reviewboard.kde.org/r/126650/diff/ > > > Testing > ------- > > Everything works as expected, plus there's an autotest. > > > Thanks, > > Martin Klapetek > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel