Re: master 5a8a5e3d: Input fontification for M-x shell

Re: master 5a8a5e3d: Input fontification for M-x shell

[Eli Zaretskii]

> +(defcustom shell-indirect-setup-hook nil
> +  "Hook run after setting up an indirect shell fontification buffer."

Thank you for working on these features.  I have a general comment
about this and other of your changes that were installed recently:

It is not useful to have customizable user options whose doc string
doesn't explain their purpose.  It makes discovery and use of such
options much harder.  Imagine a user who upgrades to Emacs 29 and runs
"M-x customize-changed" to learn about new options in the new version:
what will such user understand by reading the doc string of this
option?  There's no explanation what are indirect shell fontification
buffers, nor how and for what purpose they are used.  Without that, it
is impossible to understand when and how this hook could be useful.

Could you please augment the doc string with the above in mind?

The same goes for several other hooks in your changes installed today.

Re: master 5a8a5e3d: Input fontification for M-x shell

[miha]

[-- Attachment #1.1: Type: text/plain, Size: 1095 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> +(defcustom shell-indirect-setup-hook nil
>> +  "Hook run after setting up an indirect shell fontification buffer."
>
> Thank you for working on these features.  I have a general comment
> about this and other of your changes that were installed recently:
>
> It is not useful to have customizable user options whose doc string
> doesn't explain their purpose.  It makes discovery and use of such
> options much harder.  Imagine a user who upgrades to Emacs 29 and runs
> "M-x customize-changed" to learn about new options in the new version:
> what will such user understand by reading the doc string of this
> option?  There's no explanation what are indirect shell fontification
> buffers, nor how and for what purpose they are used.  Without that, it
> is impossible to understand when and how this hook could be useful.
>
> Could you please augment the doc string with the above in mind?
>
> The same goes for several other hooks in your changes installed today.

Thanks. Will something like this be good? I also tried to address your
other e-mail.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Improve-doc-strings-of-comint-input-fontification-me.patch --]
[-- Type: text/x-patch, Size: 6043 bytes --]

From 31b0c60a8ae1e0fe10a3f3d7dd865e2b20086d6e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Sat, 10 Sep 2022 17:47:12 +0200
Subject: [PATCH] Improve doc strings of comint input fontification mechanism

---
 lisp/comint.el | 14 +++++++++-----
 lisp/ielm.el   | 12 +++++++++---
 lisp/shell.el  | 27 +++++++++++++++++++--------
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 751e561d3e..f9094a932c 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -4030,9 +4030,13 @@ comint-indirect-setup-function
 `after-change-major-mode-hook' are bound to nil.")
 
 (defcustom comint-indirect-setup-hook nil
-  "Hook run after setting up an indirect comint fontification buffer.
-It is run after the indirect buffer is set up for fontification
-of input regions."
+  "Hook run in an indirect buffer for input fontification.
+If input fontification or indentation is enabled, create an
+indirect buffer and set up its major mode and syntax
+highlighting.  As the final step of its initialization, run this
+hook with the indirect buffer as the current buffer.
+Fontification and indentation of input is then performed in the
+indirect buffer."
   :group 'comint
   :type 'hook
   :version "29.1")
@@ -4117,8 +4121,8 @@ comint--fl-ppss-flush-indirect
 
 (defun comint--fl-fontify-region (fun beg end verbose)
   "Fontify process output and user input in the current comint buffer.
-First, highlight the region between BEG and END using FUN.  Then
-highlight only the input text in the region with the help of an
+First, fontify the region between BEG and END using FUN.  Then
+fontify only the input text in the region with the help of an
 indirect buffer.  VERBOSE is passed to the fontify-region
 functions.  Skip fontification of input regions with non-nil
 `comint--fl-inhibit-fontification' text property."
diff --git a/lisp/ielm.el b/lisp/ielm.el
index 211804210c..3af91113ef 100644
--- a/lisp/ielm.el
+++ b/lisp/ielm.el
@@ -475,16 +475,22 @@ ielm-set-pm
 ;;; Input fontification
 
 (defcustom ielm-comint-fl-enable t
-  "Enable highlighting of input in ielm buffers.
+  "Enable fontification of input in ielm buffers.
 This variable only has effect when creating an ielm buffer.  Use
-the command `comint-fl-mode' to toggle highlighting of input in
+the command `comint-fl-mode' to toggle fontification of input in
 an already existing ielm buffer."
   :type 'boolean
   :safe 'booleanp
   :version "29.1")
 
 (defcustom ielm-indirect-setup-hook nil
-  "Hook run after setting up an indirect ielm fontification buffer."
+  "Hook run in an indirect buffer for input fontification.
+If input fontification is enabled for an IELM buffer, create an
+indirect buffer and set up its major mode and syntax
+highlighting.  In addition to `comint-indirect-setup-hook', run
+this hook with the indirect buffer as the current buffer, to
+perform the final step of its initialization.  Fontification and
+indentation of input is then performed in the indirect buffer."
   :type 'boolean
   :safe 'booleanp
   :version "29.1")
diff --git a/lisp/shell.el b/lisp/shell.el
index eccac66376..a45746be50 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -309,16 +309,23 @@ shell-input-autoexpand
   :group 'shell)
 
 (defcustom shell-comint-fl-enable t
-  "Enable highlighting of input in shell buffers.
+  "Enable fontification of input in shell buffers.
 This variable only has effect when the shell is started.  Use the
-command `comint-fl-mode' to toggle highlighting of input."
+command `comint-fl-mode' to toggle fontification of input."
   :type 'boolean
   :group 'shell
   :safe 'booleanp
   :version "29.1")
 
 (defcustom shell-indirect-setup-hook nil
-  "Hook run after setting up an indirect shell fontification buffer."
+  "Hook run in an indirect buffer for input fontification.
+If input fontification or indentation is enabled for a
+`shell-mode' buffer, create an indirect buffer and set up its
+major mode and syntax highlighting.  In addition to
+`comint-indirect-setup-hook', run this hook with the indirect
+buffer as the current buffer, to perform the final step of its
+initialization.  Fontification and indentation of input is then
+performed in the indirect buffer."
   :type 'boolean
   :group 'shell
   :safe 'booleanp
@@ -1680,7 +1687,7 @@ shell-highlight-undef-matcher
     t))
 
 (defvar-local shell--highlight-undef-indirect nil
-  "t if shell commands are fontified in `comint-indirect-buffer'.")
+  "Non-nil if shell commands are fontified in `comint-indirect-buffer'.")
 
 (declare-function sh-feature "sh-script" (alist &optional function))
 (defvar sh-leading-keywords)
@@ -1700,7 +1707,7 @@ shell-highlight-undef-mode
             (font-lock-remove-keywords nil shell-highlight-undef-keywords))))
     (font-lock-remove-keywords nil shell-highlight-undef-keywords))
   (remove-hook 'comint-fl-mode-hook
-               #'shell-highlight-undef-reset-mode t)
+               #'shell-highlight-undef-mode-restart t)
 
   (when shell-highlight-undef-mode
     (when comint-use-prompt-regexp
@@ -1742,12 +1749,16 @@ shell-highlight-undef-mode
             (t (funcall setup))))
 
     (add-hook 'comint-fl-mode-hook
-              #'shell-highlight-undef-reset-mode nil t))
+              #'shell-highlight-undef-mode-restart nil t))
 
   (font-lock-flush))
 
-(defun shell-highlight-undef-reset-mode ()
-  "If `shell-highlight-undef-mode' is on, turn it off and on."
+(defun shell-highlight-undef-mode-restart ()
+  "If `shell-highlight-undef-mode' is on, restart it.
+`shell-highlight-undef-mode' performs its set-up differently
+depending on `comint-fl-mode'.  It's useful to call this function
+when switching `comint-fl-mode' in order to make
+`shell-highlight-undef-mode' redo its setup."
   (when shell-highlight-undef-mode
     (shell-highlight-undef-mode 1)))
 
-- 
2.37.2


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

Re: master 5a8a5e3d: Input fontification for M-x shell

[Eli Zaretskii]

> From: <miha@kamnitnik.top>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, emacs-devel@gnu.org
> Date: Sat, 10 Sep 2022 17:53:51 +0200
> 
> > It is not useful to have customizable user options whose doc string
> > doesn't explain their purpose.  It makes discovery and use of such
> > options much harder.  Imagine a user who upgrades to Emacs 29 and runs
> > "M-x customize-changed" to learn about new options in the new version:
> > what will such user understand by reading the doc string of this
> > option?  There's no explanation what are indirect shell fontification
> > buffers, nor how and for what purpose they are used.  Without that, it
> > is impossible to understand when and how this hook could be useful.
> >
> > Could you please augment the doc string with the above in mind?
> >
> > The same goes for several other hooks in your changes installed today.
> 
> Thanks. Will something like this be good? I also tried to address your
> other e-mail.

It is much better, thanks.

>  (defcustom comint-indirect-setup-hook nil
> -  "Hook run after setting up an indirect comint fontification buffer.
> -It is run after the indirect buffer is set up for fontification
> -of input regions."
> +  "Hook run in an indirect buffer for input fontification.
> +If input fontification or indentation is enabled, create an
> +indirect buffer and set up its major mode and syntax

You say "create an indirect buffer and set up...", but isn't that done
by some of the other code you submitted?  If so, instead of
"create...and set up...", which implies that the user of this variable
must do so, the doc string should say something like

  If input fontification or indentation is enabled, function
  `so-and-so' creates an indirect buffer and sets up its major mode
  and syntax highlighting.  This hook is useful for running in that
  indirect buffer when its major mode is turned on.

> -(defun shell-highlight-undef-reset-mode ()
> -  "If `shell-highlight-undef-mode' is on, turn it off and on."
> +(defun shell-highlight-undef-mode-restart ()
> +  "If `shell-highlight-undef-mode' is on, restart it.
> +`shell-highlight-undef-mode' performs its set-up differently
> +depending on `comint-fl-mode'.  It's useful to call this function
> +when switching `comint-fl-mode' in order to make
> +`shell-highlight-undef-mode' redo its setup."

Ah, now this makes sense.  Thanks.

Re: master 5a8a5e3d: Input fontification for M-x shell

[miha]

[-- Attachment #1.1: Type: text/plain, Size: 636 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:


> [...]

> You say "create an indirect buffer and set up...", but isn't that done
> by some of the other code you submitted?  If so, instead of
> "create...and set up...", which implies that the user of this variable
> must do so, the doc string should say something like
>
>   If input fontification or indentation is enabled, function
>   `so-and-so' creates an indirect buffer and sets up its major mode
>   and syntax highlighting.  This hook is useful for running in that
>   indirect buffer when its major mode is turned on.

Okay, thanks for feedback, please find a refined patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Improve-doc-strings-of-comint-input-fontification-me.patch --]
[-- Type: text/x-patch, Size: 6156 bytes --]

From dc79db872ac0a90b759f7999afadc75a60b2ade2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Sat, 10 Sep 2022 17:47:12 +0200
Subject: [PATCH] Improve doc strings of comint input fontification mechanism

---
 lisp/comint.el | 14 +++++++++-----
 lisp/ielm.el   | 13 ++++++++++---
 lisp/shell.el  | 27 +++++++++++++++++++--------
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 751e561d3e..696dac3d12 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -4030,9 +4030,13 @@ comint-indirect-setup-function
 `after-change-major-mode-hook' are bound to nil.")
 
 (defcustom comint-indirect-setup-hook nil
-  "Hook run after setting up an indirect comint fontification buffer.
-It is run after the indirect buffer is set up for fontification
-of input regions."
+  "Hook run in an indirect buffer for input fontification.
+Input fontification and indentation, if enabled, is performed in
+an indirect buffer, whose major mode and syntax highlighting are
+set up according to `comint-indirect-setup-function'.  After this
+setup is done, run this hook with the indirect buffer as the
+current buffer.  This can be used to further customize
+fontification and other behaviour of the indirect buffer."
   :group 'comint
   :type 'hook
   :version "29.1")
@@ -4117,8 +4121,8 @@ comint--fl-ppss-flush-indirect
 
 (defun comint--fl-fontify-region (fun beg end verbose)
   "Fontify process output and user input in the current comint buffer.
-First, highlight the region between BEG and END using FUN.  Then
-highlight only the input text in the region with the help of an
+First, fontify the region between BEG and END using FUN.  Then
+fontify only the input text in the region with the help of an
 indirect buffer.  VERBOSE is passed to the fontify-region
 functions.  Skip fontification of input regions with non-nil
 `comint--fl-inhibit-fontification' text property."
diff --git a/lisp/ielm.el b/lisp/ielm.el
index 211804210c..4a10c00297 100644
--- a/lisp/ielm.el
+++ b/lisp/ielm.el
@@ -475,16 +475,23 @@ ielm-set-pm
 ;;; Input fontification
 
 (defcustom ielm-comint-fl-enable t
-  "Enable highlighting of input in ielm buffers.
+  "Enable fontification of input in ielm buffers.
 This variable only has effect when creating an ielm buffer.  Use
-the command `comint-fl-mode' to toggle highlighting of input in
+the command `comint-fl-mode' to toggle fontification of input in
 an already existing ielm buffer."
   :type 'boolean
   :safe 'booleanp
   :version "29.1")
 
 (defcustom ielm-indirect-setup-hook nil
-  "Hook run after setting up an indirect ielm fontification buffer."
+  "Hook run in an indirect buffer for input fontification.
+Input fontification and indentation of an IELM buffer, if
+enabled, is performed in an indirect buffer, whose indentation
+and syntax highlighting are set up with `emacs-lisp-mode'.  In
+addition to `comint-indirect-setup-hook', run this hook with the
+indirect buffer as the current buffer after its setup is done.
+This can be used to further customize fontification and other
+behaviour of the indirect buffer."
   :type 'boolean
   :safe 'booleanp
   :version "29.1")
diff --git a/lisp/shell.el b/lisp/shell.el
index eccac66376..834ca6b627 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -309,16 +309,23 @@ shell-input-autoexpand
   :group 'shell)
 
 (defcustom shell-comint-fl-enable t
-  "Enable highlighting of input in shell buffers.
+  "Enable fontification of input in shell buffers.
 This variable only has effect when the shell is started.  Use the
-command `comint-fl-mode' to toggle highlighting of input."
+command `comint-fl-mode' to toggle fontification of input."
   :type 'boolean
   :group 'shell
   :safe 'booleanp
   :version "29.1")
 
 (defcustom shell-indirect-setup-hook nil
-  "Hook run after setting up an indirect shell fontification buffer."
+  "Hook run in an indirect buffer for input fontification.
+Input fontification and indentation of a `shell-mode' buffer, if
+enabled, is performed in an indirect buffer, whose indentation
+and syntax highlighting is set up with `sh-mode'.  In addition to
+`comint-indirect-setup-hook', run this hook with the indirect
+buffer as the current buffer after its setup is done.  This can
+be used to further customize fontification and other behaviour of
+the indirect buffer."
   :type 'boolean
   :group 'shell
   :safe 'booleanp
@@ -1680,7 +1687,7 @@ shell-highlight-undef-matcher
     t))
 
 (defvar-local shell--highlight-undef-indirect nil
-  "t if shell commands are fontified in `comint-indirect-buffer'.")
+  "Non-nil if shell commands are fontified in `comint-indirect-buffer'.")
 
 (declare-function sh-feature "sh-script" (alist &optional function))
 (defvar sh-leading-keywords)
@@ -1700,7 +1707,7 @@ shell-highlight-undef-mode
             (font-lock-remove-keywords nil shell-highlight-undef-keywords))))
     (font-lock-remove-keywords nil shell-highlight-undef-keywords))
   (remove-hook 'comint-fl-mode-hook
-               #'shell-highlight-undef-reset-mode t)
+               #'shell-highlight-undef-mode-restart t)
 
   (when shell-highlight-undef-mode
     (when comint-use-prompt-regexp
@@ -1742,12 +1749,16 @@ shell-highlight-undef-mode
             (t (funcall setup))))
 
     (add-hook 'comint-fl-mode-hook
-              #'shell-highlight-undef-reset-mode nil t))
+              #'shell-highlight-undef-mode-restart nil t))
 
   (font-lock-flush))
 
-(defun shell-highlight-undef-reset-mode ()
-  "If `shell-highlight-undef-mode' is on, turn it off and on."
+(defun shell-highlight-undef-mode-restart ()
+  "If `shell-highlight-undef-mode' is on, restart it.
+`shell-highlight-undef-mode' performs its setup differently
+depending on `comint-fl-mode'.  It's useful to call this function
+when switching `comint-fl-mode' in order to make
+`shell-highlight-undef-mode' redo its setup."
   (when shell-highlight-undef-mode
     (shell-highlight-undef-mode 1)))
 
-- 
2.37.2


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

Re: master 5a8a5e3d: Input fontification for M-x shell

[Lars Ingebrigtsen]

<miha@kamnitnik.top> writes:

> Okay, thanks for feedback, please find a refined patch attached.

Thanks; pushed to Emacs 29.