bug#64344: [PATCH] Make calculator work with customized mode-line

bug#64344: [PATCH] Make calculator work with customized mode-line

[john muhl via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

calculator.el doesn’t work after customizing the mode-line. It assumes
the line-width is always an integer but after using customize it is a
dotted list.

  - emacs -Q
  - M-x customize-apropos-faces RET mode-line RET
  - Ensure ‘mode-line-face’ has a box property with vertical and
    horizontal widths. The default config includes these already.
  - Set for current session
  - M-x calculator RET
  => calculator: Wrong type argument: number-or-marker-p, (1 . -1)

In GNU Emacs 30.0.50 (build 6, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.8) of 2023-06-28 built on localhost
Repository revision: c5d6102313076b83526dc79bfb563621671fb70b
Repository branch: master
System Description: Fedora Linux 38 (Workstation Edition)

Configured using:
 'configure --with-native-compilation --with-pgtk'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM GTK3 ZLIB


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] * lisp/calculator.el (calculator-need-3-lines): Check type --]
[-- Type: text/x-patch, Size: 965 bytes --]

From 54cd9fe40d954231a6be822d9ea448902e12c60c Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Wed, 28 Jun 2023 12:58:27 -0500
Subject: [PATCH] * lisp/calculator.el (calculator-need-3-lines): Check type

Copyright-paperwork-exempt: yes
---
 lisp/calculator.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/calculator.el b/lisp/calculator.el
index bf2ac9b6215..6af6464d2f4 100644
--- a/lisp/calculator.el
+++ b/lisp/calculator.el
@@ -746,7 +746,7 @@ calculator-need-3-lines
              ;; use 3 lines
              (let* ((bx (face-attribute 'mode-line :box))
                     (lh (plist-get bx :line-width)))
-               (and bx (or (not lh) (> lh 0))))
+               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
              ;; if the mode line has an overline, use 3 lines
              (not (memq (face-attribute 'mode-line :overline)
                         '(nil unspecified)))))))
-- 
2.41.0


[-- Attachment #3.1: Type: text/plain, Size: 0 bytes --]



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

bug#64344: [PATCH] Make calculator work with customized mode-line

[Eli Zaretskii]

> Date: Wed, 28 Jun 2023 13:23:45 -0500
> From:  john muhl via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> calculator.el doesn’t work after customizing the mode-line. It assumes
> the line-width is always an integer but after using customize it is a
> dotted list.
> 
>   - emacs -Q
>   - M-x customize-apropos-faces RET mode-line RET
>   - Ensure ‘mode-line-face’ has a box property with vertical and
>     horizontal widths. The default config includes these already.
>   - Set for current session
>   - M-x calculator RET
>   => calculator: Wrong type argument: number-or-marker-p, (1 . -1)

Thanks.

> diff --git a/lisp/calculator.el b/lisp/calculator.el
> index bf2ac9b6215..6af6464d2f4 100644
> --- a/lisp/calculator.el
> +++ b/lisp/calculator.el
> @@ -746,7 +746,7 @@ calculator-need-3-lines
>               ;; use 3 lines
>               (let* ((bx (face-attribute 'mode-line :box))
>                      (lh (plist-get bx :line-width)))
> -               (and bx (or (not lh) (> lh 0))))
> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
                                               ^^^^^
Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
argument.

bug#64344: [PATCH] Make calculator work with customized mode-line

[Andreas Schwab]

On Jun 29 2023, Eli Zaretskii wrote:

>> diff --git a/lisp/calculator.el b/lisp/calculator.el
>> index bf2ac9b6215..6af6464d2f4 100644
>> --- a/lisp/calculator.el
>> +++ b/lisp/calculator.el
>> @@ -746,7 +746,7 @@ calculator-need-3-lines
>>               ;; use 3 lines
>>               (let* ((bx (face-attribute 'mode-line :box))
>>                      (lh (plist-get bx :line-width)))
>> -               (and bx (or (not lh) (> lh 0))))
>> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
>                                                ^^^^^
> Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
> argument.

lh cannot be nil here, but it wouldn't make a difference anyway, since
(cdr nil) returns nil.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#64344: [PATCH] Make calculator work with customized mode-line

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: john muhl <jm@pub.pink>,  64344@debbugs.gnu.org
> Date: Thu, 29 Jun 2023 10:22:02 +0200
> 
> On Jun 29 2023, Eli Zaretskii wrote:
> 
> >> -               (and bx (or (not lh) (> lh 0))))
> >> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
> >                                                ^^^^^
> > Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
> > argument.
> 
> lh cannot be nil here, but it wouldn't make a difference anyway, since
> (cdr nil) returns nil.

I prefer not to have, nor force others, to analyze code in order to
ensure it's correct.

bug#64344: [PATCH] Make calculator work with customized mode-line

[Andreas Schwab]

On Jun 29 2023, Eli Zaretskii wrote:

>> From: Andreas Schwab <schwab@suse.de>
>> Cc: john muhl <jm@pub.pink>,  64344@debbugs.gnu.org
>> Date: Thu, 29 Jun 2023 10:22:02 +0200
>> 
>> On Jun 29 2023, Eli Zaretskii wrote:
>> 
>> >> -               (and bx (or (not lh) (> lh 0))))
>> >> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
>> >                                                ^^^^^
>> > Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
>> > argument.
>> 
>> lh cannot be nil here, but it wouldn't make a difference anyway, since
>> (cdr nil) returns nil.
>
> I prefer not to have, nor force others, to analyze code in order to
> ensure it's correct.

Then you need to add a comment, not change correct code.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#64344: [PATCH] Make calculator work with customized mode-line

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: jm@pub.pink,  64344@debbugs.gnu.org
> Date: Thu, 29 Jun 2023 11:31:06 +0200
> 
> On Jun 29 2023, Eli Zaretskii wrote:
> 
> >> From: Andreas Schwab <schwab@suse.de>
> >> Cc: john muhl <jm@pub.pink>,  64344@debbugs.gnu.org
> >> Date: Thu, 29 Jun 2023 10:22:02 +0200
> >> 
> >> On Jun 29 2023, Eli Zaretskii wrote:
> >> 
> >> >> -               (and bx (or (not lh) (> lh 0))))
> >> >> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
> >> >                                                ^^^^^
> >> > Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
> >> > argument.
> >> 
> >> lh cannot be nil here, but it wouldn't make a difference anyway, since
> >> (cdr nil) returns nil.
> >
> > I prefer not to have, nor force others, to analyze code in order to
> > ensure it's correct.
> 
> Then you need to add a comment, not change correct code.

There's no reason whatsoever to comment correct code.

bug#64344: [PATCH] Make calculator work with customized mode-line

[Andreas Schwab]

On Jun 29 2023, Eli Zaretskii wrote:

>> From: Andreas Schwab <schwab@suse.de>
>> Cc: jm@pub.pink,  64344@debbugs.gnu.org
>> Date: Thu, 29 Jun 2023 11:31:06 +0200
>> 
>> On Jun 29 2023, Eli Zaretskii wrote:
>> 
>> >> From: Andreas Schwab <schwab@suse.de>
>> >> Cc: john muhl <jm@pub.pink>,  64344@debbugs.gnu.org
>> >> Date: Thu, 29 Jun 2023 10:22:02 +0200
>> >> 
>> >> On Jun 29 2023, Eli Zaretskii wrote:
>> >> 
>> >> >> -               (and bx (or (not lh) (> lh 0))))
>> >> >> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
>> >> >                                                ^^^^^
>> >> > Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
>> >> > argument.
>> >> 
>> >> lh cannot be nil here, but it wouldn't make a difference anyway, since
>> >> (cdr nil) returns nil.
>> >
>> > I prefer not to have, nor force others, to analyze code in order to
>> > ensure it's correct.
>> 
>> Then you need to add a comment, not change correct code.
>
> There's no reason whatsoever to comment correct code.

Ok, so nothing needs to change.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#64344: [PATCH] Make calculator work with customized mode-line

[Stephen Berman]

On Thu, 29 Jun 2023 10:49:38 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> Date: Wed, 28 Jun 2023 13:23:45 -0500
>> From:  john muhl via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> calculator.el doesn’t work after customizing the mode-line. It assumes
>> the line-width is always an integer but after using customize it is a
>> dotted list.
>> 
>>   - emacs -Q
>>   - M-x customize-apropos-faces RET mode-line RET
>>   - Ensure ‘mode-line-face’ has a box property with vertical and
>>     horizontal widths. The default config includes these already.
>>   - Set for current session
>>   - M-x calculator RET
>>   => calculator: Wrong type argument: number-or-marker-p, (1 . -1)
>
> Thanks.
>
>> diff --git a/lisp/calculator.el b/lisp/calculator.el
>> index bf2ac9b6215..6af6464d2f4 100644
>> --- a/lisp/calculator.el
>> +++ b/lisp/calculator.el
>> @@ -746,7 +746,7 @@ calculator-need-3-lines
>>               ;; use 3 lines
>>               (let* ((bx (face-attribute 'mode-line :box))
>>                      (lh (plist-get bx :line-width)))
>> -               (and bx (or (not lh) (> lh 0))))
>> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))

Why is the state of the mode-line face shown as "EDITED, shown value
does not take effect until you set or save it." before setting it in the
above recipe?  In fact, I see nine such faces in the buffer *Customize
Faces* with -Q (the state of all other faces is shown as "STANDARD").

Steve Berman

bug#64344: [PATCH] Make calculator work with customized mode-line

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: john muhl <jm@pub.pink>,  64344@debbugs.gnu.org
> Date: Thu, 29 Jun 2023 10:46:11 +0200
> 
> Why is the state of the mode-line face shown as "EDITED, shown value
> does not take effect until you set or save it." before setting it in the
> above recipe?  In fact, I see nine such faces in the buffer *Customize
> Faces* with -Q (the state of all other faces is shown as "STANDARD").

Please submit a separate bug report about this, as it's unrelated to
the issue at hand here.

Thanks.

bug#64344: [PATCH] Make calculator work with customized mode-line

[Stephen Berman]

On Thu, 29 Jun 2023 12:26:00 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: john muhl <jm@pub.pink>,  64344@debbugs.gnu.org
>> Date: Thu, 29 Jun 2023 10:46:11 +0200
>>
>> Why is the state of the mode-line face shown as "EDITED, shown value
>> does not take effect until you set or save it." before setting it in the
>> above recipe?  In fact, I see nine such faces in the buffer *Customize
>> Faces* with -Q (the state of all other faces is shown as "STANDARD").
>
> Please submit a separate bug report about this, as it's unrelated to
> the issue at hand here.
>
> Thanks.

Done, bug#64347.

Steve Berman

bug#64344: [PATCH] Make calculator work with customized mode-line

[john muhl via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> +               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
>                                                ^^^^^
> Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
> argument.

I’m not expert enough to say more than that either way works to fix the
issue. Originally I did the ‘(cdr lh)’ unconditionally and got an error
mentioning ‘listp’ so I just used that when I added the condition; if it
had said ‘conps’ I would have used that.

Here is the patch with ‘consp’ instead.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] * lisp/calculator.el (calculator-need-3-lines): Check type --]
[-- Type: text/x-patch, Size: 965 bytes --]

From 36ea9a67c9e559c490f2c5787a75b229772a97e4 Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Wed, 28 Jun 2023 12:58:27 -0500
Subject: [PATCH] * lisp/calculator.el (calculator-need-3-lines): Check type

Copyright-paperwork-exempt: yes
---
 lisp/calculator.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/calculator.el b/lisp/calculator.el
index bf2ac9b6215..6af6464d2f4 100644
--- a/lisp/calculator.el
+++ b/lisp/calculator.el
@@ -746,7 +746,7 @@ calculator-need-3-lines
              ;; use 3 lines
              (let* ((bx (face-attribute 'mode-line :box))
                     (lh (plist-get bx :line-width)))
-               (and bx (or (not lh) (> lh 0))))
+               (and bx (or (not lh) (> (if (listp lh) (cdr lh) lh) 0))))
              ;; if the mode line has an overline, use 3 lines
              (not (memq (face-attribute 'mode-line :overline)
                         '(nil unspecified)))))))
-- 
2.41.0

bug#64344: [PATCH] Make calculator work with customized mode-line

[Eli Zaretskii]

> From: john muhl <jm@pub.pink>
> Cc: 64344@debbugs.gnu.org
> Date: Thu, 29 Jun 2023 10:12:06 -0500
> 
> > Shouldn't that be 'consp' instead?  'listp' returns non-nil for nil
> > argument.
> 
> I’m not expert enough to say more than that either way works to fix the
> issue. Originally I did the ‘(cdr lh)’ unconditionally and got an error
> mentioning ‘listp’ so I just used that when I added the condition; if it
> had said ‘conps’ I would have used that.
> 
> Here is the patch with ‘consp’ instead.

Thanks.  That still uses 'listp', so I fixed that manually.

This is now installed on the emacs-29 branch; closing the bug.