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

[Harald Sitter]

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



FTR org.freedesktop.PowerManagement is deprecated 
https://www.freedesktop.org/wiki/Specifications/power-management-spec/ 
supposedly in favor of 
https://www.freedesktop.org/wiki/Software/systemd/inhibit/ which apparently is 
being ignored to a degree by powerdevil.

- Harald Sitter


On Jan. 26, 2016, 6:13 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126650/
> ---
> 
> (Updated Jan. 26, 2016, 6:13 p.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
> -
> 
>   CMakeLists.txt ed5dc0c 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fakePMServer.h PRE-CREATION 
>   autotests/fakePMServer.cpp PRE-CREATION 
>   autotests/inhibition_test.cpp PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/CMakeLists.txt 23e5e29 
>   src/inhibition.h PRE-CREATION 
>   src/inhibition.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126650/diff/
> 
> 
> Testing
> ---
> 
> Everything works as expected, plus there's an autotest.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

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

[Aleix Pol Gonzalez]

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



Are we convinced that KIdleTime is the best place to put this?

KIdleTime definition is:
> KIdleTime is a singleton reporting information on idle time. It is useful not 
> only for finding out about the current idle time of the PC, but also for 
> getting notified upon idle time events, such as custom timeouts, or user 
> activity.

Which I'm not sure is the same.

Furthermore, KIdleTime is supposed to be crossplatform.


src/inhibition.cpp (line 38)


move all as initializers?



src/inhibition.cpp (line 61)


A bunch of these can be marked as const.



src/inhibition.cpp (line 94)


If this has to be cross-platform, dbus usage should be abstracted out. On 
Windows/OS X one won't find a dbus interface with this information.


- Aleix Pol Gonzalez


On Jan. 26, 2016, 7:13 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126650/
> ---
> 
> (Updated Jan. 26, 2016, 7:13 p.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
> -
> 
>   CMakeLists.txt ed5dc0c 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fakePMServer.h PRE-CREATION 
>   autotests/fakePMServer.cpp PRE-CREATION 
>   autotests/inhibition_test.cpp PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/CMakeLists.txt 23e5e29 
>   src/inhibition.h PRE-CREATION 
>   src/inhibition.cpp PRE-CREATION 
> 
> 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

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

[Martin Klapetek]

> On Feb. 10, 2016, 4:19 p.m., Aleix Pol Gonzalez wrote:
> > Are we convinced that KIdleTime is the best place to put this?
> > 
> > KIdleTime definition is:
> > > KIdleTime is a singleton reporting information on idle time. It is useful 
> > > not only for finding out about the current idle time of the PC, but also 
> > > for getting notified upon idle time events, such as custom timeouts, or 
> > > user activity.
> > 
> > Which I'm not sure is the same.
> > 
> > Furthermore, KIdleTime is supposed to be crossplatform.

> KIdleTime definition is:

Nowhere it says that the definition cannot be updated though. Disabling what 
happens when user is idle is directly related to "idle time detection".


- Martin


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


On Jan. 26, 2016, 7:13 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126650/
> ---
> 
> (Updated Jan. 26, 2016, 7:13 p.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
> -
> 
>   CMakeLists.txt ed5dc0c 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fakePMServer.h PRE-CREATION 
>   autotests/fakePMServer.cpp PRE-CREATION 
>   autotests/inhibition_test.cpp PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/CMakeLists.txt 23e5e29 
>   src/inhibition.h PRE-CREATION 
>   src/inhibition.cpp PRE-CREATION 
> 
> 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

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

[Martin Gräßlin]

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




src/inhibition.h (line 136)


QScopedPointer?



src/inhibition.cpp (line 96)


please use categorized logging



src/inhibition.cpp (line 101)


I'm sure you are aware of it: this is blocking

A non blocking variant is:

QDBusMessage message = 
QDBusMessage::createMethodCall(QStringLiteral("org.freedesktop.DBus"),
  
QStringLiteral("/"),
  
QStringLiteral("org.freedesktop.DBus"),
  
QStringLiteral("ListNames"));
QDBusPendingReply async = 
QDBusConnection::sessionBus().asyncCall(message);
QDBusPendingCallWatcher *callWatcher = new 
QDBusPendingCallWatcher(async, this);
connect(callWatcher, ::finished, this,
[this](QDBusPendingCallWatcher *self) {
QDBusPendingReply reply = *self;
self->deleteLater();
if (!reply.isValid()) {
return;
}
if (reply.value().contains(QStringLiteral("ord.kde.Solid..."))) 
{
// Whatever needs to be done
}
}
);

What could be considered is also adding a service watcher to check whether 
it becomes available later on. But I'd say that's outside the scope of this 
class.


- Martin Gräßlin


On Jan. 26, 2016, 7:13 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126650/
> ---
> 
> (Updated Jan. 26, 2016, 7:13 p.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
> -
> 
>   CMakeLists.txt ed5dc0c 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fakePMServer.h PRE-CREATION 
>   autotests/fakePMServer.cpp PRE-CREATION 
>   autotests/inhibition_test.cpp PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/CMakeLists.txt 23e5e29 
>   src/inhibition.h PRE-CREATION 
>   src/inhibition.cpp PRE-CREATION 
> 
> 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

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

[Martin Klapetek]

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

(Updated Jan. 26, 2016, 7:13 p.m.)


Review request for KDE Frameworks and Kai Uwe Broulik.


Changes
---

* significantly more tests
* now returns QSharedPointer instead of raw pointer
* improved comments
* fixed most issues; still the sync dbus call one left
* it now builds when UNIX and NOT APPLE instead of X11 only


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 (updated)
-

  CMakeLists.txt ed5dc0c 
  autotests/CMakeLists.txt PRE-CREATION 
  autotests/fakePMServer.h PRE-CREATION 
  autotests/fakePMServer.cpp PRE-CREATION 
  autotests/inhibition_test.cpp PRE-CREATION 
  autotests/qtest_dbus.h PRE-CREATION 
  src/CMakeLists.txt 23e5e29 
  src/inhibition.h PRE-CREATION 
  src/inhibition.cpp PRE-CREATION 

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

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

[Kai Uwe Broulik]

> On Jan. 6, 2016, 2:59 nachm., Kai Uwe Broulik wrote:
> > src/inhibition.cpp, lines 38-40
> > 
> >
> > initializer list?
> > 
> > Private()
> > : status(Inhibition::Inactive)
> > , ...
> > 
> > Also, you're leaving most member variables uninitialized
> 
> Martin Klapetek wrote:
> Is there any actual difference (performance or something) between setting 
> the values in ctor and setting them before ctor?

Depends on the type, iirc for non-integral types they'll be default-constructed 
just to immediately get assigned a proper object in the constructor's body.


- Kai Uwe


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


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

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

[Martin Gräßlin]

> On Jan. 11, 2016, 1:05 p.m., Martin Gräßlin wrote:
> > src/inhibition.h, line 30
> > 
> >
> > Maybe an explicit InhibitLockScreen?
> 
> Martin Klapetek wrote:
> Can add. Btw. is there any dbus interface to inhibit only lockscreen? Or 
> does it use the screensaver one?

There is no such thing as a screensaver in modern DEs (at least Plasma, GNOME 
Shell and Unity). For historic reasons the interface is called screensaver, so 
screensaver is the one for lock screen.


> On Jan. 11, 2016, 1:05 p.m., Martin Gräßlin wrote:
> > src/inhibition.h, lines 32-34
> > 
> >
> > Please use lock screen instead of screensaver. These things don't exist 
> > anymore ;-)
> 
> Martin Klapetek wrote:
> xscreensaver? Also the interface is called fdo.ScreenSaver so I think 
> it's fitting?

As far as my quick internet research showed: xscreensaver does not support dbus 
interface (it would have surprised me). Given that: see what I wrote above: 
there is no such thing as a screensaver in a modern DE. Let's use lock screen 
and leave the legacy behind :-)


- Martin


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Martin Gräßlin]

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


I'm interested in the reasoning why to put it into KIdleTime? My first thought 
would have been Solid, so I'm interested in the reasoning.

- Martin Gräßlin


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Martin Gräßlin]

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


nice work!


src/CMakeLists.txt (line 27)


why bind it to X11?



src/inhibition.h (line 30)


Maybe an explicit InhibitLockScreen?



src/inhibition.h (lines 32 - 34)


Please use lock screen instead of screensaver. These things don't exist 
anymore ;-)



src/inhibition.h (line 60)


suggestion: return a smart pointer (e.g. std::unique or QSharedPointer). 
This would indicate in a better who is responsible for cleanup and how the 
Inhibition is released.


- Martin Gräßlin


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Martin Klapetek]

> On Jan. 11, 2016, 1:05 p.m., Martin Gräßlin wrote:
> > src/inhibition.h, lines 32-34
> > 
> >
> > Please use lock screen instead of screensaver. These things don't exist 
> > anymore ;-)

xscreensaver? Also the interface is called fdo.ScreenSaver so I think it's 
fitting?


> On Jan. 11, 2016, 1:05 p.m., Martin Gräßlin wrote:
> > src/inhibition.h, line 30
> > 
> >
> > Maybe an explicit InhibitLockScreen?

Can add. Btw. is there any dbus interface to inhibit only lockscreen? Or does 
it use the screensaver one?


> On Jan. 11, 2016, 1:05 p.m., Martin Gräßlin wrote:
> > src/CMakeLists.txt, line 27
> > 
> >
> > why bind it to X11?

Dunno, I had to put it somewhere for starters :)


- Martin


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


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

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

[Kai Uwe Broulik]

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


PowerDevil never returns the same cookie more than once, ie. it's just 
++m_cookieId;



autotests/inhibition_test.cpp (line 60)


Does this test work in a normal Plasma session? There's already PowerDevil 
claiming those services



autotests/inhibition_test.cpp (line 89)


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)


Please also add a "delete i;" test to verify that automatic unregistration 
on deletion also works



src/inhibition.h (line 31)


I think we should reserve 0x00 as "no type" or similar



src/inhibition.h (lines 32 - 34)


Please use proper doxygen comments for describing enum values:
InhibitScreen = 0x01, ///< Prevents screen turning off, ...



src/inhibition.h (line 33)


I think they should build upon each other

Screen = Suspend | 0x02,
PM = Suspend | Screen | 0x04,
All = Suspend | Screen | PM



src/inhibition.h (line 49)


Or just comment here that "this destroys the object and releases the 
inhibition"



src/inhibition.h (line 54)


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)


but deleting it will still release the inhibition, right?



src/inhibition.h (lines 105 - 106)


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)


Do we use Q_NULLPTR in Frameworks?



src/inhibition.cpp (lines 38 - 40)


initializer list?

Private()
: status(Inhibition::Inactive)
, ...

Also, you're leaving most member variables uninitialized



src/inhibition.cpp (line 83)


This blocks, doesn't it?



src/inhibition.cpp (line 144)


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)


"reason" is not a good name for this



src/inhibition.cpp (line 160)


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)


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)


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)


Depending on which call (the PM or ScreenSaver) call returns later this 
status will be returned by the API.



src/inhibition.cpp (line 238)


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.

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

[Martin Klapetek]

> On Jan. 6, 2016, 3:59 p.m., Kai Uwe Broulik wrote:
> > autotests/fakePMServer.cpp, line 23
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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:
> > 

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

[Martin Klapetek]

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

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