Re: master 2399541: Remove font-lock toggle from font-lock-update

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

> +(defun font-lock-update ()
> +  "Refontify the accessible portion of the buffer.
> +Unconditionally activate `font-lock-mode'."
> +  (interactive)
> +  (unless font-lock-mode (font-lock-mode 1))
>    (save-excursion
> +    (font-lock-fontify-region (point-min) (point-max))))

I don't see which part of the code corresponds to "the accessible portion of".
AFAICT this will forcefully fontify the whole buffer which takes quite
a while on src/xdisp.c (for example).


        Stefan

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I don't see which part of the code corresponds to "the accessible
> portion of".

Isn't that the preferred euphemism for "the entire buffer, or if it's
narrowed, then that bit?"

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Paul W. Rankin via Emacs development discussions.]

> On 25 Mar 2021, at 1:23 am, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> +(defun font-lock-update ()
>> +  "Refontify the accessible portion of the buffer.
>> +Unconditionally activate `font-lock-mode'."
>> +  (interactive)
>> +  (unless font-lock-mode (font-lock-mode 1))
>>   (save-excursion
>> +    (font-lock-fontify-region (point-min) (point-max))))
> 
> I don't see which part of the code corresponds to "the accessible portion of".
> AFAICT this will forcefully fontify the whole buffer which takes quite
> a while on src/xdisp.c (for example).

True, I mentioned this earlier with reference to var `font-lock-dont-widen'. I'm just using at least some of the docstring from the previous command I'm coming down on here. Ideally this would be called something like `font-lock-force-update' with a more accurate docstring.

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Gregory Heytings]

     * lisp/font-lock.el (font-lock-update): Remove call to
     font-lock-unfontify-region and font-lock-mode toggle with ARG; this
     did not perform what original author intended

Is this what is supposed to happen?  Just forcibly push a change while the 
discussion is happening, when Stefan told you that your understanding is 
not right, and when I told you that the command _does_ what "original 
author" intented?  Should I now submit a patch to restore the proper 
command?

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Lars Ingebrigtsen]

Gregory Heytings <gregory@heytings.org> writes:

> Is this what is supposed to happen?  Just forcibly push a change while
> the discussion is happening, when Stefan told you that your
> understanding is not right, and when I told you that the command
> _does_ what "original author" intented?

No, this is not what's supposed to happen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

Lars Ingebrigtsen [2021-03-24 16:28:02] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I don't see which part of the code corresponds to "the accessible
>> portion of".
> Isn't that the preferred euphemism for "the entire buffer, or if it's
> narrowed, then that bit?"

Oh, I think you're right.  So the problem is not just in the code but
also in the description, because I don't think this description is
appropriate for something which is intended to replace the
`f-l-fontify-block` command (tho we can make it work fast in the usual
case where jit-lock is in use: use call `f-l-flush` instead of
`f-l-fontify-region`).


        Stefan

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Gregory Heytings <gregory@heytings.org> writes:
>
>> Is this what is supposed to happen?  Just forcibly push a change while
>> the discussion is happening, when Stefan told you that your
>> understanding is not right, and when I told you that the command
>> _does_ what "original author" intented?
>
> No, this is not what's supposed to happen.

So I've reverted Paul's change.

I think the command can be improved, but pushing a change like this in
the middle of a discussion is not the way to do it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Paul W. Rankin via Emacs development discussions.]

On 2021-03-25 02:03, Lars Ingebrigtsen wrote:
> So I've reverted Paul's change.
> 
> I think the command can be improved, but pushing a change like this in
> the middle of a discussion is not the way to do it.

No worries. I thought of pushing a change as the nicer alternative to a 
revert. To be blunt, the code ought to be reverted (for the reasons 
already stated).

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Lars Ingebrigtsen]

"Paul W. Rankin" via "Emacs development discussions."
<emacs-devel@gnu.org> writes:

> No worries. I thought of pushing a change as the nicer alternative to
> a revert. To be blunt, the code ought to be reverted (for the reasons
> already stated).

It's clear that this is your opinion, but you don't seem to have
convinced the other people participating in this discussion.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

Lars Ingebrigtsen [2021-03-24 18:49:51] wrote:
> "Paul W. Rankin" via "Emacs development discussions."
> <emacs-devel@gnu.org> writes:
>> No worries. I thought of pushing a change as the nicer alternative to
>> a revert. To be blunt, the code ought to be reverted (for the reasons
>> already stated).
> It's clear that this is your opinion, but you don't seem to have
> convinced the other people participating in this discussion.

FWIW, I'm not sure if `font-lock-update` (ether it's Paul's or the
other) is really better than the old `font-lock-fontify-block`.  I think
they all suffer from being fundamentally "defined by their
implementation" making it hard to evolve when some change is made to the
font-lock machinery.

I'd much prefer a longer `font-lock-fontify-diwm` which tries to
reproduce more or less the same behavior as your favorite, but by
explicitly testing the different circumstances you care about.


        Stefan

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Gregory Heytings]

>
> FWIW, I'm not sure if `font-lock-update` (ether it's Paul's or the 
> other) is really better than the old `font-lock-fontify-block`.  I think 
> they all suffer from being fundamentally "defined by their 
> implementation" making it hard to evolve when some change is made to the 
> font-lock machinery.
>

Hmmm...  You said: "`font-lock-fontify-region` is an important function 
internally (it's the one and only function which performs font-locking, 
fundamentally)".  Is it wrong to use it nonetheless?

>
> I'd much prefer a longer `font-lock-fontify-diwm` which tries to 
> reproduce more or less the same behavior as your favorite, but by 
> explicitly testing the different circumstances you care about.
>

Could you perhaps give an example of such a circumstance and its 
corresponding action?

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Paul W. Rankin via Emacs development discussions.]

> It's clear that this is your opinion, but you don't seem to have
> convinced the other people participating in this discussion.

You can only lead a horse to water...

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I'd much prefer a longer `font-lock-fontify-diwm` which tries to
> reproduce more or less the same behavior as your favorite, but by
> explicitly testing the different circumstances you care about.

Sure.  It could, for instance, refontify the region (if selected)
instead of the entire buffer.  And probably a lot of other DWIM-ish
niceness.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Gregory Heytings]

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


>> I'd much prefer a longer `font-lock-fontify-diwm` which tries to 
>> reproduce more or less the same behavior as your favorite, but by 
>> explicitly testing the different circumstances you care about.
>
> Sure.  It could, for instance, refontify the region (if selected) 
> instead of the entire buffer.  And probably a lot of other DWIM-ish 
> niceness.
>

Here's a patch to do this, and to fix the problem when "too large" buffers 
are refontified (xdisp.c for example).

[-- Attachment #2: Type: text/x-diff, Size: 3471 bytes --]

From 44332493a8796f692605e7e7ec55390742ff23bc Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 25 Mar 2021 09:50:05 +0000
Subject: [PATCH] Improve font-lock-update and rename it to font-lock-dwim

* lisp/font-lock.el (font-lock-dwim): Renamed from 'font-lock-update'.
Only refontify the region when it is active.  Do not refontify too
large buffers at once.

* lisp/bindings.el (ctl-x-x-map): Adjust the command name.

* etc/NEWS: Adjust the command name.
---
 etc/NEWS          |  4 ++--
 lisp/bindings.el  |  2 +-
 lisp/font-lock.el | 14 +++++++++-----
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 49a4bb8106..d7b5efaaee 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -94,7 +94,7 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 * Changes in Emacs 28.1
 
 +++
-** New command 'font-lock-update', bound to 'C-x x f'.
+** New command 'font-lock-dwim', bound to 'C-x x f'.
 This command updates the syntax highlighting in this buffer.
 
 ** The new NonGNU ELPA archive is enabled by default alongside GNU ELPA.
@@ -255,7 +255,7 @@ The 'C-x x' keymap now holds keystrokes for various buffer-oriented
 commands.  The new keystrokes are 'C-x x g' ('revert-buffer'),
 'C-x x r' ('rename-buffer'), 'C-x x u' ('rename-uniquely'), 'C-x x n'
 ('clone-buffer'), 'C-x x i' ('insert-buffer'), 'C-x x t'
-('toggle-truncate-lines') and 'C-x x f' ('font-lock-update').
+('toggle-truncate-lines') and 'C-x x f' ('font-lock-dwim').
 
 ---
 ** Commands 'set-frame-width' and 'set-frame-height' can now get their
diff --git a/lisp/bindings.el b/lisp/bindings.el
index 6eac528eb6..6f25e9738a 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -1432,7 +1432,7 @@ if `inhibit-field-text-motion' is non-nil."
 
 (defvar ctl-x-x-map
   (let ((map (make-sparse-keymap)))
-    (define-key map "f" #'font-lock-update)
+    (define-key map "f" #'font-lock-dwim)
     (define-key map "g" #'revert-buffer)
     (define-key map "r" #'rename-buffer)
     (define-key map "u" #'rename-uniquely)
diff --git a/lisp/font-lock.el b/lisp/font-lock.el
index 82915d8c8b..6b351d34ea 100644
--- a/lisp/font-lock.el
+++ b/lisp/font-lock.el
@@ -1120,15 +1120,19 @@ portion of the buffer."
     (funcall font-lock-ensure-function
              (or beg (point-min)) (or end (point-max)))))
 
-(defun font-lock-update (&optional arg)
+(defun font-lock-dwim (&optional arg)
   "Updates the syntax highlighting in this buffer.
-Refontify the accessible portion of this buffer, or enable Font Lock mode
-in this buffer if it is currently disabled.  With prefix ARG, toggle Font
-Lock mode."
+Enable Font Lock mode if it is disabled.  Otherwise, refontify the region
+if it is active, or a large part of the accessible portion of the buffer.
+With prefix ARG, toggle Font Lock mode."
   (interactive "P")
   (save-excursion
     (if (and (not arg) font-lock-mode)
-        (font-lock-fontify-region (point-min) (point-max))
+        (if (use-region-p)
+            (font-lock-fontify-region (region-beginning) (region-end))
+          (font-lock-flush (point-min) (point-max))
+          (font-lock-fontify-region (max (point-min) (- (point) 50000))
+                                    (min (point-max) (+ (point) 50000))))
       (font-lock-unfontify-region (point-min) (point-max))
       (font-lock-mode 'toggle))))
 
-- 
2.30.2

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Alan Mackenzie]

Hello, Gregory.

There's an unnecessary vagueness in your proposed doc string for
font-lock-dwim.

On Thu, Mar 25, 2021 at 10:14:06 +0000, Gregory Heytings wrote:

[ .... ]

> -(defun font-lock-update (&optional arg)
> +(defun font-lock-dwim (&optional arg)
>    "Updates the syntax highlighting in this buffer.
> -Refontify the accessible portion of this buffer, or enable Font Lock mode
> -in this buffer if it is currently disabled.  With prefix ARG, toggle Font
> -Lock mode."
> +Enable Font Lock mode if it is disabled.  Otherwise, refontify the region
> +if it is active, or a large part of the accessible portion of the buffer.
> +With prefix ARG, toggle Font Lock mode." <===========================
>    (interactive "P")
>    (save-excursion
>      (if (and (not arg) font-lock-mode)
> -        (font-lock-fontify-region (point-min) (point-max))
> +        (if (use-region-p)
> +            (font-lock-fontify-region (region-beginning) (region-end))
> +          (font-lock-flush (point-min) (point-max))
> +          (font-lock-fontify-region (max (point-min) (- (point) 50000))
> +                                    (min (point-max) (+ (point) 50000))))
>        (font-lock-unfontify-region (point-min) (point-max))
>        (font-lock-mode 'toggle))))

I think the sentence should read:

    With prefix ARG, INSTEAD toggle Font Lock mode.

..  The other meaning would be captured by:

    With prefix ARG, ALSO toggle Font Lock mode.

..  Please consider amending the doc string accordingly.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

> Hmmm...  You said: "`font-lock-fontify-region` is an important function
> internally (it's the one and only function which performs font-locking,
> fundamentally)".  Is it wrong to use it nonetheless?

That's the wrong question, since my beef is about the definition of what
the function should do, as opposed to how it's implemented.
[ Tho maybe I'd prefer if it used `font-lock-flush` or
  `font-lock-ensure`.  ]

>> I'd much prefer a longer `font-lock-fontify-diwm` which tries to reproduce
>> more or less the same behavior as your favorite, but by explicitly testing
>> the different circumstances you care about.
> Could you perhaps give an example of such a circumstance and its
> corresponding action?

The main "circumstances" that matter are whether the font-lock machinery
is activated and whether font-lock-mode is nil or not.  The rest is more
"obvious" (like whether it should pay attention to an active region, or
only refresh the visible portion of the buffer, ...).

Currently, I've heard mention of two use cases for font-lock-fontify-block:

  ;; - Correct misfontification when fontification is highly context-dependent
  ;;   and has a bug (typically associated with multiline constructs).
  ;;   `font-lock-flush` should work as well in that case.
  ;; - Removing fontification, e.g. when yanking font-locked strings into
  ;;   a text-mode buffer.  Not sure if the intention would be to generalize
  ;;   this to all buffers with a nil `font-lock-keywords` or also to buffers
  ;;   where font-lock-mode is disabled.

Maybe the docstring should describe just those behaviors, and the
implementation could then implement them explicitly, rather than have
that grow accidentally as a result of the implementation.


        Stefan

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Gregory Heytings]

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


>
> That's the wrong question, since my beef is about the definition of what 
> the function should do, as opposed to how it's implemented.
>

I think the simplest way to express what the function should do is: if 
something is wrong with the fontification, fix it.  (And with a prefix 
argument, toggle fontification.)

That's basically what Eli used font-lock-fontify-block for, for instance. 
Someone else said something like "I did not know that 
font-lock-fontify-block exists, when the fontification is wrong, I just 
turn font-lock-mode off and on".

>
> [ Tho maybe I'd prefer if it used `font-lock-flush` or 
> `font-lock-ensure`.  ]
>

Yes, but alas that doesn't work e.g. when yanking a font-locked string 
into a text-mode buffer.

>>> I'd much prefer a longer `font-lock-fontify-diwm` which tries to 
>>> reproduce more or less the same behavior as your favorite, but by 
>>> explicitly testing the different circumstances you care about.
>>
>> Could you perhaps give an example of such a circumstance and its 
>> corresponding action?
>
> The main "circumstances" that matter are whether the font-lock machinery 
> is activated and whether font-lock-mode is nil or not.
>

Are these two separate conditions?  Or does font-lock-mode t imply that 
the font-lock machinery is activated?  (Apart from the case where someone 
would do (progn (font-lock-mode -1) (setq font-lock-mode t)) of course.)

>
> Currently, I've heard mention of two use cases for 
> font-lock-fontify-block:
>
>  ;; - Correct misfontification when fontification is highly context-dependent
>  ;;  and has a bug (typically associated with multiline constructs).
>  ;;  `font-lock-flush` should work as well in that case.
>  ;; - Removing fontification, e.g. when yanking font-locked strings into
>  ;;  a text-mode buffer.  Not sure if the intention would be to generalize
>  ;;  this to all buffers with a nil `font-lock-keywords` or also to buffers
>  ;;  where font-lock-mode is disabled.
>

There is another use case I think, which is close to your first use case, 
and is perhaps the most common one: the fontification is not what you 
expect (say, you're inside a function and the fontification indicates 
you're inside a comment), and you are not sure whether it's because the 
fontification has not yet been updated, or because you indeed forgot to 
close a comment.  A refontifcation is useful in that case, too.

Another similar case is when you know that the fontification should change 
and do not want to wait for the refontification to take place (say, you 
open a comment, and want immediately see the effect).

In those case font-lock-flush should work well, too.

>
> Maybe the docstring should describe just those behaviors, and the 
> implementation could then implement them explicitly, rather than have 
> that grow accidentally as a result of the implementation.
>

Is a KISS approach not better?  Could it do something wrong?  What would 
be the benefit of identifying use cases one by one and adding them to a 
(presumably much more complex) function?

I attach an updated patch; it occurred to me that with outline-mode and 
friends, "far away from point" might not be enough to cover the window.

[-- Attachment #2: Type: text/x-diff, Size: 3481 bytes --]

From c77e6e67da31b918d92aacce1c86ac819cfd5fcc Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Thu, 25 Mar 2021 20:08:53 +0000
Subject: [PATCH] Improve font-lock-update and rename it to font-lock-dwim

* lisp/font-lock.el (font-lock-dwim): Renamed from 'font-lock-update'.
Only refontify the region when it is active.  Do not refontify too
large buffers at once.

* lisp/bindings.el (ctl-x-x-map): Adjust the command name.

* etc/NEWS: Adjust the command name.
---
 etc/NEWS          |  4 ++--
 lisp/bindings.el  |  2 +-
 lisp/font-lock.el | 14 +++++++++-----
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 68812c64cc..58cc4a2b82 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -94,7 +94,7 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 * Changes in Emacs 28.1
 
 +++
-** New command 'font-lock-update', bound to 'C-x x f'.
+** New command 'font-lock-dwim', bound to 'C-x x f'.
 This command updates the syntax highlighting in this buffer.
 
 ** The new NonGNU ELPA archive is enabled by default alongside GNU ELPA.
@@ -255,7 +255,7 @@ The 'C-x x' keymap now holds keystrokes for various buffer-oriented
 commands.  The new keystrokes are 'C-x x g' ('revert-buffer'),
 'C-x x r' ('rename-buffer'), 'C-x x u' ('rename-uniquely'), 'C-x x n'
 ('clone-buffer'), 'C-x x i' ('insert-buffer'), 'C-x x t'
-('toggle-truncate-lines') and 'C-x x f' ('font-lock-update').
+('toggle-truncate-lines') and 'C-x x f' ('font-lock-dwim').
 
 ---
 ** Commands 'set-frame-width' and 'set-frame-height' can now get their
diff --git a/lisp/bindings.el b/lisp/bindings.el
index 6eac528eb6..6f25e9738a 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -1432,7 +1432,7 @@ ctl-x-map
 
 (defvar ctl-x-x-map
   (let ((map (make-sparse-keymap)))
-    (define-key map "f" #'font-lock-update)
+    (define-key map "f" #'font-lock-dwim)
     (define-key map "g" #'revert-buffer)
     (define-key map "r" #'rename-buffer)
     (define-key map "u" #'rename-uniquely)
diff --git a/lisp/font-lock.el b/lisp/font-lock.el
index 82915d8c8b..1800f0b56d 100644
--- a/lisp/font-lock.el
+++ b/lisp/font-lock.el
@@ -1120,15 +1120,19 @@ font-lock-ensure
     (funcall font-lock-ensure-function
              (or beg (point-min)) (or end (point-max)))))
 
-(defun font-lock-update (&optional arg)
+(defun font-lock-dwim (&optional arg)
   "Updates the syntax highlighting in this buffer.
-Refontify the accessible portion of this buffer, or enable Font Lock mode
-in this buffer if it is currently disabled.  With prefix ARG, toggle Font
-Lock mode."
+Enable Font Lock mode if it is disabled.  Otherwise, refontify the region
+if it is active, or a large part of the accessible portion of the buffer.
+Otherwise, with prefix ARG, toggle Font Lock mode."
   (interactive "P")
   (save-excursion
     (if (and (not arg) font-lock-mode)
-        (font-lock-fontify-region (point-min) (point-max))
+        (if (use-region-p)
+            (font-lock-fontify-region (region-beginning) (region-end))
+          (font-lock-flush (point-min) (point-max))
+          (font-lock-fontify-region (max (point-min) (min (- (point) 50000) (window-start)))
+                                    (min (point-max) (max (+ (point) 50000) (window-end)))))
       (font-lock-unfontify-region (point-min) (point-max))
       (font-lock-mode 'toggle))))
 
-- 
2.30.2

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

>> [ Tho maybe I'd prefer if it used `font-lock-flush` or
>> `font-lock-ensure`.  ]
>
> Yes, but alas that doesn't work e.g. when yanking a font-locked string into
> a text-mode buffer.

And that's part of the problem: as it is the code can't be changed to
use those functions because it would behave slightly differently.
With a proper definition of what the behavior should be (as opposed to
it being just the result of the current implementation), we could write
the code so that it can use `font-lock-flush` when that gives the right
behavior and something else in other cases).

>>>> I'd much prefer a longer `font-lock-fontify-diwm` which tries to
>>>> reproduce more or less the same behavior as your favorite, but by
>>>> explicitly testing the different circumstances you care about.
>>>
>>> Could you perhaps give an example of such a circumstance and its
>>> corresponding action?
>>
>> The main "circumstances" that matter are whether the font-lock machinery
>> is activated and whether font-lock-mode is nil or not.
> Are these two separate conditions?

Yes, there are three cases:
- font-lock-mode is nil
- font-lock-mode is t but the font-lock machinery is not activated
  (e.g. in text-mode with the default config).
- font-lock fully activated.

>>  ;; - Correct misfontification when fontification is highly context-dependent
>>  ;;  and has a bug (typically associated with multiline constructs).
>>  ;;  `font-lock-flush` should work as well in that case.
>>  ;; - Removing fontification, e.g. when yanking font-locked strings into
>>  ;;  a text-mode buffer.  Not sure if the intention would be to generalize
>>  ;;  this to all buffers with a nil `font-lock-keywords` or also to buffers
>>  ;;  where font-lock-mode is disabled.
>
> There is another use case I think, which is close to your first use case,
> and is perhaps the most common one: the fontification is not what you expect
> (say, you're inside a function and the fontification indicates you're inside
> a comment), and you are not sure whether it's because the fontification has
> not yet been updated, or because you indeed forgot to close a comment.
> A refontifcation is useful in that case, too.

Yes, tho I think it's the same case as far as Emacs is concerned:
The difference is only in what motivated the user to launch the command
and how the user uses the result.

> Another similar case is when you know that the fontification should change
> and do not want to wait for the refontification to take place (say, you
> open a comment, and want immediately see the effect).

Indeed, and I guess this one is slightly different.

> In those case font-lock-flush should work well, too.

That's right.

>> Maybe the docstring should describe just those behaviors, and the
>> implementation could then implement them explicitly, rather than have that
>> grow accidentally as a result of the implementation.
> Is a KISS approach not better?  Could it do something wrong?

I don't like a command which is only right because it is defined to do
what it happens to do.
[ And in the case of `font-lock-fontify-buffer` it also makes it
  hard/impossible to make it more efficient: in many cases what is
  needed is just `font-lock-flush`, and in many other cases what is
  needed is `font-lock-ensure`, both of which can be significantly more
  efficient than `font-lock-fontify-buffer`.  But the function itself
  can't know which was meant.  ]

> -(defun font-lock-update (&optional arg)
> +(defun font-lock-dwim (&optional arg)
>    "Updates the syntax highlighting in this buffer.

BTW, This should say "Update" without a final "s".

> +Enable Font Lock mode if it is disabled.

Is this behavior useful?

> +Otherwise, refontify the region
> +if it is active, or a large part of the accessible portion of the buffer.

I don't see any mention of what should happen in a case like `text-mode`.

> +Otherwise, with prefix ARG, toggle Font Lock mode."

Is this behavior useful?

>    (interactive "P")
>    (save-excursion
>      (if (and (not arg) font-lock-mode)
> -        (font-lock-fontify-region (point-min) (point-max))
> +        (if (use-region-p)
> +            (font-lock-fontify-region (region-beginning) (region-end))

When font-lock is active and the region is about the refreshed
(e.g. a comment was just opened or something), this will result in
double font-locking (first it will happen now in response to this
command, and then it will happen again because this command did not tell
jit-lock that it just refreshed the area, so the planned refresh will
take place anyway).  And to make things worse, the second refresh may
re-introduce problems which were fixed by the first.

> +          (font-lock-flush (point-min) (point-max))
> +          (font-lock-fontify-region (max (point-min) (min (- (point) 50000) (window-start)))
> +                                    (min (point-max) (max (+ (point) 50000) (window-end)))))

Why use `font-lock-flush` and `font-lock-fontify-region`?

>        (font-lock-unfontify-region (point-min) (point-max))
>        (font-lock-mode 'toggle))))

If font-lock-mode was already activated, then (font-lock-mode 'toggle)
will call `font-lock-unfontify-region`, and if it's not then there's
nothing to unfontify, I think.  Or am I missing something?


        Stefan

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Gregory Heytings]

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


Thanks for your detailed reply!

>
> Yes, there are three cases: - font-lock-mode is nil - font-lock-mode is 
> t but the font-lock machinery is not activated (e.g. in text-mode with 
> the default config). - font-lock fully activated.
>

With this indication and another mail you sent to Eli, I think I finally 
understand what you mean.  One thing I'm not entirely sure is whether the 
second case is (and font-lock-mode (not font-lock-keywords)) or (and 
font-lock-mode (not font-lock-fontified)), but my guess is that 
font-lock-fontified is an internal variable and that it is safer to use 
font-lock-keywords here.

>> +Otherwise, with prefix ARG, toggle Font Lock mode."
>
> Is this behavior useful?
>

I think it is, yes, and I think it makes sense to use the prefix argument 
for that.  M-x font-lock-mode does not always produce the expected effect, 
which can be puzzling, so having a way to "do what I mean" in a command is 
useful.

I attach the patch, updated with your suggestions.

[-- Attachment #2: Type: text/x-diff, Size: 4248 bytes --]

From 49d758e5a91f19615fe1e7858b013c88d95da0a4 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 29 Mar 2021 09:31:15 +0000
Subject: [PATCH] Improve font-lock-update and rename it to font-lock-dwim

* lisp/font-lock.el (font-lock-dwim): Renamed from 'font-lock-update'.
Only refontify the region when it is active, and act more cautiously.

* list/font-lock.el (font-lock-update-region): New function that calls
the appropriate function depending on the current Font Lock state.

* lisp/bindings.el (ctl-x-x-map): Adjust the command name.

* etc/NEWS: Adjust the command name.
---
 etc/NEWS          |  4 ++--
 lisp/bindings.el  |  2 +-
 lisp/font-lock.el | 33 +++++++++++++++++++++++++--------
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 2d66a93474..4747d49ac5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -94,7 +94,7 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
 * Changes in Emacs 28.1
 
 +++
-** New command 'font-lock-update', bound to 'C-x x f'.
+** New command 'font-lock-dwim', bound to 'C-x x f'.
 This command updates the syntax highlighting in this buffer.
 
 ** The new NonGNU ELPA archive is enabled by default alongside GNU ELPA.
@@ -255,7 +255,7 @@ The 'C-x x' keymap now holds keystrokes for various buffer-oriented
 commands.  The new keystrokes are 'C-x x g' ('revert-buffer'),
 'C-x x r' ('rename-buffer'), 'C-x x u' ('rename-uniquely'), 'C-x x n'
 ('clone-buffer'), 'C-x x i' ('insert-buffer'), 'C-x x t'
-('toggle-truncate-lines') and 'C-x x f' ('font-lock-update').
+('toggle-truncate-lines') and 'C-x x f' ('font-lock-dwim').
 
 ---
 ** Commands 'set-frame-width' and 'set-frame-height' can now get their
diff --git a/lisp/bindings.el b/lisp/bindings.el
index 6eac528eb6..6f25e9738a 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -1432,7 +1432,7 @@ ctl-x-map
 
 (defvar ctl-x-x-map
   (let ((map (make-sparse-keymap)))
-    (define-key map "f" #'font-lock-update)
+    (define-key map "f" #'font-lock-dwim)
     (define-key map "g" #'revert-buffer)
     (define-key map "r" #'rename-buffer)
     (define-key map "u" #'rename-uniquely)
diff --git a/lisp/font-lock.el b/lisp/font-lock.el
index 82915d8c8b..4fb66fc45a 100644
--- a/lisp/font-lock.el
+++ b/lisp/font-lock.el
@@ -1120,16 +1120,33 @@ font-lock-ensure
     (funcall font-lock-ensure-function
              (or beg (point-min)) (or end (point-max)))))
 
-(defun font-lock-update (&optional arg)
-  "Updates the syntax highlighting in this buffer.
-Refontify the accessible portion of this buffer, or enable Font Lock mode
-in this buffer if it is currently disabled.  With prefix ARG, toggle Font
-Lock mode."
+(defun font-lock-update-region (&optional beg end)
+  "Update the syntax highlighting between BEG and END.
+Refontify depending on the current Font Lock mode state.
+Font Lock mode can be disabled, in which case the text is unfontified,
+or enabled but inactive, in which case the text is refontified,
+or enabled and active, in which case the text fontification is flushed.
+If the region is not specified, it defaults to the entire accessible
+portion of the current buffer."
+  (let ((b (or beg (point-min)))
+        (e (or end (point-max))))
+    (if (not font-lock-mode)
+        (font-lock-unfontify-region b e)
+      (if (not font-lock-keywords)
+          (font-lock-fontify-region b e)
+        (font-lock-flush b e)))))
+
+(defun font-lock-dwim (&optional arg)
+  "Update the syntax highlighting in this buffer.
+Refontify the region if it is active, or the accessible portion of the
+buffer.  Otherwise, with prefix ARG, toggle Font Lock mode."
   (interactive "P")
   (save-excursion
-    (if (and (not arg) font-lock-mode)
-        (font-lock-fontify-region (point-min) (point-max))
-      (font-lock-unfontify-region (point-min) (point-max))
+    (if (not arg)
+        (if (use-region-p)
+            (font-lock-update-region (region-beginning) (region-end))
+          (font-lock-update-region))
+      (font-lock-update-region)
       (font-lock-mode 'toggle))))
 
 (defun font-lock-default-fontify-buffer ()
-- 
2.30.2

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

> One thing I'm not entirely sure is whether the
> second case is (and font-lock-mode (not font-lock-keywords)) or (and
> font-lock-mode (not font-lock-fontified)), but my guess is that
> font-lock-fontified is an internal variable and that it is safer to use
> font-lock-keywords here.

I'm not entirely sure what is the best way to detect this
middle-point either.  The code that decides whether to activate the
font-lock machinery calls `font-lock-specified-p` for that, but maybe
there are corner cases where the machinery can be activated even when
`font-lock-specified-p` returns nil?  Similarly, I'm not sure if
`font-lock-fontified` is always non-nil when the font-lock machinery is
activated and always nil when it isn't.

IOW, someone needs to look carefully at the code to find out (and
presumably then document the result e.g. by adding a function that
returns this info, or with comments, or by adding a variable which
keeps track of this info or ...).

>>> +Otherwise, with prefix ARG, toggle Font Lock mode."
>> Is this behavior useful?
> I think it is, yes, and I think it makes sense to use the prefix argument
> for that.  M-x font-lock-mode does not always produce the expected effect,
> which can be puzzling, so having a way to "do what I mean" in a command
> is useful.

Could you describe what you mean by "does not always produce the
expected effect" here?  [ And maybe how the prefix ARG to
`font-lock-dwim` avoids those problems?  ]


        Stefan

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Gregory Heytings]

>> One thing I'm not entirely sure is whether the second case is (and 
>> font-lock-mode (not font-lock-keywords)) or (and font-lock-mode (not 
>> font-lock-fontified)), but my guess is that font-lock-fontified is an 
>> internal variable and that it is safer to use font-lock-keywords here.
>
> I'm not entirely sure what is the best way to detect this middle-point 
> either.  The code that decides whether to activate the font-lock 
> machinery calls `font-lock-specified-p` for that, but maybe there are 
> corner cases where the machinery can be activated even when 
> `font-lock-specified-p` returns nil?  Similarly, I'm not sure if 
> `font-lock-fontified` is always non-nil when the font-lock machinery is 
> activated and always nil when it isn't.
>
> IOW, someone needs to look carefully at the code to find out (and 
> presumably then document the result e.g. by adding a function that 
> returns this info, or with comments, or by adding a variable which keeps 
> track of this info or ...).
>

I guess that Someone^TM is me? ;-)  I'll have a look.

>>>> +Otherwise, with prefix ARG, toggle Font Lock mode."
>>>
>>> Is this behavior useful?
>>
>> I think it is, yes, and I think it makes sense to use the prefix 
>> argument for that.  M-x font-lock-mode does not always produce the 
>> expected effect, which can be puzzling, so having a way to "do what I 
>> mean" in a command is useful.
>
> Could you describe what you mean by "does not always produce the 
> expected effect" here?  [ And maybe how the prefix ARG to 
> `font-lock-dwim` avoids those problems?  ]
>

By "does not always produce the expected effect", I mean for example that 
M-x font-lock-mode in a text-mode or fundamental-mode buffer does not 
remove then fontification from a piece of code that was killed-yanked from 
a prog-mode buffer, and M-x font-lock-mode again (which re-enables font 
lock mode) still does not remove the fontification.  From a certain point 
of view, this is perhaps expected, but from a user point of view it is 
not.

With the prefix ARG, font-lock-update-region is called before toggling 
font-lock-mode, which ensures that the fontification is coherent with the 
current major-mode before disabling/enabling font-lock-mode.

Re: master 2399541: Remove font-lock toggle from font-lock-update

[Stefan Monnier]

>>> One thing I'm not entirely sure is whether the second case is (and
>>> font-lock-mode (not font-lock-keywords)) or (and font-lock-mode (not
>>> font-lock-fontified)), but my guess is that font-lock-fontified is an
>>> internal variable and that it is safer to use font-lock-keywords here.
>>
>> I'm not entirely sure what is the best way to detect this middle-point
>> either.  The code that decides whether to activate the font-lock machinery
>> calls `font-lock-specified-p` for that, but maybe there are corner cases
>> where the machinery can be activated even when `font-lock-specified-p`
>> returns nil?  Similarly, I'm not sure if `font-lock-fontified` is always
>> non-nil when the font-lock machinery is activated and always nil when it
>> isn't.
>>
>> IOW, someone needs to look carefully at the code to find out (and
>> presumably then document the result e.g. by adding a function that returns
>> this info, or with comments, or by adding a variable which keeps track of
>> this info or ...).
>>
>
> I guess that Someone^TM is me? ;-)  I'll have a look.

BTW, this relates to the distinction between "font-lock-mode, the minor
mode that control whether there is highlighting or not" and
"font-lock-mode, the most common way to apply highlighting when
requested".

Admittedly, it seems the only user of `font-lock-function` (the core
variable that allows taking advantage of this distinction to provide
a different machinery) is `ert.el` which just uses in a marginal way
that could probably be replace by the use of `font-lock-hook` or
something like that.

>>>>> +Otherwise, with prefix ARG, toggle Font Lock mode."
>>>>
>>>> Is this behavior useful?
>>>
>>> I think it is, yes, and I think it makes sense to use the prefix argument
>>> for that.  M-x font-lock-mode does not always produce the expected
>>> effect, which can be puzzling, so having a way to "do what I mean" in
>>> a command is useful.
>>
>> Could you describe what you mean by "does not always produce the expected
>> effect" here?  [ And maybe how the prefix ARG to `font-lock-dwim` avoids
>> those problems?  ]
>>
> By "does not always produce the expected effect", I mean for example that
> M-x font-lock-mode in a text-mode or fundamental-mode buffer does not
> remove then fontification from a piece of code that was killed-yanked from

I see.  IIUC, that's unrelated to the:

    Otherwise, with prefix ARG, toggle Font Lock mode

part of the command's behavior, right?


        Stefan