[Kde-hardware-devel] Review Request: Fix battery full notification not being emitted and make it watch for ChargeState rather than a charge percentage

[Dario Freddi]

> On Oct. 2, 2012, 3:14 p.m., Dario Freddi wrote:
> > powerdevil/daemon/powerdevilcore.cpp, lines 589-611
> > <http://git.reviewboard.kde.org/r/106686/diff/2/?file=88099#file88099line589>
> >
> >     You don't need that much logic - when batteryChargeStateChanged is emitted it is guaranteed the state is not the same as it was before. So you could simply fire the notification if the state went to NoCharge and AC is plugged. Test this solution - if you get false positives we might add more checks, but I am pretty sure the AC plugged check is more than enough for making it reliable.
> 
> Kai Uwe Broulik wrote:
>     I tried it again and it seems that (m_backend->acAdapterState() == BackendInterface::Plugged) is always false when this slot is triggered. Without it my new logic works fine.

Indeed, I am not saying your logic is wrong (apart from the comment below) - it's simply not needed, as the checks you are doing are already implied in the emission of the slot. It really surprises me it works though, the problem I outlined below should prevent it from working in any situation in theory, or maybe I completely overlooked something.


- Dario


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


On Oct. 2, 2012, 11:21 a.m., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106686/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2012, 11:21 a.m.)
> 
> 
> Review request for Solid and Dario Freddi.
> 
> 
> Description
> -------
> 
> This patch fixes battery notification not being emitted due to early return, and makes it watch for a change in ChargeState rather than the charge percentage which might not be reached at all.
> 
> 
> Diffs
> -----
> 
>   powerdevil/daemon/powerdevilcore.h 12e4b2c 
>   powerdevil/daemon/powerdevilcore.cpp b968d21 
> 
> Diff: http://git.reviewboard.kde.org/r/106686/diff/
> 
> 
> Testing
> -------
> 
> Compiles.
> Did not test if notification is properly emited, will do. Just wanted to know if this is the right approach.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20121002/65b2de3a/attachment.html>