* bug#56679: 28.1; whitespace-style cannot be configured for diff-mode via hook @ 2022-07-21 14:46 YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-07-23 0:15 ` Michael Heerdegen 0 siblings, 1 reply; 10+ messages in thread From: YE via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-07-21 14:46 UTC (permalink / raw) To: 56679 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; whitespace-style cannot be configured for diff-mode via hook 2022-07-21 14:46 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 @ 2022-07-23 0:15 ` Michael Heerdegen 2022-07-24 7:49 ` bug#56679: 28.1; [PATCH] " YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 10+ messages in thread From: Michael Heerdegen @ 2022-07-23 0:15 UTC (permalink / raw) To: 56679; +Cc: yet 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-23 0:15 ` Michael Heerdegen @ 2022-07-24 7:49 ` YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-07-24 9:02 ` Lars Ingebrigtsen 2022-07-24 9:08 ` Eli Zaretskii 0 siblings, 2 replies; 10+ messages in thread From: YE via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-07-24 7:49 UTC (permalink / raw) To: Michael Heerdegen; +Cc: yet, 56679 [-- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-24 7:49 ` bug#56679: 28.1; [PATCH] " YE via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-07-24 9:02 ` Lars Ingebrigtsen 2022-07-24 9:08 ` Eli Zaretskii 1 sibling, 0 replies; 10+ messages in thread From: Lars Ingebrigtsen @ 2022-07-24 9:02 UTC (permalink / raw) To: YE; +Cc: Michael Heerdegen, 56679 YE <yet@ego.team> writes: > The proposed patch is attached. Looks good to me; pushed to Emacs 29. ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-24 7:49 ` bug#56679: 28.1; [PATCH] " YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-07-24 9:02 ` Lars Ingebrigtsen @ 2022-07-24 9:08 ` Eli Zaretskii 2022-07-25 2:38 ` Michael Heerdegen 1 sibling, 1 reply; 10+ messages in thread From: Eli Zaretskii @ 2022-07-24 9:08 UTC (permalink / raw) To: YE; +Cc: michael_heerdegen, yet, 56679 > 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-24 9:08 ` Eli Zaretskii @ 2022-07-25 2:38 ` Michael Heerdegen 2022-07-25 11:07 ` Eli Zaretskii 0 siblings, 1 reply; 10+ messages in thread From: Michael Heerdegen @ 2022-07-25 2:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: YE, 56679 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-25 2:38 ` Michael Heerdegen @ 2022-07-25 11:07 ` Eli Zaretskii 2022-07-26 3:59 ` Michael Heerdegen 0 siblings, 1 reply; 10+ messages in thread From: Eli Zaretskii @ 2022-07-25 11:07 UTC (permalink / raw) To: Michael Heerdegen; +Cc: yet, 56679 > 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-25 11:07 ` Eli Zaretskii @ 2022-07-26 3:59 ` Michael Heerdegen 2022-07-26 12:27 ` YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 10+ messages in thread From: Michael Heerdegen @ 2022-07-26 3:59 UTC (permalink / raw) To: Eli Zaretskii; +Cc: yet, 56679 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-26 3:59 ` Michael Heerdegen @ 2022-07-26 12:27 ` YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-07-27 9:52 ` Lars Ingebrigtsen 0 siblings, 1 reply; 10+ messages in thread From: YE via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-07-26 12:27 UTC (permalink / raw) To: Michael Heerdegen; +Cc: larsi, eliz, yet, 56679 [-- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* bug#56679: 28.1; [PATCH] whitespace-style cannot be configured for diff-mode via hook 2022-07-26 12:27 ` YE via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-07-27 9:52 ` Lars Ingebrigtsen 0 siblings, 0 replies; 10+ messages in thread From: Lars Ingebrigtsen @ 2022-07-27 9:52 UTC (permalink / raw) To: YE; +Cc: Michael Heerdegen, eliz, 56679 YE <yet@ego.team> writes: >> YE, could you maybe try to take care of Elis hints? > > Sure. See attached. Thanks; pushed to Emacs 29. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-07-27 9:52 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-21 14:46 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 2022-07-23 0:15 ` Michael Heerdegen 2022-07-24 7:49 ` bug#56679: 28.1; [PATCH] " YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-07-24 9:02 ` Lars Ingebrigtsen 2022-07-24 9:08 ` Eli Zaretskii 2022-07-25 2:38 ` Michael Heerdegen 2022-07-25 11:07 ` Eli Zaretskii 2022-07-26 3:59 ` Michael Heerdegen 2022-07-26 12:27 ` YE via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-07-27 9:52 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).