bug#56679: 28.1; whitespace-style cannot be configured for diff-mode via hook

bug#56679: 28.1; whitespace-style cannot be configured for diff-mode via hook

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

Steps To Reproduce:

emacs -Q

*diff-mode-hook*

1.
#+begin_src emacs-lisp
  (defun my-diff-mode ()
    (setq-local whitespace-style '(face trailing spaces))
    (message "diff-mode-hook: %S" whitespace-style))

  (add-hook 'diff-mode-hook 'my-diff-mode)
#+end_src

2. 'M-x vc-dir <any-repo>'
3. Activate diff view (f.i. 'M-x vc-diff' or 'M-x log-view-diff')
4. See that 'C-h v whitespace-style' outputs '(face trailing)' instead of the
expected value '(face trailing spaces)'.

Additional Information

A bit of investigation showed that 'diff-setup-whitespace' always sets
'whitespace-style' to '(face trailing)'.

The debugged order of setting 'whitespace-style' value:
- diff-setup-whitespace: (face trailing)
- diff-mode-hook: (face trailing spaces)
- diff-setup-whitespace: (face trailing)

So might be the order of the value setting can be fixed?

Or maybe adding a defcustom 'diff-whitespace-style' would be a proper solution
here? I started working on such a patch but stuck disliking the probable need in
the 'whitespace-style' large ':type' definition duplication.


*whitespace-mode-hook*

I also attempted another approach (though didn't investigate it,
so probably it's a separate issue). It's also not functional.

emacs -Q

1.
#+begin_src emacs-lisp
(defun my-whitespace-mode ()
  (when (eq major-mode 'diff-mode)
    (setq-local whitespace-style '(face trailing spaces))))

(add-hook 'whitespace-mode-hook 'my-whitespace-mode)
#+end_src

2. In diff-mode 'C-h v whitespace-style' outputs expected '(face trailing)'.
3. Enable whitespace-mode 'M-x whitespace-mode'.
4. See that 'C-h v whitespace-style' outputs expected '(face trailing spaces)'.
   But this doesn't change the buffer's output.
5. Eval '(whitespace-turn-on)' to see the expected buffer output (or toggle the
   mode twice with 'M-x whitespace-mode').


*Current Workaround*

The workaround I currently use adds a keybinding to set 'whitespace-style'
and toggle 'whitespace-mode'.

#+begin_src emacs-lisp
(defun my-diff-whitespace ()
  "Toggle whitespace visualization with local `whitespace-style'."
  (interactive)
  (setq-local whitespace-style '(face trailing spaces))
  (whitespace-mode 'toggle))

(define-key diff-mode-map (kbd "C-c b w") 'my-diff-whitespace)
#+end_src

bug#56679: 28.1; whitespace-style cannot be configured for diff-mode via hook

[Michael Heerdegen]

YE via "Bug reports for GNU Emacs, the Swiss army knife of text editors"
<bug-gnu-emacs@gnu.org> writes:

> Or maybe adding a defcustom 'diff-whitespace-style' would be a proper
> solution here?

Since `diff-setup-whitespace' unconditionally sets whitespace-style from
a hardcoded value, this is the natural fix.

> I started working on such a patch but stuck disliking the probable
> need in the 'whitespace-style' large ':type' definition duplication.

I think you don't need a duplication.  You can reuse an existing type
definition by looking up the `custom-type` symbol property of an already
defined option.  See e.g. "lisp/eshell/em-cmpl.el" for a few example
definitions.

Michael.

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

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

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

>> Or maybe adding a defcustom 'diff-whitespace-style' would be a proper
>> solution here?
>
> Since `diff-setup-whitespace' unconditionally sets whitespace-style from
> a hardcoded value, this is the natural fix.
>
>> I started working on such a patch but stuck disliking the probable
>> need in the 'whitespace-style' large ':type' definition duplication.
>
> I think you don't need a duplication.  You can reuse an existing type
> definition by looking up the `custom-type` symbol property of an already
> defined option.  See e.g. "lisp/eshell/em-cmpl.el" for a few example
> definitions.

Thanks for the advice.

The proposed patch is attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Add user option diff-whitespace-style --]
[-- Type: text/x-patch, Size: 2545 bytes --]

From 86a90344673a8ca9645f11ec790c16eda6a4120e Mon Sep 17 00:00:00 2001
From: YugaEgo <yet@ego.team>
Date: Sat, 23 Jul 2022 22:25:10 +0300
Subject: [PATCH] Add new user option 'diff-whitespace-style'

* lisp/vc/diff-mode.el (diff-whitespace-style): New user option.
  (diff-setup-whitespace): Use it (Bug#56679).
  (top level): require 'whitespace.
  (whitespace-style, whitespace-trailing-regexp): Remove defvars.
---
 etc/NEWS             |  7 +++++++
 lisp/vc/diff-mode.el | 11 +++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 412a93bbf9..dcece83767 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1190,6 +1190,13 @@ the run/continue command.
 This is bound to 'H' and toggles whether to hide or show the widget
 contents.
 
+** Diff mode
+
+---
+*** New user option 'diff-whitespace-style'.
+This option determines buffer-local 'whitespace-style' value.
+
+
 ** Ispell
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 30ba4153a9..8d9caf35a3 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -56,6 +56,7 @@
 (eval-when-compile (require 'cl-lib))
 (eval-when-compile (require 'subr-x))
 (require 'easy-mmode)
+(require 'whitespace)
 
 (autoload 'vc-find-revision "vc")
 (autoload 'vc-find-revision-no-save "vc")
@@ -147,6 +148,11 @@ diff-font-lock-syntax
                  (const :tag "Highlight syntax" t)
                  (const :tag "Allow hunk-based fallback" hunk-also)))
 
+(defcustom diff-whitespace-style '(face trailing)
+  "Specify `whitespace-style' variable for the current Diff mode buffer."
+  :type (get 'whitespace-style 'custom-type)
+  :version "29.1")
+
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
@@ -1476,9 +1482,6 @@ diff--font-lock-cleanup
     ;; Added when diff--font-lock-prettify is non-nil!
     (cl-pushnew 'display font-lock-extra-managed-props)))
 
-(defvar whitespace-style)
-(defvar whitespace-trailing-regexp)
-
 (defvar-local diff-mode-read-only nil
   "Non-nil when read-only diff buffer uses short keys.")
 
@@ -1572,7 +1575,7 @@ diff-setup-whitespace
 This sets `whitespace-style' and `whitespace-trailing-regexp' so
 that Whitespace mode shows trailing whitespace problems on the
 modified lines of the diff."
-  (setq-local whitespace-style '(face trailing))
+  (setq-local whitespace-style diff-whitespace-style)
   (let ((style (save-excursion
 		 (goto-char (point-min))
                  ;; FIXME: For buffers filled from async processes, this search
-- 
2.34.1

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

[Lars Ingebrigtsen]

YE <yet@ego.team> writes:

> The proposed patch is attached.

Looks good to me; pushed to Emacs 29.

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

[Eli Zaretskii]

> Cc: yet@ego.team, 56679@debbugs.gnu.org
> Date: Sun, 24 Jul 2022 10:49:33 +0300
> From:  YE via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > I think you don't need a duplication.  You can reuse an existing type
> > definition by looking up the `custom-type` symbol property of an already
> > defined option.  See e.g. "lisp/eshell/em-cmpl.el" for a few example
> > definitions.
> 
> Thanks for the advice.
> 
> The proposed patch is attached.

Thanks.

> --- a/lisp/vc/diff-mode.el
> +++ b/lisp/vc/diff-mode.el
> @@ -56,6 +56,7 @@
>  (eval-when-compile (require 'cl-lib))
>  (eval-when-compile (require 'subr-x))
>  (require 'easy-mmode)
> +(require 'whitespace)

Can we delay loading whitespace.el until the user actually wants to
turn on whitespace-mode, or until he/she customizes this option?
AFAIU, whitespace-mode is not turned on unconditionally by diff-mode,
so this 'require' is not needed for users who don't turn that minor
mode in Diff buffers.

> +(defcustom diff-whitespace-style '(face trailing)
> +  "Specify `whitespace-style' variable for the current Diff mode buffer."

AFAIU, this style will be applied to all Diff mode buffers, not just
the "current" one.  Right?

> +** Diff mode
> +
> +---
> +*** New user option 'diff-whitespace-style'.
> +This option determines buffer-local 'whitespace-style' value.

Should we tell that if someone was using whitespace-style directly for
this purpose, they should use this new option instead?

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Can we delay loading whitespace.el until the user actually wants to
> turn on whitespace-mode, or until he/she customizes this option?
> AFAIU, whitespace-mode is not turned on unconditionally by diff-mode,
> so this 'require' is not needed for users who don't turn that minor
> mode in Diff buffers.

Principally correct - but how do we do this correctly for the
(get 'whitespace-style 'custom-type) :type definition?

> > +(defcustom diff-whitespace-style '(face trailing)
> > +  "Specify `whitespace-style' variable for the current Diff mode buffer."
>
> AFAIU, this style will be applied to all Diff mode buffers, not just
> the "current" one.  Right?

Yes, that, and also the language of the News entry, is a bit misleading.

> > +** Diff mode
> > +
> > +---
> > +*** New user option 'diff-whitespace-style'.
> > +This option determines buffer-local 'whitespace-style' value.
>
> Should we tell that if someone was using whitespace-style directly for
> this purpose, they should use this new option instead?

AFAIU this never worked, this is what this patch is about (unless those
someones knew some trick maybe).

Michael.

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: YE <yet@ego.team>,  56679@debbugs.gnu.org
> Date: Mon, 25 Jul 2022 04:38:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can we delay loading whitespace.el until the user actually wants to
> > turn on whitespace-mode, or until he/she customizes this option?
> > AFAIU, whitespace-mode is not turned on unconditionally by diff-mode,
> > so this 'require' is not needed for users who don't turn that minor
> > mode in Diff buffers.
> 
> Principally correct - but how do we do this correctly for the
> (get 'whitespace-style 'custom-type) :type definition?

What are the problems with this that require us to load whitespace.el
up front?

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > Principally correct - but how do we do this correctly for the
> > (get 'whitespace-style 'custom-type) :type definition?
>
> What are the problems with this that require us to load whitespace.el
> up front?

Ahhh... that expression is not evaluated before you try to customize the
option, right?  Nice.

YE, could you maybe try to take care of Elis hints?

Thanks,

Michael.

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

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

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

> YE, could you maybe try to take care of Elis hints?

Sure. See attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve 'diff-whitespace-style' user option declaration --]
[-- Type: text/x-patch, Size: 2187 bytes --]

From 2c4b52d4b9cb9728e2de8d8e01f27c73fb0a0f8d Mon Sep 17 00:00:00 2001
From: YugaEgo <yet@ego.team>
Date: Tue, 26 Jul 2022 15:17:44 +0300
Subject: [PATCH] Improve 'diff-whitespace-style' user option declaration

* lisp/vc/diff-mode.el (diff-whitespace-style):
Use ':require'.  Minor docstring fix.
(top level): Do not require 'whitespace.
(whitespace-style, whitespace-trailing-regexp):
Add defvars (rollback recent removal).
* etc/NEWS: Extend 'diff-whitespace-style' introduction.
(Bug#56679).
---
 etc/NEWS             | 4 +++-
 lisp/vc/diff-mode.el | 7 +++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1d0e45fdcc..86780165e9 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1194,7 +1194,9 @@ contents.
 
 ---
 *** New user option 'diff-whitespace-style'.
-This option determines buffer-local 'whitespace-style' value.
+Sets the value of the buffer-local variable 'whitespace-style' in
+'diff-mode' buffers.  By default, this variable is '(face trailing)',
+which preserves behavior from previous Emacs versions.
 
 
 ** Ispell
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 8d9caf35a3..aa426446d7 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -56,7 +56,6 @@
 (eval-when-compile (require 'cl-lib))
 (eval-when-compile (require 'subr-x))
 (require 'easy-mmode)
-(require 'whitespace)
 
 (autoload 'vc-find-revision "vc")
 (autoload 'vc-find-revision-no-save "vc")
@@ -149,7 +148,8 @@ diff-font-lock-syntax
                  (const :tag "Allow hunk-based fallback" hunk-also)))
 
 (defcustom diff-whitespace-style '(face trailing)
-  "Specify `whitespace-style' variable for the current Diff mode buffer."
+  "Specify `whitespace-style' variable for `diff-mode' buffers."
+  :require 'whitespace
   :type (get 'whitespace-style 'custom-type)
   :version "29.1")
 
@@ -1490,6 +1490,9 @@ diff-mode-read-only
     (nconc minor-mode-map-alist
            (list (cons 'diff-mode-read-only diff-mode-shared-map))))
 
+(defvar whitespace-style)
+(defvar whitespace-trailing-regexp)
+
 ;;;###autoload
 (define-derived-mode diff-mode fundamental-mode "Diff"
   "Major mode for viewing/editing context diffs.
-- 
2.34.1

bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook

[Lars Ingebrigtsen]

YE <yet@ego.team> writes:

>> YE, could you maybe try to take care of Elis hints?
>
> Sure. See attached.

Thanks; pushed to Emacs 29.