all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
@ 2022-07-06 16:22 Michael Heerdegen
  2022-07-06 18:58 ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-06 16:22 UTC (permalink / raw)
  To: 56423


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)






^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-06 16:22 bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode Michael Heerdegen
@ 2022-07-06 18:58 ` Michael Heerdegen
  2022-07-06 19:19   ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-06 18:58 UTC (permalink / raw)
  To: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-06 18:58 ` Michael Heerdegen
@ 2022-07-06 19:19   ` Michael Heerdegen
  2022-07-07  5:35     ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-06 19:19 UTC (permalink / raw)
  To: 56423

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-06 19:19   ` Michael Heerdegen
@ 2022-07-07  5:35     ` Eli Zaretskii
  2022-07-07 10:26       ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2022-07-07  5:35 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 56423

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07  5:35     ` Eli Zaretskii
@ 2022-07-07 10:26       ` Michael Heerdegen
  2022-07-07 10:35         ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-07 10:26 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 10:26       ` Michael Heerdegen
@ 2022-07-07 10:35         ` Eli Zaretskii
  2022-07-07 11:14           ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2022-07-07 10:35 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 56423

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 10:35         ` Eli Zaretskii
@ 2022-07-07 11:14           ` Michael Heerdegen
  2022-07-07 13:29             ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-07 11:14 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 11:14           ` Michael Heerdegen
@ 2022-07-07 13:29             ` Eli Zaretskii
  2022-07-07 15:09               ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2022-07-07 13:29 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 56423

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 13:29             ` Eli Zaretskii
@ 2022-07-07 15:09               ` Michael Heerdegen
  2022-07-07 15:19                 ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-07 15:09 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 15:09               ` Michael Heerdegen
@ 2022-07-07 15:19                 ` Michael Heerdegen
  2022-07-07 16:03                   ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-07 15:19 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 15:19                 ` Michael Heerdegen
@ 2022-07-07 16:03                   ` Eli Zaretskii
  2022-07-08 12:04                     ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2022-07-07 16:03 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 56423

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-07 16:03                   ` Eli Zaretskii
@ 2022-07-08 12:04                     ` Michael Heerdegen
  2022-07-08 12:22                       ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-08 12:04 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-08 12:04                     ` Michael Heerdegen
@ 2022-07-08 12:22                       ` Eli Zaretskii
  2022-07-08 12:57                         ` Michael Heerdegen
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2022-07-08 12:22 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 56423

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-08 12:22                       ` Eli Zaretskii
@ 2022-07-08 12:57                         ` Michael Heerdegen
  2022-07-08 12:59                           ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Heerdegen @ 2022-07-08 12:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 56423

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.





^ permalink raw reply	[flat|nested] 15+ messages in thread

* bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode
  2022-07-08 12:57                         ` Michael Heerdegen
@ 2022-07-08 12:59                           ` Eli Zaretskii
  0 siblings, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2022-07-08 12:59 UTC (permalink / raw)
  To: Michael Heerdegen; +Cc: 56423

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





^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-07-08 12:59 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-06 16:22 bug#56423: 29.0.50; Hi-lock in dired broken by toggling wdired-mode Michael Heerdegen
2022-07-06 18:58 ` Michael Heerdegen
2022-07-06 19:19   ` Michael Heerdegen
2022-07-07  5:35     ` Eli Zaretskii
2022-07-07 10:26       ` Michael Heerdegen
2022-07-07 10:35         ` Eli Zaretskii
2022-07-07 11:14           ` Michael Heerdegen
2022-07-07 13:29             ` Eli Zaretskii
2022-07-07 15:09               ` Michael Heerdegen
2022-07-07 15:19                 ` Michael Heerdegen
2022-07-07 16:03                   ` Eli Zaretskii
2022-07-08 12:04                     ` Michael Heerdegen
2022-07-08 12:22                       ` Eli Zaretskii
2022-07-08 12:57                         ` Michael Heerdegen
2022-07-08 12:59                           ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.