Emacs revision #107149

Emacs revision #107149

[Alan Mackenzie]

Hello, Lars.

Why did you make this change:

  * progmodes/cc-mode.el
  (c-standard-font-lock-fontify-region-function): Set the default at
  load time, too, so that `font-lock-fontify-buffer' can be called
  without setting up the entire mode first.  This fixes a bug in
  `mm-inline-text' with C MIME parts.

without so much as a discussion on bug-cc-mode?  Or even emacs-devel?
cc-mode.el is my file.  I'm not happy about this change.

Is "the entire mode first" CC Mode first or font lock mode first?

Why, should the case be, would you want to call font-lock-fontify-buffer
without initialising font lock mode first?  Would this not, perhaps,
indicate some other problem in mm-inline-text?

Why, if the case may be, would you not want to initialise CC Mode?  This
is a genuine question, not a rhetorical one.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Emacs revision #107149

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> Why, should the case be, would you want to call font-lock-fontify-buffer
> without initialising font lock mode first?  Would this not, perhaps,
> indicate some other problem in mm-inline-text?

mm sets `font-lock-mode-hook' (and friends) to nil to avoid problems
when calling `(font-lock-fontify-buffer)', if I remember correctly.
Since it's that hook that sets
`c-standard-font-lock-fontify-region-function' (which is nil by
default), this breaks when viewing C mode files.

Is there any reason why `c-standard-font-lock-fontify-region-function'
is nil, when this makes just calling `(font-lock-fontify-buffer)' not
work?  All the other modes seem to work fine.

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: Emacs revision #107149

[Wolfgang Jenkner]

On Sat, Feb 11 2012, Lars Ingebrigtsen wrote:

> Alan Mackenzie <acm@muc.de> writes:
>
>> Why, should the case be, would you want to call font-lock-fontify-buffer
>> without initialising font lock mode first?  Would this not, perhaps,
>> indicate some other problem in mm-inline-text?
>
> mm sets `font-lock-mode-hook' (and friends) to nil to avoid problems
> when calling `(font-lock-fontify-buffer)', if I remember correctly.

I think the point is to inhibit anything that could delay fontification.

Please see (the whole) bug#10063 thread [1].  The bogus buffer local
binding of font-lock-mode-hook in mm-display-inline-fontify is still
there, by the way.

[1] http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10063

Wolfgang

Re: Emacs revision #107149

[Alan Mackenzie]

Hello, Lars.

On Sat, Feb 11, 2012 at 04:56:53PM +0100, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > Why, should the case be, would you want to call font-lock-fontify-buffer
> > without initialising font lock mode first?  Would this not, perhaps,
> > indicate some other problem in mm-inline-text?

> mm sets `font-lock-mode-hook' (and friends) to nil to avoid problems
> when calling `(font-lock-fontify-buffer)', if I remember correctly.

You cannot get away with this.  These hooks belong to the major mode (or
perhaps the user), and if you arbitrarily inhibit them, then font lock,
in the general case, will not be fully initialised.

> Since it's that hook that sets
> `c-standard-font-lock-fontify-region-function' (which is nil by
> default), this breaks when viewing C mode files.

Again, why are you breaking these hook calls?  This seems to be a very
bad solution to whatever the problem was.

> Is there any reason why `c-standard-font-lock-fontify-region-function'
> is nil, when this makes just calling `(font-lock-fontify-buffer)' not
> work?

font-lock-fontify-region will work fine if you just initialise font lock
fully.  That involves running the hook.

> All the other modes seem to work fine.

Then you've been lucky, so far.  What in mm gets upset by running
font-lock-mode-hook?

Setting that variable to font-lock-default-fontify-region at build time
couples CC Mode and font lock mode too closely.  In particular, it will
prevent CC Mode loading on any system in which font lock is not present.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Emacs revision #107149

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> You cannot get away with this.  These hooks belong to the major mode (or
> perhaps the user), and if you arbitrarily inhibit them, then font lock,
> in the general case, will not be fully initialised.

As Wolfgang Jenkner said, I think it has something to do with disabling
the delayed fontification modes.  But I'm not actually sure -- the code
has been that way forever, and I probably didn't write it.

> Again, why are you breaking these hook calls?  This seems to be a very
> bad solution to whatever the problem was.

It works for all modes tested, except C mode, apparently.

>> Is there any reason why `c-standard-font-lock-fontify-region-function'
>> is nil, when this makes just calling `(font-lock-fontify-buffer)' not
>> work?
>
> font-lock-fontify-region will work fine if you just initialise font lock
> fully.  That involves running the hook.

So there is no reason?

> Setting that variable to font-lock-default-fontify-region at build time
> couples CC Mode and font lock mode too closely.  In particular, it will
> prevent CC Mode loading on any system in which font lock is not present.

What systems would this be?

As far as I can tell from the cc-mode code, `c-font-lock-fontify-region'
unconditionally calls `c-standard-font-lock-fontify-region-function'.
Furthermore, `c-standard-font-lock-fontify-region-function' doesn't seem
to be set to anything other than `(default-value
'font-lock-fontify-region-function)'.  And the variable is not a
user-level variable, which seems to make the entire tap-dance routine
here rather ... odd.

Why not just call `(default-value 'font-lock-fontify-region-function)'
unconditionally?

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: Emacs revision #107149

[Alan Mackenzie]

Evening, Lars.

On Sun, Feb 12, 2012 at 09:42:59PM +0100, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > You cannot get away with this.  These hooks belong to the major mode (or
> > perhaps the user), and if you arbitrarily inhibit them, then font lock,
> > in the general case, will not be fully initialised.

> As Wolfgang Jenkner said, I think it has something to do with disabling
> the delayed fontification modes.  But I'm not actually sure -- the code
> has been that way forever, and I probably didn't write it.

Try this instead: It solves the actual problem stated in the comment,
rather than papering over it:


*** mm-view.el~	2012-02-08 14:25:18.000000000 +0000
--- mm-view.el	2012-02-12 21:50:34.000000000 +0000
***************
*** 601,610 ****
        (require 'font-lock)
        ;; I find font-lock a bit too verbose.
        (let ((font-lock-verbose nil)
! 	    (font-lock-support-mode nil))
  	;; Disable support modes, e.g., jit-lock, lazy-lock, etc.
  	;; Note: XEmacs people use `font-lock-mode-hook' to run those modes.
! 	(set (make-local-variable 'font-lock-mode-hook) nil)
          (setq buffer-file-name (mm-handle-filename handle))
          (set (make-local-variable 'enable-local-variables) nil)
  	(with-demoted-errors
--- 601,612 ----
        (require 'font-lock)
        ;; I find font-lock a bit too verbose.
        (let ((font-lock-verbose nil)
! 	    (font-lock-support-mode nil)
! 	    (font-lock-mode-hook font-lock-mode-hook))
  	;; Disable support modes, e.g., jit-lock, lazy-lock, etc.
  	;; Note: XEmacs people use `font-lock-mode-hook' to run those modes.
! 	(remove-hook 'font-lock-mode-hook 'turn-on-fast-lock)
! 	(remove-hook 'font-lock-mode-hook 'turn-on-lazy-lock)
          (setq buffer-file-name (mm-handle-filename handle))
          (set (make-local-variable 'enable-local-variables) nil)
  	(with-demoted-errors

But, do you actually have to suppress these support modes?  They're there
to fontify text screens faster.  Why disable them?  Nowadays, with faster
HW than ever, it seems pointless even to bother.  You've admitted, I
think, that you don't know why that code is there.  You could just rip it
out.

> > Again, why are you breaking these hook calls?  This seems to be a very
> > bad solution to whatever the problem was.

> It works for all modes tested, except C mode, apparently.

How do you know?  font-lock-mode-hook is used by hi-lock mode,
cperl-mode, sql mode, ada mode, verilog mode, and CC Mode.  One of these
is definitely being broken.  The others probably are, if you ever use
them.

The point is, you are reaching into other major modes and messing them
up to an unknown extent.  This is not good programming practice.

> > Setting that variable to font-lock-default-fontify-region at build time
> > couples CC Mode and font lock mode too closely.  In particular, it will
> > prevent CC Mode loading on any system in which font lock is not present.

> What systems would this be?

XEmacs, of course, which is the only system you need to mask out
turn-on-lazy-lock and friends for.

> As far as I can tell from the cc-mode code, `c-font-lock-fontify-region'
> unconditionally calls `c-standard-font-lock-fontify-region-function'.
> Furthermore, `c-standard-font-lock-fontify-region-function' doesn't seem
> to be set to anything other than `(default-value
> 'font-lock-fontify-region-function)'.  And the variable is not a
> user-level variable, which seems to make the entire tap-dance routine
> here rather ... odd.

> Why not just call `(default-value 'font-lock-fontify-region-function)'
> unconditionally?

It's unclean programming, and won't work properly under XEmacs.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Emacs revision #107149

[Wolfgang Jenkner]

On Sun, Feb 12 2012, Alan Mackenzie wrote:

> But, do you actually have to suppress these support modes?  They're there
> to fontify text screens faster.  Why disable them?  Nowadays, with faster
> HW than ever, it seems pointless even to bother.

The buffer where the fontification happens is not displayed, instead its
content is inserted in a buffer with a different major-mode, and in
between emacs stays never idle for the font-lock support functions to
kick in.


Wolfgang

Re: Emacs revision #107149

[Alan Mackenzie]

'evening, Wolfgang.

On Mon, Feb 13, 2012 at 02:00:43AM +0100, Wolfgang Jenkner wrote:
> On Sun, Feb 12 2012, Alan Mackenzie wrote:

> > But, do you actually have to suppress these support modes?  They're there
> > to fontify text screens faster.  Why disable them?  Nowadays, with faster
> > HW than ever, it seems pointless even to bother.

> The buffer where the fontification happens is not displayed, instead its
> content is inserted in a buffer with a different major-mode, and in
> between emacs stays never idle for the font-lock support functions to
> kick in.

I'm not sure I've fully understood you here.  Certainly in JIT lock mode
(the default under GNU Emacs), and probably under lazy-lock too, "no"
runtime is consumed until the buffer is displayed.
font-lock-fontify-buffer (without a support mode) will take some time,
sometimes quite a lot.

> Wolfgang

Alan Mackenzie (Nürnberg).

Re: Emacs revision #107149

[Stefan Monnier]

> Try this instead: It solves the actual problem stated in the comment,
> rather than papering over it:

> *** mm-view.el~	2012-02-08 14:25:18.000000000 +0000
> --- mm-view.el	2012-02-12 21:50:34.000000000 +0000
> ***************
> *** 601,610 ****
>         (require 'font-lock)
>         ;; I find font-lock a bit too verbose.
>         (let ((font-lock-verbose nil)
> ! 	    (font-lock-support-mode nil))
>   	;; Disable support modes, e.g., jit-lock, lazy-lock, etc.
>   	;; Note: XEmacs people use `font-lock-mode-hook' to run those modes.
> ! 	(set (make-local-variable 'font-lock-mode-hook) nil)
>           (setq buffer-file-name (mm-handle-filename handle))
>           (set (make-local-variable 'enable-local-variables) nil)
>   	(with-demoted-errors
> --- 601,612 ----
>         (require 'font-lock)
>         ;; I find font-lock a bit too verbose.
>         (let ((font-lock-verbose nil)
> ! 	    (font-lock-support-mode nil)
> ! 	    (font-lock-mode-hook font-lock-mode-hook))
>   	;; Disable support modes, e.g., jit-lock, lazy-lock, etc.
>   	;; Note: XEmacs people use `font-lock-mode-hook' to run those modes.
> ! 	(remove-hook 'font-lock-mode-hook 'turn-on-fast-lock)
> ! 	(remove-hook 'font-lock-mode-hook 'turn-on-lazy-lock)
>           (setq buffer-file-name (mm-handle-filename handle))
>           (set (make-local-variable 'enable-local-variables) nil)
>   	(with-demoted-errors

Actually, a real fix to this problem involves adding new a font-lock
function, which I like to call font-lock-ensure-fontification, which
mm-view would then call to force immediate fontification of the
specified region.  This function would know how to handle jit-lock,
lazy-lock, and whichever other accelerator we come up with.

Such a functionality is also needed in other packages, such as
in htmlfontify.  We have a bug report for this, but I can't find it
any more.


        Stefan

Re: Emacs revision #107149

[Alan Mackenzie]

On Sun, Feb 12, 2012 at 10:15:18PM -0500, Stefan Monnier wrote:

> Actually, a real fix to this problem involves adding new a font-lock
> function, which I like to call font-lock-ensure-fontification, which
> mm-view would then call to force immediate fontification of the
> specified region.  This function would know how to handle jit-lock,
> lazy-lock, and whichever other accelerator we come up with.

I Don't understand.  Why is the forcing of immediate fontification ever
necessary?  That's what JIT lock does, isn't it?

> Such a functionality is also needed in other packages, such as
> in htmlfontify.  We have a bug report for this, but I can't find it
> any more.


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Emacs revision #107149

[Stefan Monnier]

> I Don't understand.  Why is the forcing of immediate fontification ever
> necessary?  That's what JIT lock does, isn't it?

JIT lock is only involved when the buffer is displayed directly.
In htmlfontify or ps-print, we need to fontify the buffer even if it's
not displayed because the output (HTML or Postscript) wants to include
the fact info that font-lock adds.

In the mm-view case it's similar: the buffer itself is not displayed,
instead its content is copied into some other buffer (which uses
another major-mode).  We could setup this other buffer so that jit-lock
there is redirected to apply font-lock to the internal buffer after
which the result is copied back into the displayed buffer, but instead
mm-view just font-locks the whole buffer eagerly, which is simpler, tho
it loses the laziness of jit-lock.


        Stefan

Re: Emacs revision #107149

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

>> As far as I can tell from the cc-mode code, `c-font-lock-fontify-region'
>> unconditionally calls `c-standard-font-lock-fontify-region-function'.
>> Furthermore, `c-standard-font-lock-fontify-region-function' doesn't seem
>> to be set to anything other than `(default-value
>> 'font-lock-fontify-region-function)'.  And the variable is not a
>> user-level variable, which seems to make the entire tap-dance routine
>> here rather ... odd.
>
>> Why not just call `(default-value 'font-lock-fontify-region-function)'
>> unconditionally?
>
> It's unclean programming, and won't work properly under XEmacs.

You may well be right, but looking at the code, I don't really see why.
There's one single consumer of
`c-standard-font-lock-fontify-region-function'?  There's one single
place it's set?  If it isn't set, the function breaks unconditionally?

Or is it set to something else under XEmacs (in code that's not in the
Emacs 24 tree)?

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: Emacs revision #107149

[Alan Mackenzie]

Hi, Lars.

On Mon, Feb 13, 2012 at 10:03:14PM +0100, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> >> Why not just call `(default-value 'font-lock-fontify-region-function)'
> >> unconditionally?

> > It's unclean programming, and won't work properly under XEmacs.

> You may well be right, but looking at the code, I don't really see why.
> There's one single consumer of
> `c-standard-font-lock-fontify-region-function'?  There's one single
> place it's set?  If it isn't set, the function breaks unconditionally?

> Or is it set to something else under XEmacs (in code that's not in the
> Emacs 24 tree)?

No, it's identical in XEmacs.

Anyhow, you've neglected to address the main points in my last post.
Would you please do so now:


>>Try this instead: It solves the actual problem stated in the comment,
>>rather than papering over it:


>>*** mm-view.el~	2012-02-08 14:25:18.000000000 +0000
>>--- mm-view.el	2012-02-12 21:50:34.000000000 +0000
>>***************
>>*** 601,610 ****
>>        (require 'font-lock)
>>        ;; I find font-lock a bit too verbose.
>>        (let ((font-lock-verbose nil)
>>! 	    (font-lock-support-mode nil))
>>  	;; Disable support modes, e.g., jit-lock, lazy-lock, etc.
>>  	;; Note: XEmacs people use `font-lock-mode-hook' to run those modes.
>>! 	(set (make-local-variable 'font-lock-mode-hook) nil)
>>          (setq buffer-file-name (mm-handle-filename handle))
>>          (set (make-local-variable 'enable-local-variables) nil)
>>  	(with-demoted-errors
>>--- 601,612 ----
>>        (require 'font-lock)
>>        ;; I find font-lock a bit too verbose.
>>        (let ((font-lock-verbose nil)
>>! 	    (font-lock-support-mode nil)
>>! 	    (font-lock-mode-hook font-lock-mode-hook))
>>  	;; Disable support modes, e.g., jit-lock, lazy-lock, etc.
>>  	;; Note: XEmacs people use `font-lock-mode-hook' to run those modes.
>>! 	(remove-hook 'font-lock-mode-hook 'turn-on-fast-lock)
>>! 	(remove-hook 'font-lock-mode-hook 'turn-on-lazy-lock)
>>          (setq buffer-file-name (mm-handle-filename handle))
>>          (set (make-local-variable 'enable-local-variables) nil)
>>  	(with-demoted-errors

So, how about this patch?  Would it work for what you need to do?  As
yet, I've no clear understanding of what your mode is trying to do here,
or why.  I'd appreciate you filling in the gaps for me.

>>But, do you actually have to suppress these support modes?  They're there
>>to fontify text screens faster.  Why disable them?  Nowadays, with faster
>>HW than ever, it seems pointless even to bother.  You've admitted, I
>>think, that you don't know why that code is there.  You could just rip it
>>out.

>>> > Again, why are you breaking these hook calls?  This seems to be a very
>>> > bad solution to whatever the problem was.

>>> It works for all modes tested, except C mode, apparently.

>>How do you know?  font-lock-mode-hook is used by hi-lock mode,
>>cperl-mode, sql mode, ada mode, verilog mode, and CC Mode.  One of these
>>is definitely being broken.  The others probably are, if you ever use
>>them.

Have you individually checked any of the other modes you might use, to
make sure that they don't get fouled up by not calling their
font-lock-mode-hooks?

>>The point is, you are reaching into other major modes and messing them
>>up to an unknown extent.  This is not good programming practice.

This is the main point.  You are knowingly sabotaging the initialisation
of modes that you use.  This is bad.  Please respond to this point.


Evening, Lars.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Emacs revision #107149

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> Anyhow, you've neglected to address the main points in my last post.
> Would you please do so now:

I thought Stefan and Wolfgang had answered the question.

The fontification takes place in one buffer, and the result is then
copied over to a different buffer.

> Have you individually checked any of the other modes you might use, to
> make sure that they don't get fouled up by not calling their
> font-lock-mode-hooks?

There have been no bug reports on other modes; just C mode.

You still haven't responded to my question of why CC mode does the
initialisation of this variable in this rather baroque way:

> You may well be right, but looking at the code, I don't really see why.
> There's one single consumer of
> `c-standard-font-lock-fontify-region-function'?  There's one single
> place it's set?  If it isn't set, the function breaks unconditionally?

The code does not seem to make sense as it is, but I may well be
misreading it.

-- 
(domestic pets only, the antidote for overdose, milk.)
  http://lars.ingebrigtsen.no  *  Sent from my Rome

Re: Emacs revision #107149

[Alan Mackenzie]

Hi, Lars.

On Mon, Feb 20, 2012 at 10:51:31AM +0100, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> The fontification takes place in one buffer, and the result is then
> copied over to a different buffer.

OK, I've got this now.  ;-)

> > Have you individually checked any of the other modes you might use, to
> > make sure that they don't get fouled up by not calling their
> > font-lock-mode-hooks?

> There have been no bug reports on other modes; just C mode.

That's scarcely the point.  It's an accident waiting to happen again,
just as it has already happened once.

> You still haven't responded to my question of why CC mode does the
> initialisation of this variable in this rather baroque way:

> > You may well be right, but looking at the code, I don't really see why.
> > There's one single consumer of
> > `c-standard-font-lock-fontify-region-function'?  There's one single
> > place it's set?  If it isn't set, the function breaks unconditionally?

If font-lock-fontify-region-function doesn't exist (font-lock-mode hasn't
(yet) been loaded (xemacs)), trying to access its value will throw an
error.

> The code does not seem to make sense as it is, but I may well be
> misreading it.

The reason for inhibiting font-lock-mode-hook in
mm-view-display-inline-fontify is that otherwise one of two functions
(turn-on-lazy-lock, turn-on-fast-lock) might get called, and that
font-lock-support-mode would prevent the desired immediate fontification
of the whole buffer.  Solely in XEmacs, of course.

Therefore, you don't need to switch off the entire font-lock-mode-hook;
you merely have to remove these two functions from it, which can be done
very simply by binding font-lock-mode-hook and using remove-hook.

Again, please try my patch (after putting `copy-list' into the binding of
font-lock-mode-hook ;-).

-- 
Alan Mackenzie (Nuremberg, Germany).