bug#18157: 24.4.50; battery-pmset fails to report critical battery state

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Sebastian Wiesner]

`battery-pmset' fails to correctly report critical battery state:  Even
if the battery charge level is below `battery-load-critical', it reports
"low" battery state only.

The culprit is the `cond' expression in the body of `battery-pmset'
which compares the reported `load-percentage' against `battery-load-low'
first, and then against `battery-load-critical'.

Since the latter is typically lower, it will never be reached, because
`cond' already returns after the former succeeded.

To fix this issue, `batter-pmset' needs to check `battery-load-critical'
*first*, and then `battery-load-low'.

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Marcin Borkowski]

On 2014-07-31, at 16:16, Sebastian Wiesner <swiesner@lunaryorn.com> wrote:

> `battery-pmset' fails to correctly report critical battery state:  Even
> if the battery charge level is below `battery-load-critical', it reports
> "low" battery state only.
>
> The culprit is the `cond' expression in the body of `battery-pmset'
> which compares the reported `load-percentage' against `battery-load-low'
> first, and then against `battery-load-critical'.
>
> Since the latter is typically lower, it will never be reached, because
> `cond' already returns after the former succeeded.
>
> To fix this issue, `batter-pmset' needs to check `battery-load-critical'
> *first*, and then `battery-load-low'.

Does this patch help?

--8<---------------cut here---------------start------------->8---
diff -u --label /usr/local/share/emacs/25.1.50/lisp/battery.el.gz --label \#\<buffer\ battery.el.gz\> /tmp/jka-com1889YvM /tmp/buffer-content-1889yDZ
--- /usr/local/share/emacs/25.1.50/lisp/battery.el.gz
+++ #<buffer battery.el.gz>
@@ -628,12 +628,12 @@
 	    (cond ((looking-at "; charging")
 		   (setq battery-status "charging"
 			 battery-status-symbol "+"))
-		  ((< (string-to-number load-percentage) battery-load-low)
-		   (setq battery-status "low"
-			 battery-status-symbol "-"))
 		  ((< (string-to-number load-percentage) battery-load-critical)
 		   (setq battery-status "critical"
 			 battery-status-symbol "!"))
+		  ((< (string-to-number load-percentage) battery-load-low)
+		   (setq battery-status "low"
+			 battery-status-symbol "-"))
 		  (t
 		   (setq battery-status "high"
 			 battery-status-symbol "")))

Diff finished.  Tue Feb 16 15:56:03 2016
--8<---------------cut here---------------end--------------->8---

Best,

--
mb

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Sebastian Wiesner]

> Am 16.02.2016 um 15:56 schrieb Marcin Borkowski <mbork@mbork.pl>:
> 
> On 2014-07-31, at 16:16, Sebastian Wiesner <swiesner@lunaryorn.com> wrote:
> 
>> `battery-pmset' fails to correctly report critical battery state:  Even
>> if the battery charge level is below `battery-load-critical', it reports
>> "low" battery state only.
>> 
>> The culprit is the `cond' expression in the body of `battery-pmset'
>> which compares the reported `load-percentage' against `battery-load-low'
>> first, and then against `battery-load-critical'.
>> 
>> Since the latter is typically lower, it will never be reached, because
>> `cond' already returns after the former succeeded.
>> 
>> To fix this issue, `batter-pmset' needs to check `battery-load-critical'
>> *first*, and then `battery-load-low'.
> 
> Does this patch help?

I'm sorry, are you asking me?  It would be bold to believe that I still cared for this issue now, almost *two years* after I reported it.  

At a cursory look the fix surely looks correct, but I hope you understand that Emacs taking two years for what seems to be a trivial issue to fix does not particularly contribute to my enthusiasm, nor does it motivate me to test this patch.

> --8<---------------cut here---------------start------------->8---
> diff -u --label /usr/local/share/emacs/25.1.50/lisp/battery.el.gz --label \#\<buffer\ battery.el.gz\> /tmp/jka-com1889YvM /tmp/buffer-content-1889yDZ
> --- /usr/local/share/emacs/25.1.50/lisp/battery.el.gz
> +++ #<buffer battery.el.gz>
> @@ -628,12 +628,12 @@
> 	    (cond ((looking-at "; charging")
> 		   (setq battery-status "charging"
> 			 battery-status-symbol "+"))
> -		  ((< (string-to-number load-percentage) battery-load-low)
> -		   (setq battery-status "low"
> -			 battery-status-symbol "-"))
> 		  ((< (string-to-number load-percentage) battery-load-critical)
> 		   (setq battery-status "critical"
> 			 battery-status-symbol "!"))
> +		  ((< (string-to-number load-percentage) battery-load-low)
> +		   (setq battery-status "low"
> +			 battery-status-symbol "-"))
> 		  (t
> 		   (setq battery-status "high"
> 			 battery-status-symbol "")))
> 
> Diff finished.  Tue Feb 16 15:56:03 2016
> --8<---------------cut here---------------end--------------->8---
> 
> Best,
> 
> --
> mb

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Eli Zaretskii]

> From: Sebastian Wiesner <swiesner@lunaryorn.com>
> Date: Tue, 16 Feb 2016 19:41:25 +0100
> Cc: 18157@debbugs.gnu.org
> 
> I hope you understand that Emacs taking two years for what seems to be a trivial issue to fix does not particularly contribute to my enthusiasm, nor does it motivate me to test this patch.

FWIW, I'm very grateful to Marcin and a few people like him for
investing their time into working on this and other bug reports, and
hope that more contributors like them will come on board and help in
this effort.

Thanks a lot, guys!

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Marcin Borkowski]

On 2016-02-16, at 19:41, Sebastian Wiesner <swiesner@lunaryorn.com> wrote:

>> Does this patch help?
>
> I'm sorry, are you asking me?  It would be bold to believe that I still cared for this issue now, almost *two years* after I reported it.  
>
> At a cursory look the fix surely looks correct, but I hope you understand that Emacs taking two years for what seems to be a trivial issue to fix does not particularly contribute to my enthusiasm, nor does it motivate me to test this patch.

I am also sorry if I sounded rude.  The question was directed to people
reading the bug-gnu-emacs mailing list as well as to you.  I am also
sorry that this was not fixed earlier, but I am bold enough to believe
that it is better to try to investigate it now than to leave it forever.

I am also sorry that I didn't test this myself.  However, I do not own
a Mac, so I could only transcribe your report into a (trivial,
admittedly) patch for others to test.  I do not consider my input here
too valuable (everyone could write this patch, even without any deeper
knowledge about Elisp, thanks to your detailed report and suggestion as
to were the correct fix was - thank you for that!), but I hoped that
what I may have /really/ fixed was the problem of your report not
gaining enough attention when it was sent.

Also, even if you don't care for this issue anymore, there may be other
people who do.  And you were the author of the report, so it was
a natural thing to send this message to you.  (In fact, this is what the
debbugs tool did, as it does by default when responding to bug reports.)

Also, if I should have behaved differently, please tell me what I could
have done better (apart from the not-so-optimal wording of my message).

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> FWIW, I'm very grateful to Marcin and a few people like him for investing
> their time into working on this and other bug reports, and hope that more
> contributors like them will come on board and help in this effort.
> 
> Thanks a lot, guys!

I would like to second that thanks. We move at the speed of our volunteers.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 625 bytes --]

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Lars Ingebrigtsen]

Marcin Borkowski <mbork@mbork.pl> writes:

> Does this patch help?
>
> diff -u --label /usr/local/share/emacs/25.1.50/lisp/battery.el.gz --label \#\<buffer\ battery.el.gz\> /tmp/jka-com1889YvM /tmp/buffer-content-1889yDZ
> --- /usr/local/share/emacs/25.1.50/lisp/battery.el.gz
> +++ #<buffer battery.el.gz>
> @@ -628,12 +628,12 @@
>  	    (cond ((looking-at "; charging")
>  		   (setq battery-status "charging"
>  			 battery-status-symbol "+"))
> -		  ((< (string-to-number load-percentage) battery-load-low)
> -		   (setq battery-status "low"
> -			 battery-status-symbol "-"))
>  		  ((< (string-to-number load-percentage) battery-load-critical)
>  		   (setq battery-status "critical"
>  			 battery-status-symbol "!"))
> +		  ((< (string-to-number load-percentage) battery-load-low)
> +		   (setq battery-status "low"
> +			 battery-status-symbol "-"))
>  		  (t
>  		   (setq battery-status "high"
>  			 battery-status-symbol "")))

Looks logical to me.  I've applied it to emacs-25.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18157: 24.4.50; battery-pmset fails to report critical battery state

[Lars Ingebrigtsen]

Marcin Borkowski <mbork@mbork.pl> writes:

> Also, if I should have behaved differently, please tell me what I could
> have done better (apart from the not-so-optimal wording of my message).

No, you've done exactly what we hope people would do.  Thank you for
investigating the problem and providing a patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no