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

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



powerdevil/daemon/powerdevilcore.cpp
<http://git.reviewboard.kde.org/r/106686/#comment15727>

    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.



powerdevil/daemon/powerdevilcore.cpp
<http://git.reviewboard.kde.org/r/106686/#comment15728>

    Just because you asked - the notification is never triggered because when previousCharged is false, currentCharged will never be true :)
    
    Maybe you meant:
    
    } else {
    m_batteriesCharged[udi] = true;
    currentCharged = true;
    }
    
    However, please follow the approach I mentioned in the other comment.


- Dario Freddi


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/f73cae3f/attachment-0001.html>