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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56679 in the body.
You can then email your comments to 56679 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: YE <yet <at> ego.team>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.1; whitespace-style cannot be configured for diff-mode via hook
Date: Thu, 21 Jul 2022 17:46:17 +0300

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

Message #8 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: YE via "Bug reports for GNU Emacs, the Swiss army knife of text editors"
 <bug-gnu-emacs <at> gnu.org>
Cc: YE <yet <at> ego.team>, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; whitespace-style cannot be configured for
 diff-mode via hook
Date: Sat, 23 Jul 2022 02:15:39 +0200

YE via "Bug reports for GNU Emacs, the Swiss army knife of text editors"
<bug-gnu-emacs <at> 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.

Message #14 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: YE <yet <at> ego.team>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: yet <at> ego.team, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for
 diff-mode via hook
Date: Sun, 24 Jul 2022 10:49:33 +0300

[Message part 1 (text/plain, inline)]

>> 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.

[0001-Add-new-user-option-diff-whitespace-style.patch (text/x-patch, attachment)]

Message #17 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: YE <yet <at> ego.team>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured
 for diff-mode via hook
Date: Sun, 24 Jul 2022 11:02:12 +0200

YE <yet <at> ego.team> writes:

> The proposed patch is attached.

Looks good to me; pushed to Emacs 29.

Message #22 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: YE <yet <at> ego.team>
Cc: michael_heerdegen <at> web.de, yet <at> ego.team, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1;
 [PATCH] whitespace-style cannot be configured for diff-mode via hook
Date: Sun, 24 Jul 2022 12:08:27 +0300

> Cc: yet <at> ego.team, 56679 <at> 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 <at> 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?

Message #25 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: YE <yet <at> ego.team>, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured
 for diff-mode via hook
Date: Mon, 25 Jul 2022 04:38:18 +0200

Eli Zaretskii <eliz <at> 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.

Message #28 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: yet <at> ego.team, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured
 for diff-mode via hook
Date: Mon, 25 Jul 2022 14:07:11 +0300

> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: YE <yet <at> ego.team>,  56679 <at> debbugs.gnu.org
> Date: Mon, 25 Jul 2022 04:38:18 +0200
> 
> Eli Zaretskii <eliz <at> 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?

Message #31 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: yet <at> ego.team, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured
 for diff-mode via hook
Date: Tue, 26 Jul 2022 05:59:22 +0200

Eli Zaretskii <eliz <at> 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.

Message #34 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: YE <yet <at> ego.team>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: larsi <at> gnus.org, eliz <at> gnu.org, yet <at> ego.team, 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured
 for diff-mode via hook
Date: Tue, 26 Jul 2022 15:27:18 +0300

[Message part 1 (text/plain, inline)]

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

Sure. See attached.

[0001-Improve-diff-whitespace-style-user-option-declaratio.patch (text/x-patch, attachment)]

Message #37 received at 56679 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: YE <yet <at> ego.team>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, eliz <at> gnu.org,
 56679 <at> debbugs.gnu.org
Subject: Re: bug#56679: 28.1; [PATCH] whitespace-style cannot be configured
 for diff-mode via hook
Date: Wed, 27 Jul 2022 11:52:11 +0200

YE <yet <at> ego.team> writes:

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

Thanks; pushed to Emacs 29.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.