GNU bug report logs - #58518
29.0.50; [PATCH] Turning off compilation-minor-mode removes fontification of other modes

Previous Next

To reply to this bug, email your comments to 58518 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: miha <at> kamnitnik.top
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Turning off compilation-minor-mode removes
 fontification of other modes
Date: Fri, 14 Oct 2022 17:15:24 +0200

[Message part 1 (text/plain, inline)]

I propose the attached patch, which fixes the following problem:

1. M-x shell
2. grep -R -n --color=always 'some-search-string-which-yeilds-resuts'
   File names and line numbers output by grep should be in color
3. M-x compilation-shell-minor-mode
   File names and line numbers are now underlined by compile.el
4. M-x compilation-shell-minor-mode, to turn it back off

File names and line numbers lose their fontification completely, that
is, they are now in the default face.

The attached patch makes them return to the face they had after step 2,
as specified by the grep command. It also "name-spaces" other text
properties used by compile.el, such as keymap and mouse-face. If other
minor or major modes make use of these text properties, turning off
compilation-shell-minor-mode or compilation-minor-mode should now leave
them alone. (Though the ones from compile.el take precedence as long as
compilation-*-mode is active.)

[0001-compile-Don-t-clobber-text-properties-of-other-modes.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 58518 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: miha <at> kamnitnik.top
Cc: Eli Zaretskii <eliz <at> gnu.org>, 58518 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#58518: 29.0.50; [PATCH] Turning off compilation-minor-mode
 removes fontification of other modes
Date: Sat, 15 Oct 2022 12:25:32 +0200

miha <at> kamnitnik.top writes:

> The attached patch makes them return to the face they had after step 2,
> as specified by the grep command.
[...]

> -    `(font-lock-face ,(if leave
> -                          compilation-leave-directory-face
> -                        compilation-enter-directory-face)
> +    `(compilation-face ,(if leave
> +                            compilation-leave-directory-face

[...]

> +  (let ((alist (copy-alist char-property-alias-alist)))
> +    (cl-pushnew 'compilation-face (alist-get 'face alist))

Hm, it's an interesting approach -- we don't use this trick elsewhere in
other mode (I mean, except for `font-lock-face'), but this seems like a
good solution.

But since it's so unusual, I wonder whether there's any reason we don't
use this more in general.  Are there performance impacts, for instance?
(I don't think so, since it's a buffer-local variable.)  But I've added
Stefan and Eli to the CCs; perhaps they have comments.

Message #11 received at 58518 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 58518 <at> debbugs.gnu.org, miha <at> kamnitnik.top
Subject: Re: bug#58518: 29.0.50; [PATCH] Turning off compilation-minor-mode
 removes fontification of other modes
Date: Sat, 15 Oct 2022 10:45:28 -0400

> But since it's so unusual, I wonder whether there's any reason we don't
> use this more in general.  Are there performance impacts, for instance?
> (I don't think so, since it's a buffer-local variable.)  But I've added
> Stefan and Eli to the CCs; perhaps they have comments.

I can't remember anyone measuring the performance impact, but it does
come at a cost since every time we call `lookup_char_property` it takes
significantly more work.

This is called at every text-property (or overlay) boundary in things
like `next-single-property-change` and similar operations used by the
redisplay.

The "more work" is the `assq` itself whose time is proportional to the
length of this alist, plus a `plist-get` per alias listed, whenever
the `assq` finds a match (i.e. whenever we're looking for a property
which has aliases).

FWIW, in the past I suggested maybe we should introduce a notion of
"property planes".  So `compilation` could use one property plane,
`font-lock` could use another and they could just blindly remove all the
properties in their plane without affecting others.

The idea is similar to using the approach suggested by "miha", except
that the intention is to implement the hard work of merging the planes
in the code that adds/removes properties rather than in the code which
looks it up (and it would probably only apply to text properties, not to
overlays).
The idea of doing it when adding/removing properties is:
- Should keep the important lookup path used during redisplay fast.
- Should make it possible to run ELisp while merging, thus allowing
  "smart" merging (e.g. concatenating faces, or composing keymaps)
  rather than only choosing the value with highest priority and ignoring
  all the others.

BTW, another option is to use overlays rather than
text-properties :-)


        Stefan

Message #14 received at 58518 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 58518 <at> debbugs.gnu.org, miha <at> kamnitnik.top
Subject: Re: bug#58518: 29.0.50; [PATCH] Turning off compilation-minor-mode
 removes fontification of other modes
Date: Sun, 16 Oct 2022 10:24:51 +0200

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

> I can't remember anyone measuring the performance impact, but it does
> come at a cost since every time we call `lookup_char_property` it takes
> significantly more work.
>
> This is called at every text-property (or overlay) boundary in things
> like `next-single-property-change` and similar operations used by the
> redisplay.
>
> The "more work" is the `assq` itself whose time is proportional to the
> length of this alist, plus a `plist-get` per alias listed, whenever
> the `assq` finds a match (i.e. whenever we're looking for a property
> which has aliases).

But this would be for the *compilation* buffer only, so perhaps that's
fine...

> FWIW, in the past I suggested maybe we should introduce a notion of
> "property planes".  So `compilation` could use one property plane,
> `font-lock` could use another and they could just blindly remove all the
> properties in their plane without affecting others.

Yes, it's implementing planes, only manually.  Having real planes would
be great.

> BTW, another option is to use overlays rather than
> text-properties :-)

I'd rather not -- things are confusing enough in this area already.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.