bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Hello,

in a dired buffer, M-x hi-lock-face-buffer RET and enter a pattern so
that there are matches.  Then M-x wdired-change-to-wdired-mode and edit
something (insert a char or so).  Hi-Lock highlighting immediately
disappears.  Changing back to normal dired doesn't let highlighting
reappear.  That's not good.  I would like highlighting to stay when
starting or finishing `wdired-mode'.

The (only) reason for this is that when toggling `wdired-mode' the
hi-lock pattern rules added to `font-lock-keywords' are gone (why?).

So this fixes the problem for me:

#+begin_src emacs-lisp
(defun my-wdired-run-font-lock-mode-hook (&rest _)
  "Necessary to get hi-lock survive toggling `wdired-mode'."
  (run-hooks 'font-lock-mode-hook))

(dolist (f `(,#'wdired-change-to-wdired-mode
             ,#'wdired-finish-edit
             ,#'wdired-abort-changes))
  (advice-add f :after #'my-wdired-run-font-lock-mode-hook))
#+end_src

Running `hi-lock-font-lock-hook' instead of the complete
`font-lock-mode-hook' also works.

I would like to know why this is necessary at all, and what a correct
fix that can be installed would look like.

TIA,

Michael.


In GNU Emacs 29.0.50 (build 14, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2022-07-06 built on drachen
Repository revision: b2df1cc19c2312b27f7bab6b3eb7d339f668113d
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> The (only) reason for this is that when toggling `wdired-mode' the
> hi-lock pattern rules added to `font-lock-keywords' are gone (why?).

Ok, tried to follow what happens.  Hitting the first key after entering
wdired calls `font-lock-set-defaults' like this:

| (font-lock-set-defaults)
| (font-lock-fontify-region 132 243)
| (jit-lock--run-functions 132 243)
| (jit-lock-fontify-now 132 1632)
| (jit-lock-function 132)

and `font-lock-set-defaults' finds that

  (derived-mode-p font-lock-major-mode)

fails so that it recomputes the `font-lock-keywords' from the defaults.
I guess we could prevent that happening?

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> [...] and `font-lock-set-defaults' finds that
>
>   (derived-mode-p font-lock-major-mode)
>
> fails so that it recomputes the `font-lock-keywords' from the
> defaults.  I guess we could prevent that happening?

This patch seems to fix the issue.  Good idea?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Fix-wdired-vs.-hi-lock-Bug-56423.patch --]
[-- Type: text/x-diff, Size: 1152 bytes --]

From 26baa6c65e934b110499c77255c5c54a89acd116 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Wed, 6 Jul 2022 21:16:19 +0200
Subject: [PATCH] WIP: Fix wdired vs. hi-lock Bug#56423

---
 lisp/wdired.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/wdired.el b/lisp/wdired.el
index a5858ed190..7c8969431f 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -261,6 +261,7 @@ wdired-change-to-wdired-mode
   (add-hook 'before-change-functions #'wdired--before-change-fn nil t)
   (add-hook 'after-change-functions #'wdired--restore-properties nil t)
   (setq major-mode 'wdired-mode)
+  (setq font-lock-major-mode 'wdired-mode)
   (setq mode-name "Editable Dired")
   (add-function :override (local 'revert-buffer-function) #'wdired-revert)
   (set-buffer-modified-p nil)
@@ -457,6 +458,7 @@ wdired-change-to-dired-mode
   (force-mode-line-update)
   (setq buffer-read-only t)
   (setq major-mode 'dired-mode)
+  (setq font-lock-major-mode 'dired-mode)
   (setq mode-name "Dired")
   (dired-advertise)
   (remove-hook 'kill-buffer-hook #'wdired-check-kill-buffer t)
--
2.30.2


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



TIA,

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Wed, 06 Jul 2022 21:19:35 +0200
> 
> diff --git a/lisp/wdired.el b/lisp/wdired.el
> index a5858ed190..7c8969431f 100644
> --- a/lisp/wdired.el
> +++ b/lisp/wdired.el
> @@ -261,6 +261,7 @@ wdired-change-to-wdired-mode
>    (add-hook 'before-change-functions #'wdired--before-change-fn nil t)
>    (add-hook 'after-change-functions #'wdired--restore-properties nil t)
>    (setq major-mode 'wdired-mode)
> +  (setq font-lock-major-mode 'wdired-mode)
>    (setq mode-name "Editable Dired")
>    (add-function :override (local 'revert-buffer-function) #'wdired-revert)
>    (set-buffer-modified-p nil)
> @@ -457,6 +458,7 @@ wdired-change-to-dired-mode
>    (force-mode-line-update)
>    (setq buffer-read-only t)
>    (setq major-mode 'dired-mode)
> +  (setq font-lock-major-mode 'dired-mode)
>    (setq mode-name "Dired")
>    (dired-advertise)
>    (remove-hook 'kill-buffer-hook #'wdired-check-kill-buffer t)

Don't we need to make sure font-lock-major-mode is restored no matter
how wdired is exited, whether normally or abnormally?

I'm not too familiar with wdired, so apologies if what I said makes no
sense.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > diff --git a/lisp/wdired.el b/lisp/wdired.el
> > index a5858ed190..7c8969431f 100644
> > --- a/lisp/wdired.el
> > +++ b/lisp/wdired.el
> > @@ -261,6 +261,7 @@ wdired-change-to-wdired-mode
> >    (add-hook 'before-change-functions #'wdired--before-change-fn nil t)
> >    (add-hook 'after-change-functions #'wdired--restore-properties nil t)
> >    (setq major-mode 'wdired-mode)
> > +  (setq font-lock-major-mode 'wdired-mode)
> >    (setq mode-name "Editable Dired")
> >    (add-function :override (local 'revert-buffer-function)
> > #'wdired-revert)
> >    (set-buffer-modified-p nil)
> > @@ -457,6 +458,7 @@ wdired-change-to-dired-mode
> >    (force-mode-line-update)
> >    (setq buffer-read-only t)
> >    (setq major-mode 'dired-mode)
> > +  (setq font-lock-major-mode 'dired-mode)
> >    (setq mode-name "Dired")
> >    (dired-advertise)
> >    (remove-hook 'kill-buffer-hook #'wdired-check-kill-buffer t)

> Don't we need to make sure font-lock-major-mode is restored no matter
> how wdired is exited, whether normally or abnormally?

There are two official ways to return - `wdired-abort-changes' and
`wdired-finish-edit' - both call `wdired-change-to-dired-mode' (which I
modified).

If a way of exiting doesn't call `wdired-change-to-dired-mode' your
dired buffer would be broken afterwards AFAIU - you must call it to get
rid of the modifications made by wdired.

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 56423@debbugs.gnu.org
> Date: Thu, 07 Jul 2022 12:26:03 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Don't we need to make sure font-lock-major-mode is restored no matter
> > how wdired is exited, whether normally or abnormally?
> 
> There are two official ways to return - `wdired-abort-changes' and
> `wdired-finish-edit' - both call `wdired-change-to-dired-mode' (which I
> modified).
> 
> If a way of exiting doesn't call `wdired-change-to-dired-mode' your
> dired buffer would be broken afterwards AFAIU - you must call it to get
> rid of the modifications made by wdired.

It sounds like you described the "normal" ways of returning?  I mostly
had in mind the "abnormal" ones, via one of the non-local exits.  If
that is possible, we should set up unwind-protect form.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> It sounds like you described the "normal" ways of returning?  I mostly
> had in mind the "abnormal" ones, via one of the non-local exits.  If
> that is possible, we should set up unwind-protect form.

Ah - ok.  But AFAIK there is no non-local way of exiting possible.  In
wdired you are at top-level, you can only "exit" explicitly.  The
wording "exit" is a bit misleading in this context, actually you are
just switching to a (or something like a) different major mode and back.
There is no code where we could add an `unwind-protect' to - there is no
continuation executed after "exiting" wdired.

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 56423@debbugs.gnu.org
> Date: Thu, 07 Jul 2022 13:14:05 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It sounds like you described the "normal" ways of returning?  I mostly
> > had in mind the "abnormal" ones, via one of the non-local exits.  If
> > that is possible, we should set up unwind-protect form.
> 
> Ah - ok.  But AFAIK there is no non-local way of exiting possible.

Not even with C-g at some un-opportune moment?  If so, there's no
problem, indeed.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Not even with C-g at some un-opportune moment?

I think this can only happen if you directly interrupt
`wdired-change-to-wdired-mode'.  You must have bad luck for that to
happen... but I think that case is already broken: there are no
provisions to handle that, you will likely end in an inconsistent state.
It's something to be fixed, but it's independent from my change.

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> It's something to be fixed, but it's independent from my change.

But you have a good nose (do you say that in English?).

We could protect the code with binding `inhibit-quit'.
Switching to wdired now nearly happens immediately, unlike in older
Emacsen where the whole buffer had to be traversed.  So inhibiting quit
should be ok...?

Thanks,

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 56423@debbugs.gnu.org
> Date: Thu, 07 Jul 2022 17:19:40 +0200
> 
> Michael Heerdegen <michael_heerdegen@web.de> writes:
> 
> > It's something to be fixed, but it's independent from my change.
> 
> But you have a good nose

Been there, did crash and burn, have scars to show.

> (do you say that in English?).

No clue (English is not my first language), but the meaning is clear
to me.

> We could protect the code with binding `inhibit-quit'.
> Switching to wdired now nearly happens immediately, unlike in older
> Emacsen where the whole buffer had to be traversed.  So inhibiting quit
> should be ok...?

Yes, it's okay to inhibit-quit for short intervals of time and around
small code fragments.

Thanks.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, it's okay to inhibit-quit for short intervals of time and around
> small code fragments.

The direction back (especially `wdired-finish-edit' after making many
changes) is problematic however.  If the user aborts when file changes
have been performed partly we have a problem I think.

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 56423@debbugs.gnu.org
> Date: Fri, 08 Jul 2022 14:04:22 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, it's okay to inhibit-quit for short intervals of time and around
> > small code fragments.
> 
> The direction back (especially `wdired-finish-edit' after making many
> changes) is problematic however.  If the user aborts when file changes
> have been performed partly we have a problem I think.

You mean, the display will no longer match the actual situation on
disk?  Then I think an unwind-protect or condition-case form that
resyncs the display in case of a signal is in order, since preventing
the user from aborting in that place would be wrong -- what if the
user realizes, too late, that some of the changes mean trouble?

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> You mean, the display will no longer match the actual situation on
> disk?  Then I think an unwind-protect or condition-case form that
> resyncs the display in case of a signal is in order, since preventing
> the user from aborting in that place would be wrong -- what if the
> user realizes, too late, that some of the changes mean trouble?

The question is how to unwind.  I guess the only sensible action would
be to revert the dired buffer, since we can't revert partial file
changes, and the situation could look differently in many different
ways.

Michael.

bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 56423@debbugs.gnu.org
> Date: Fri, 08 Jul 2022 14:57:34 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > You mean, the display will no longer match the actual situation on
> > disk?  Then I think an unwind-protect or condition-case form that
> > resyncs the display in case of a signal is in order, since preventing
> > the user from aborting in that place would be wrong -- what if the
> > user realizes, too late, that some of the changes mean trouble?
> 
> The question is how to unwind.  I guess the only sensible action would
> be to revert the dired buffer, since we can't revert partial file
> changes, and the situation could look differently in many different
> ways.

Yes, I think reverting is TRT here, since (AFAIU) the display will
then reflect what was actually changed and what wasn't.