[Kde-hardware-devel] Review Request 108407: systemd-login1 support for PowerDevil

[Lukáš Tinkl]

> On Jan. 14, 2013, 1:22 p.m., Dario Freddi wrote:
> > powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp, line 152
> > <http://git.reviewboard.kde.org/r/108407/diff/1/?file=107187#file107187line152>
> >
> >     Wow. Is it really a "no" and not a boolean from the remote interface? Can we submit a RFC to upstream systemd here? Or am I missing something?

Yup, it's a string with possible values "yes", "no" and "challenge" (the last being an interactive polkit based dialog)


> On Jan. 14, 2013, 1:22 p.m., Dario Freddi wrote:
> > powerdevil/daemon/backends/upower/login1suspendjob.cpp, lines 64-72
> > <http://git.reviewboard.kde.org/r/108407/diff/1/?file=107185#file107185line64>
> >
> >     I don't really like the blocking part here. I'd rather see the async call connecting to a slot emitting the result... lots of bad things might happen otherwise. Moreover, there's no UI thread in the daemon, so the WithGui wouldn't make sense anyway

Umm, does it matter at all? The system is going to sleep anyway :)


> On Jan. 14, 2013, 1:22 p.m., Dario Freddi wrote:
> > powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp, lines 44-68
> > <http://git.reviewboard.kde.org/r/108407/diff/1/?file=107187#file107187line44>
> >
> >     Especially, once we get rid of m_login1Present, we can IMHO get rid of all of this with a simple "if (!serviceRegistered) activate();". If the service is not activable the call would simply fail. Note that this already happens in init(), line 83, making this whole portion of code redundant.

Right, gonna fix this


> On Jan. 14, 2013, 1:22 p.m., Dario Freddi wrote:
> > powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp, line 120
> > <http://git.reviewboard.kde.org/r/108407/diff/1/?file=107187#file107187line120>
> >
> >     Moreover, here the interface is created regardless of its existence - not good imho.

Yup...


- Lukáš


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108407/#review25443
-----------------------------------------------------------


On Jan. 14, 2013, 1:12 p.m., Lukáš Tinkl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108407/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2013, 1:12 p.m.)
> 
> 
> Review request for Solid and Dario Freddi.
> 
> 
> Description
> -------
> 
> This patch adds support for systemd-login1 service to Powerdevil's upower backend. The main purpose is that UPower will be soon dropping support[1] for suspend/resume features so we have to rely on systemd. With this login1, we are also gaining support for HybridSleep, where implemented by the system.
> 
> One caveat: the current login1 implementation doesn't support[2] emitting the "resume from suspend" signal
> 
> [1] http://lists.freedesktop.org/archives/devkit-devel/2013-January/001339.html
> [2] http://www.freedesktop.org/wiki/Software/systemd/inhibit
> 
> 
> This addresses bug https://bugzilla.redhat.com/show_bug.cgi?id=859227.
>     http://bugs.kde.org/show_bug.cgi?id=https://bugzilla.redhat.com/show_bug.cgi?id=859227
> 
> 
> Diffs
> -----
> 
>   powerdevil/daemon/BackendConfig.cmake 5dbe6f6 
>   powerdevil/daemon/backends/upower/login1suspendjob.h PRE-CREATION 
>   powerdevil/daemon/backends/upower/login1suspendjob.cpp PRE-CREATION 
>   powerdevil/daemon/backends/upower/powerdevilupowerbackend.h ba942bd 
>   powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp 97a409b 
>   powerdevil/daemon/backends/upower/upowersuspendjob.h bbe2f45 
>   powerdevil/daemon/backends/upower/upowersuspendjob.cpp fa64ab0 
> 
> Diff: http://git.reviewboard.kde.org/r/108407/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Lukáš Tinkl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20130114/9c03a7c9/attachment-0001.html>