Question: add-face-text-property for 'font-lock-face?

Question: add-face-text-property for 'font-lock-face?

[Qiantan Hong]

Users of crdt.el report that crdt-visualize-author-mode (which use font-lock-face to color code author of texts)
conflict with faces from other modes. Apparently there are more than 1 mode which sets this text property.

Is there some way to add-face-text-property to 'font-lock-face property, 
i.e. adding new face attributes instead of overwriting any existing face?
add-face-text-property doesn’t seem to work because 'font-lock-face will just override ‘face.

Best,
Qiantan

Re: Question: add-face-text-property for 'font-lock-face?

[Yuan Fu]

> On Aug 28, 2021, at 9:13 PM, Qiantan Hong <qhong@mit.edu> wrote:
> 
> Users of crdt.el report that crdt-visualize-author-mode (which use font-lock-face to color code author of texts)
> conflict with faces from other modes. Apparently there are more than 1 mode which sets this text property.
> 
> Is there some way to add-face-text-property to 'font-lock-face property, 
> i.e. adding new face attributes instead of overwriting any existing face?
> add-face-text-property doesn’t seem to work because 'font-lock-face will just override ‘face.
> 
> Best,
> Qiantan
> 

Is there anything preventing you from using overlays? They seem to be perfect for the job.

Yuan

Question: add-face-text-property for 'font-lock-face?

[Qiantan Hong]

That’s an option, but I’m worried about performance because crdt-visuallize-author-mode
need to color code the whole document and that might be too many overlays.
Maybe one can lazily create overlay for visible part only but I feel like
at that point I’ll probably just reinvent an adhoc add-font-lock-face-text-property in Elisp.

> On Aug 28, 2021, at 9:24 PM, Yuan Fu <casouri@gmail.com> wrote:
> 
> 
> 
>> On Aug 28, 2021, at 9:13 PM, Qiantan Hong <qhong@mit.edu> wrote:
>> 
>> Users of crdt.el report that crdt-visualize-author-mode (which use font-lock-face to color code author of texts)
>> conflict with faces from other modes. Apparently there are more than 1 mode which sets this text property.
>> 
>> Is there some way to add-face-text-property to 'font-lock-face property, 
>> i.e. adding new face attributes instead of overwriting any existing face?
>> add-face-text-property doesn’t seem to work because 'font-lock-face will just override ‘face.
>> 
>> Best,
>> Qiantan
>> 
> 
> Is there anything preventing you from using overlays? They seem to be perfect for the job.
> 
> Yuan

Re: Question: add-face-text-property for 'font-lock-face?

[Ihor Radchenko]

Qiantan Hong <qhong@mit.edu> writes:

> That’s an option, but I’m worried about performance because crdt-visuallize-author-mode
> need to color code the whole document and that might be too many overlays.
> Maybe one can lazily create overlay for visible part only but I feel like
> at that point I’ll probably just reinvent an adhoc add-font-lock-face-text-property in Elisp.

What about hooking directly into font-lock? If you change faces as part
of font-locking process, they will be properly transferred into
'font-lock-face property.  Fontifying within font-lock framework will
also benefit form lazy fontification when jit-lock-mode is enabled.

Best,
Ihor

Re: Question: add-face-text-property for 'font-lock-face?

[Qiantan Hong]

I think in reality, probably not that many overlays are strictly required,
because each collaborators tend to author in consecutive chunks.
However it does require some more implementation effort to make it work,
because we need to color code every newly inserted text based on
its author. The naive way that create an overlay for each character
clearly doesn’t work (text property however works, because Emacs itself
always merge them). We would need to look at overlays around the insertion
point and opportunistically merge them.

I feel like using text property will be cleaner, but let’s see if there’s a right way.


> On Aug 28, 2021, at 9:39 PM, Yuan Fu <casouri@gmail.com> wrote:
> 
> 
> 
>> On Aug 28, 2021, at 9:36 PM, Qiantan Hong <qhong@mit.edu> wrote:
>> 
>> That’s an option, but I’m worried about performance because crdt-visuallize-author-mode
>> need to color code the whole document and that might be too many overlays.
>> Maybe one can lazily create overlay for visible part only but I feel like
>> at that point I’ll probably just reinvent an adhoc add-font-lock-face-text-property in Elisp.
> 
> I don’t think you need to worry about performance beofore there are thousands of overlays. Will there be that many?
> 
> Yuan

Re: Question: add-face-text-property for 'font-lock-face?

[Stefan Monnier]

> its author. The naive way that create an overlay for each character
> clearly doesn’t work (text property however works, because Emacs itself
> always merge them). We would need to look at overlays around the insertion
> point and opportunistically merge them.

IIRC we have pretty much exactly that need in outline.el where we use
overlays to put an `invisible` property.  See `outline-flag-region`.
AFAICT this code currently doesn't try and merge overlays upon
insertion, but it shouldn't be hard to add that functionality.


        Stefan

Re: Question: add-face-text-property for 'font-lock-face?

[Eli Zaretskii]

> From: Qiantan Hong <qhong@mit.edu>
> Date: Sun, 29 Aug 2021 04:13:40 +0000
> Accept-Language: en-US
> 
> Users of crdt.el report that crdt-visualize-author-mode (which use font-lock-face to color code author of texts)
> conflict with faces from other modes. Apparently there are more than 1 mode which sets this text property.
> 
> Is there some way to add-face-text-property to 'font-lock-face property, 
> i.e. adding new face attributes instead of overwriting any existing face?
> add-face-text-property doesn’t seem to work because 'font-lock-face will just override ‘face.

I don't think I understand well enough what you want to do, but in
general, you change a face's attribute by using set-face-attribute.

Re: Question: add-face-text-property for 'font-lock-face?

[Qiantan Hong]

I need to add some face attribute to a range of text.
And in case that font-lock-mode is active, the attribute needs to be
added to ‘font-lock-face instead of ‘face text property.

> I don't think I understand well enough what you want to do, but in
> general, you change a face's attribute by using set-face-attribute.
That doesn’t seem to work for anonymous faces.
I don’t want to globally modify other modes’ faces,
I want to add some attribute over some range of text.

add-face-text-property seems to exactly do this,
but it’s hardcoded for ‘face property in C code

textprop.c L1355:
>   AUTO_LIST2 (properties, Qface, face);
>   add_text_properties_1 (start, end, properties, object,
> 			 (NILP (append)
> 			  ? TEXT_PROPERTY_PREPEND
> 			  : TEXT_PROPERTY_APPEND),
> 			 false);
>   return Qnil;


It’s a trivial modification to support attributes other than face.
However it requires a patch to C code.
For now I think I’ll just workaround it by reinventing
the same functionality in Elisp.

Re: Question: add-face-text-property for 'font-lock-face?

[Eli Zaretskii]

> From: Qiantan Hong <qhong@mit.edu>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sun, 29 Aug 2021 07:19:03 +0000
> 
> I need to add some face attribute to a range of text.
> And in case that font-lock-mode is active, the attribute needs to be
> added to ‘font-lock-face instead of ‘face text property.
> 
> > I don't think I understand well enough what you want to do, but in
> > general, you change a face's attribute by using set-face-attribute.
> That doesn’t seem to work for anonymous faces.
> I don’t want to globally modify other modes’ faces,
> I want to add some attribute over some range of text.
> 
> add-face-text-property seems to exactly do this,
> but it’s hardcoded for ‘face property in C code

A face property is just a list of keyword-value pairs, so adding an
attribute to the list should be trivial, no?  Or what am I missing?

> textprop.c L1355:
> >   AUTO_LIST2 (properties, Qface, face);
> >   add_text_properties_1 (start, end, properties, object,
> > 			 (NILP (append)
> > 			  ? TEXT_PROPERTY_PREPEND
> > 			  : TEXT_PROPERTY_APPEND),
> > 			 false);
> >   return Qnil;
> 
> 
> It’s a trivial modification to support attributes other than face.
> However it requires a patch to C code.
> For now I think I’ll just workaround it by reinventing
> the same functionality in Elisp.

Adding something to a list is so simple you don't need to invent
anything, I think.

Re: Question: add-face-text-property for 'font-lock-face?

[Clément Pit-Claudel]

On 8/29/21 3:49 AM, Eli Zaretskii wrote:
>> From: Qiantan Hong <qhong@mit.edu>
>> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
>> Date: Sun, 29 Aug 2021 07:19:03 +0000
>>
>> I need to add some face attribute to a range of text.
>> And in case that font-lock-mode is active, the attribute needs to be
>> added to ‘font-lock-face instead of ‘face text property.
>>
>>> I don't think I understand well enough what you want to do, but in
>>> general, you change a face's attribute by using set-face-attribute.
>> That doesn’t seem to work for anonymous faces.
>> I don’t want to globally modify other modes’ faces,
>> I want to add some attribute over some range of text.
>>
>> add-face-text-property seems to exactly do this,
>> but it’s hardcoded for ‘face property in C code

> A face property is just a list of keyword-value pairs, so adding an
> attribute to the list should be trivial, no?  Or what am I missing?

You still need to iterate over regions of continuous text properties; it's not a complicated program, but it's not trivial either.

(Qiantan is hoping to preserve existing font-lock faces and append the CRDT.el face to them, AFAIU; so, it's a bit more work than appending to one list: you need to iterate over the region of interest, and append to each list of faces).

And if I understand correctly, then font-lock-append-text-property should fit the bill nicely.

Re: Question: add-face-text-property for 'font-lock-face?

[Qiantan Hong]

> You want font-lock-append-text-property, I think:
> 
>  (font-lock-append-text-property START END PROP VALUE &optional OBJECT)

I think rather than hardcode a C function for another property, we could just
expose the general function the C implementation now is internally using
(add-text-property-1 START END PROP OBJECT MODE DESTRUCTIVE)

However, sorry for my own confusion, I found out this actually doesn’t solve my problem.
Recall the problem:
> Users of crdt.el report that crdt-visualize-author-mode (which use font-lock-face to color code author of texts)
> conflict with faces from other modes. Apparently there are more than 1 mode which sets this text property.
In fact in most cases those other mode doesn’t set the ‘font-lock-face property,
the problem is ‘font-lock-face property (aka recalculated fontification) I set overrides other font lock mechanism.

Is there a right way to add attributes while preserving font lock faces from other mode?
I now see that Yuan’s suggestion of overlay will do this, but there’s performance concern.

Re: Question: add-face-text-property for 'font-lock-face?

[Stefan Monnier]

> I now see that Yuan’s suggestion of overlay will do this, but there’s
> performance concern.

The performance concern is largely hypothetical, and if it ever proves
real, we have a branch waiting to be merged that reimplement overlays
in a more efficient (algorithmically) way.


        Stefan

Re: Question: add-face-text-property for 'font-lock-face?

[Qiantan Hong]

> IIRC we have pretty much exactly that need in outline.el where we use
> overlays to put an `invisible` property.  See `outline-flag-region`.
> AFAICT this code currently doesn't try and merge overlays upon
> insertion, but it shouldn't be hard to add that functionality.
It seems that outline.el always call outline-flag-region on chunk of texts
(an entry, a subtree, etc). On the other hand crdt.el need to add property
to every modification received from remote peer, lots of them will contain
only single character. Therefore I don’t performance concern is hypothetical
for naive handling (without overlay merging). This can easily result in
> 10k overlays in the worst case for a medium sized buffer.

> The performance concern is largely hypothetical, and if it ever proves
> real, we have a branch waiting to be merged that reimplement overlays
> in a more efficient (algorithmically) way.
Thanks for the information. I’ll implement using overlay+merging then.
It will probably work for most cases for now, and let that branch fix
the pathological case where user inputs largely interleaves in the future.
(The case sounds implausible, but it could happen, e.g. when
using crdt.el to share an artist-mode buffer)

Re: Question: add-face-text-property for 'font-lock-face?

[Ihor Radchenko]

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

>> I now see that Yuan’s suggestion of overlay will do this, but there’s
>> performance concern.
>
> The performance concern is largely hypothetical, and if it ever proves
> real, we have a branch waiting to be merged that reimplement overlays
> in a more efficient (algorithmically) way.

It is not hypothetical [1]. I have reported issues with overlays in
org-mode in the past. Also, from my experience, large number of overlays
appear to cause garbage collection issues and overall Emacs slowdown.

Is the overlay branch ready to be merged? I am only aware about yet
another effort to revive it back in 2019 [2]. Is there any progress
since then?

Best,
Ihor


[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2019-05/msg00206.html
[2] https://lists.gnu.org/archive/html/emacs-devel/2019-12/msg00323.html

Re: Question: add-face-text-property for 'font-lock-face?

[Clément Pit-Claudel]

On 8/29/21 12:13 AM, Qiantan Hong wrote:
> Users of crdt.el report that crdt-visualize-author-mode (which use font-lock-face to color code author of texts)
> conflict with faces from other modes. Apparently there are more than 1 mode which sets this text property.
> 
> Is there some way to add-face-text-property to 'font-lock-face property, 
> i.e. adding new face attributes instead of overwriting any existing face?
> add-face-text-property doesn’t seem to work because 'font-lock-face will just override ‘face.

You want font-lock-append-text-property, I think:

  (font-lock-append-text-property START END PROP VALUE &optional OBJECT)

  Append to one property of the text from START to END.
  Arguments PROP and VALUE specify the property and value to append to the value
  already in place.  The resulting property values are always lists.
  Optional argument OBJECT is the string or buffer containing the text.