Review Request 126650: [WIP] Add PM/ScreenSaver Inhibition capabilities to KIdleTime

Wed Jan 6 14:59:02 UTC 2016

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160106/1586606b/attachment-0001.html>