Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Eshel Yaron]

Hi Stefan,

Stefan Kangas <stefankangas@gmail.com> writes:

> branch: emacs-30
> commit 73c1252bb6b7cc61d9f992818568d3c57de4ff67
> Author: Stefan Kangas <stefankangas@gmail.com>
> Commit: Stefan Kangas <stefankangas@gmail.com>
>
>     Fix link to major mode variable in docstring
>     
>     * lisp/emacs-lisp/easy-mmode.el (easy-mmode--arg-docstring): Fix link to
>     major mode variable in docstring.  (Bug#71815)
> ---
>  lisp/emacs-lisp/easy-mmode.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
> index ba0f8bad393..12712cb2bc3 100644
> --- a/lisp/emacs-lisp/easy-mmode.el
> +++ b/lisp/emacs-lisp/easy-mmode.el
> @@ -91,7 +91,7 @@ Enable the mode if ARG is nil, omitted, or is a positive number.
>  Disable the mode if ARG is a negative number.
>  
>  To check whether the minor mode is enabled in the current buffer,
> -evaluate `%s'.
> +evaluate the variable `%s'.
>  
>  The mode's hook is called both when the mode is enabled and when
>  it is disabled.")

I think it's not always correct to say "the variable" here, because that
"%s" may be replaced with a form that's not just a variable.  E.g. now
C-h f global-auto-revert-mode RET says:

  To check whether the minor mode is enabled in the current buffer,
  evaluate the variable ‘(default-value 'global-auto-revert-mode)’.


Best,

Eshel

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Stefan Kangas]

Eshel Yaron <me@eshelyaron.com> writes:

> I think it's not always correct to say "the variable" here, because that
> "%s" may be replaced with a form that's not just a variable.  E.g. now
> C-h f global-auto-revert-mode RET says:
>
>   To check whether the minor mode is enabled in the current buffer,
>   evaluate the variable ‘(default-value 'global-auto-revert-mode)’.

Thanks, you're right.  Now reverted.

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Thierry Volpiatto]

Stefan Kangas <stefankangas@gmail.com> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>> I think it's not always correct to say "the variable" here, because that
>> "%s" may be replaced with a form that's not just a variable.  E.g. now
>> C-h f global-auto-revert-mode RET says:
>>
>>   To check whether the minor mode is enabled in the current buffer,
>>   evaluate the variable ‘(default-value 'global-auto-revert-mode)’.
>
> Thanks, you're right.  Now reverted.

No, this is not right, reverting fix nothing, we are just back to previous
bug, this is unrelated, the bug is in `easy-mmode--mode-docstring` which
have a misleading arg called 'getter' and naturally `define-minor-mode`
pass 'getter' to it instead of passing 'mode'.

PS: Please CC me as I am not suscribed to this list, thanks.

-- 
Thierry

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Michael Heerdegen via Emacs development discussions.]

Thierry Volpiatto <thievol@posteo.net> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
> > Eshel Yaron <me@eshelyaron.com> writes:
> >
> >> I think it's not always correct to say "the variable" here, because that
> >> "%s" may be replaced with a form that's not just a variable.  E.g. now
> >> C-h f global-auto-revert-mode RET says:
> >>
> >>   To check whether the minor mode is enabled in the current buffer,
> >>   evaluate the variable ‘(default-value 'global-auto-revert-mode)’.
> >
> > Thanks, you're right.  Now reverted.
>
> No, this is not right, reverting fix nothing, we are just back to previous
> bug, this is unrelated, the bug is in `easy-mmode--mode-docstring` which
> have a misleading arg called 'getter' and naturally `define-minor-mode`
> pass 'getter' to it instead of passing 'mode'.

No, this is what we want: in some cases what the user should eval to
check whether a mode is enabled is _not_ the mode variable, but some
other expression.  This is why we have that sentence!  To inform the
user about what to eval exactly.

The code receives that expression via the argument named GETTER.

Also not that in your original recipe (I just tried it), you do _not_
just get to the very same page.  Instead I get to a new help page
listing _all_ things named `...-mode' - the mode variable has been
added.

And I think this is the best behavior our current heuristic can come up
with, and in this case a minor issue.

What we could do is maybe: generate different text in the two cases
(expression is the mode variable vs. it is different), or even better,
add the text only in the "special" case where the mode variable is not
the thing to eval.


Michael.

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Michael Heerdegen via Emacs development discussions.]

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

Michael Heerdegen via "Emacs development discussions."
<emacs-devel@gnu.org> writes:

> What we could do is maybe: generate different text in the two cases
> (expression is the mode variable vs. it is different), or even better,
> add the text only in the "special" case where the mode variable is not
> the thing to eval.

How 'bout this? (based on master where the last fix had not yet been
reverted)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Minor-improvement-in-easy-mmode-mode-docstring.patch --]
[-- Type: text/x-diff, Size: 1526 bytes --]

From 701068acd90e5d5cd9b12cb7541a9020568a9893 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Tue, 9 Jul 2024 02:32:54 +0200
Subject: [PATCH] WIP: Minor improvement in easy-mmode--mode-docstring

---
 lisp/emacs-lisp/easy-mmode.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 2c12454af5c..2d6333f146f 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -91,7 +91,7 @@ easy-mmode--arg-docstring
 Disable the mode if ARG is a negative number.
 
 To check whether the minor mode is enabled in the current buffer,
-evaluate the variable `%s'.
+evaluate %s.
 
 The mode's hook is called both when the mode is enabled and when
 it is disabled.")
@@ -128,8 +128,9 @@ easy-mmode--mode-docstring
                         easy-mmode--arg-docstring
                         (if global "global " "")
                         mode-pretty-name
-                        ;; Avoid having quotes turn into pretty quotes.
-                        (string-replace "'" "\\='" (format "%S" getter)))))
+                        (format (if (symbolp getter) "the variable `%s'" "%s")
+                                ;; Avoid having quotes turn into pretty quotes.
+                                (help--docstring-quote (format "%S" getter))))))
           (let ((start (point)))
             (insert argdoc)
             (when (fboundp 'fill-region) ;Don't break bootstrap!
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 111 bytes --]


I took the chance and replaced the manual quoting treatment with a call
to `help--docstring-quote'.

Michael.

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Thierry Volpiatto]

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

Thierry Volpiatto <thievol@posteo.net> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Eshel Yaron <me@eshelyaron.com> writes:
>>
>>> I think it's not always correct to say "the variable" here, because that
>>> "%s" may be replaced with a form that's not just a variable.  E.g. now
>>> C-h f global-auto-revert-mode RET says:
>>>
>>>   To check whether the minor mode is enabled in the current buffer,
>>>   evaluate the variable ‘(default-value 'global-auto-revert-mode)’.
>>
>> Thanks, you're right.  Now reverted.
>
> No, this is not right, reverting fix nothing, we are just back to previous
> bug, this is unrelated, the bug is in `easy-mmode--mode-docstring` which
> have a misleading arg called 'getter' and naturally `define-minor-mode`
> pass 'getter' to it instead of passing 'mode'.
>
> PS: Please CC me as I am not suscribed to this list, thanks.

So to resume, the change done in bug#71815 i.e. adding "the variable"
before the button in `easy-mmode--arg-docstring` have nothing to do with
what described above (about global-auto-revert-mode), this bug was here
before the change introduced by bug#71815, so please reenable "the
variable" change in `easy-mmode--arg-docstring` . The following patch
should fixes the bug described above about global minor-mode showing the
exp (default-value 'global-xxx-mode), this happens in all global minor-modes.

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 7006ae6c785..445cf822045 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -97,7 +97,7 @@ The mode's hook is called both when the mode is enabled and when
 it is disabled.")
 
 (defun easy-mmode--mode-docstring (doc mode-pretty-name keymap-sym
-                                       getter global)
+                                       variable global)
   ;; If we have a doc string, and it's already complete (which we
   ;; guess at with the simple heuristic below), then just return that
   ;; as is.
@@ -129,7 +129,7 @@ it is disabled.")
                         (if global "global " "")
                         mode-pretty-name
                         ;; Avoid having quotes turn into pretty quotes.
-                        (string-replace "'" "\\='" (format "%S" getter)))))
+                        (string-replace "'" "\\='" (format "%S" variable)))))
           (let ((start (point)))
             (insert argdoc)
             (when (fboundp 'fill-region) ;Don't break bootstrap!
@@ -336,7 +336,7 @@ or call the function `%s'."))))
          warnwrap
          `(defun ,modefun (&optional arg ,@extra-args)
             ,(easy-mmode--mode-docstring doc pretty-name keymap-sym
-                                         getter globalp)
+                                         (or variable mode) globalp)
             ,(when interactive
 	       ;; Use `toggle' rather than (if ,mode 0 1) so that using
 	       ;; repeat-command still does the toggling correctly.

-- 
Thierry

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

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Michael Heerdegen via Emacs development discussions.]

Thierry Volpiatto <thievol@posteo.net> writes:

>  (defun easy-mmode--mode-docstring (doc mode-pretty-name keymap-sym
> -                                       getter global)
> +                                       variable global)

I think this would not tell what is intended to tell: to inform the user
about how to decide whether the mode is on.

Please have a look at my other messages from today, and sorry for CC'ing
the wrong address.

Or am I wrong and the user should always use the mode variable to
decide?


Thanks,

Michael.

Re: emacs-30 73c1252bb6b: Fix link to major mode variable in docstring

[Thierry Volpiatto]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>
>> Or even better would be to eval directly the getter and tell user
>> "mode is %s" on/off instead of proposing to eval.
>
> AFAIU, this is not about telling whether the mode is on at the moment,
> but to tell the user - or programmer - how to check that.
>
>> IIUC the only expessions we may have are (default-value
>> 'global-foo-mode) for global mmodes or foo-mode for normal mmodes.
>
> No, there are more complicated ones, like these:
>
> (frame-parameter nil 'auto-raise)      - auto-raise-mode
> (frame-parameter nil 'auto-lower)      - auto-lower-mode
> (show-paren--enabled-p)                - show-paren-local-mode
> (get-scroll-bar-mode)                  - scroll-bar-mode
> (and buffer-auto-save-file-name
>   (>= buffer-saved-size 0))            - auto-save-mode
> (eq (terminal-parameter nil 'normal-erase-is-backspace) 1)
>                              - normal-erase-is-backspace-mode
> (eq standard-display-table iso-ascii-display-table)
>                              - iso-ascii-mode
>
> AFAIU that piece of docstring we discuss is especially helpful in these
> special cases.

Ok, good to know, thanks.

> [ did we lose emacs-dev intentially btw? ]

No, don't know what's happened, now CCing to all.

>
> Regards,
>
> Michael.

-- 
Thierry

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