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

Wed Jan 6 20:50:10 UTC 2016

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/fakePMServer.cpp, line 23
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428568#file428568line23>
> >
> >     PowerDevil never returns the same cookie more than once, ie. it's just ++m_cookieId;

Well I don't think that's an actual problem here, it's a mock/fake server and the client does not care *what* cookie data it will receive.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/inhibition_test.cpp, line 60
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428569#file428569line60>
> >
> >     Does this test work in a normal Plasma session? There's already PowerDevil claiming those services

The qtest_dbus.h has a code that launces a separate dbus instance. So yeah this is fine.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/inhibition_test.cpp, line 89
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428569#file428569line89>
> >
> >     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"

RequestSent? WaitingForReply?

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/inhibition_test.cpp, line 134
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428569#file428569line134>
> >
> >     Please also add a "delete i;" test to verify that automatic unregistration on deletion also works

Yeah that's in the plan.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 31
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428572#file428572line31>
> >
> >     I think we should reserve 0x00 as "no type" or similar

I can sure start the flags from a higher value, but imo "NoType" makes no sense, I mean, requesting to inhibit "nothing"?

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 33
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428572#file428572line33>
> >
> >     I think they should build upon each other
> >     
> >     Screen = Suspend | 0x02,
> >     PM = Suspend | Screen | 0x04,
> >     All = Suspend | Screen | PM

Yeah they really should.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 54
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428572#file428572line54>
> >
> >     This is standard QObject behavior but I would still mention that when the object is destroyed, the inhibition will be released

It says that right there, no? "Parenting the inhibition to any object will deactivate ... when that object is destroyed"

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 66
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428572#file428572line66>
> >
> >     but deleting it will still release the inhibition, right?

If it was activated first, yes. I'll expand that comment.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, lines 105-106
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428572#file428572line105>
> >
> >     Then you'll have two inhibitions, one of which will no longer be managed by this object. You need to deactivate first.

Ah yes, I was hoping this could be done in an easier way but needs deactivate() first.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.h, line 122
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428572#file428572line122>
> >
> >     Do we use Q_NULLPTR in Frameworks?

Iii...think so?

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, lines 38-40
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line38>
> >
> >     initializer list?
> >     
> >     Private()
> >         : status(Inhibition::Inactive)
> >         , ...
> >         
> >     Also, you're leaving most member variables uninitialized

Is there any actual difference (performance or something) between setting the values in ctor and setting them before ctor?

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 83
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line83>
> >
> >     This blocks, doesn't it?

Yes, but I'm not sure if we actually don't want blocking here? If you use the big static method, it may get into activate() before the solid reply has returned. Alternatively I could make a kind of semaphore thingy and trigger activate() only after the solid reply has arrived.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 144
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line144>
> >
> >     If I didn't change anything but status was Revoked or Failed I would not be able to activate it again?

Good point, needs fixing.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 186
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line186>
> >
> >     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.

I don't think the user should care, imho the user should activate the inhibition and the code does the right thing(tm)

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 211
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line211>
> >
> >     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.

> we should probably also split this in the API

Yes, that's why you have different enums for it :P

Is lock screen inhibited by something else?

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 228
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line228>
> >
> >     Depending on which call (the PM or ScreenSaver) call returns later this status will be returned by the API.

Ah, good point. This needs to be treated as atomic.

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, line 238
> > <https://git.reviewboard.kde.org/r/126650/diff/1/?file=428573#file428573line238>
> >
> >     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.

Hmm...I think that would actually be an app/client bug. The scenairo I have in my mind goes as follows:

* client creates an inhibition
* client deletes the Inhibition* object before the cookie reply comes back

Now this is can either mean that the app has closed (handled by powerdevil), or the Inhibition* was deleted prematurely.

On the other hand, if the Inhibition* is parented to say a window that gets closed by the user fast enough, there needs to be a way to recover from that, but it cannot be handled by the Inhibition* itself as that is deleted at that time. Sooo maybe I should put back the singleton manager that we had in the initial plan...? I see no other way around?

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126650/#review90697
-----------------------------------------------------------

On Jan. 6, 2016, 8:17 a.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126650/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2016, 8:17 a.m.)
> 
> 
> 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/4bbe29d6/attachment-0001.html>