unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Question: add-face-text-property for 'font-lock-face?
@ 2021-08-29  4:13 Qiantan Hong
  2021-08-29  4:24 ` Yuan Fu
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Qiantan Hong @ 2021-08-29  4:13 UTC (permalink / raw)
  To: Emacs Devel

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


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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  4:13 Question: add-face-text-property for 'font-lock-face? Qiantan Hong
@ 2021-08-29  4:24 ` Yuan Fu
       [not found]   ` <B73E855F-7E55-4217-BD43-E1272939E491@mit.edu>
  2021-08-29  7:05 ` Eli Zaretskii
  2021-08-29 16:13 ` Clément Pit-Claudel
  2 siblings, 1 reply; 15+ messages in thread
From: Yuan Fu @ 2021-08-29  4:24 UTC (permalink / raw)
  To: Qiantan Hong; +Cc: Emacs Devel



> 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


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

* Question: add-face-text-property for 'font-lock-face?
       [not found]   ` <B73E855F-7E55-4217-BD43-E1272939E491@mit.edu>
@ 2021-08-29  4:36     ` Qiantan Hong
  2021-08-30  8:22       ` Ihor Radchenko
       [not found]     ` <945E4D23-3D8C-45AB-9C10-992638A30607@gmail.com>
  1 sibling, 1 reply; 15+ messages in thread
From: Qiantan Hong @ 2021-08-29  4:36 UTC (permalink / raw)
  To: Emacs Devel

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

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

* Re: Question: add-face-text-property for 'font-lock-face?
       [not found]     ` <945E4D23-3D8C-45AB-9C10-992638A30607@gmail.com>
@ 2021-08-29  4:47       ` Qiantan Hong
  2021-08-29 23:50         ` Stefan Monnier
  0 siblings, 1 reply; 15+ messages in thread
From: Qiantan Hong @ 2021-08-29  4:47 UTC (permalink / raw)
  To: Yuan Fu; +Cc: Emacs Devel

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


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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  4:13 Question: add-face-text-property for 'font-lock-face? Qiantan Hong
  2021-08-29  4:24 ` Yuan Fu
@ 2021-08-29  7:05 ` Eli Zaretskii
  2021-08-29  7:19   ` Qiantan Hong
  2021-08-29 16:13 ` Clément Pit-Claudel
  2 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2021-08-29  7:05 UTC (permalink / raw)
  To: Qiantan Hong; +Cc: emacs-devel

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



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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  7:05 ` Eli Zaretskii
@ 2021-08-29  7:19   ` Qiantan Hong
  2021-08-29  7:49     ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Qiantan Hong @ 2021-08-29  7:19 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel@gnu.org

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.



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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  7:19   ` Qiantan Hong
@ 2021-08-29  7:49     ` Eli Zaretskii
  2021-08-29 16:15       ` Clément Pit-Claudel
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2021-08-29  7:49 UTC (permalink / raw)
  To: Qiantan Hong; +Cc: emacs-devel

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



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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  4:13 Question: add-face-text-property for 'font-lock-face? Qiantan Hong
  2021-08-29  4:24 ` Yuan Fu
  2021-08-29  7:05 ` Eli Zaretskii
@ 2021-08-29 16:13 ` Clément Pit-Claudel
  2 siblings, 0 replies; 15+ messages in thread
From: Clément Pit-Claudel @ 2021-08-29 16:13 UTC (permalink / raw)
  To: emacs-devel

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.




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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  7:49     ` Eli Zaretskii
@ 2021-08-29 16:15       ` Clément Pit-Claudel
  2021-08-29 21:13         ` Qiantan Hong
  0 siblings, 1 reply; 15+ messages in thread
From: Clément Pit-Claudel @ 2021-08-29 16:15 UTC (permalink / raw)
  To: emacs-devel

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.





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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29 16:15       ` Clément Pit-Claudel
@ 2021-08-29 21:13         ` Qiantan Hong
  2021-08-29 23:47           ` Stefan Monnier
  0 siblings, 1 reply; 15+ messages in thread
From: Qiantan Hong @ 2021-08-29 21:13 UTC (permalink / raw)
  To: cpitclaudel@gmail.com, Eli Zaretskii; +Cc: Emacs Devel

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

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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29 21:13         ` Qiantan Hong
@ 2021-08-29 23:47           ` Stefan Monnier
  2021-08-30  0:41             ` Qiantan Hong
  2021-08-30  8:18             ` Ihor Radchenko
  0 siblings, 2 replies; 15+ messages in thread
From: Stefan Monnier @ 2021-08-29 23:47 UTC (permalink / raw)
  To: Qiantan Hong; +Cc: cpitclaudel@gmail.com, Eli Zaretskii, Emacs Devel

> 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




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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  4:47       ` Qiantan Hong
@ 2021-08-29 23:50         ` Stefan Monnier
  0 siblings, 0 replies; 15+ messages in thread
From: Stefan Monnier @ 2021-08-29 23:50 UTC (permalink / raw)
  To: Qiantan Hong; +Cc: Yuan Fu, Emacs Devel

> 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




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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29 23:47           ` Stefan Monnier
@ 2021-08-30  0:41             ` Qiantan Hong
  2021-08-30  8:18             ` Ihor Radchenko
  1 sibling, 0 replies; 15+ messages in thread
From: Qiantan Hong @ 2021-08-30  0:41 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: cpitclaudel@gmail.com, Eli Zaretskii, Emacs Devel

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

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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29 23:47           ` Stefan Monnier
  2021-08-30  0:41             ` Qiantan Hong
@ 2021-08-30  8:18             ` Ihor Radchenko
  1 sibling, 0 replies; 15+ messages in thread
From: Ihor Radchenko @ 2021-08-30  8:18 UTC (permalink / raw)
  To: Stefan Monnier
  Cc: Qiantan Hong, Eli Zaretskii, cpitclaudel@gmail.com, Emacs Devel

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




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

* Re: Question: add-face-text-property for 'font-lock-face?
  2021-08-29  4:36     ` Qiantan Hong
@ 2021-08-30  8:22       ` Ihor Radchenko
  0 siblings, 0 replies; 15+ messages in thread
From: Ihor Radchenko @ 2021-08-30  8:22 UTC (permalink / raw)
  To: Qiantan Hong; +Cc: Emacs Devel

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



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

end of thread, other threads:[~2021-08-30  8:22 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-29  4:13 Question: add-face-text-property for 'font-lock-face? Qiantan Hong
2021-08-29  4:24 ` Yuan Fu
     [not found]   ` <B73E855F-7E55-4217-BD43-E1272939E491@mit.edu>
2021-08-29  4:36     ` Qiantan Hong
2021-08-30  8:22       ` Ihor Radchenko
     [not found]     ` <945E4D23-3D8C-45AB-9C10-992638A30607@gmail.com>
2021-08-29  4:47       ` Qiantan Hong
2021-08-29 23:50         ` Stefan Monnier
2021-08-29  7:05 ` Eli Zaretskii
2021-08-29  7:19   ` Qiantan Hong
2021-08-29  7:49     ` Eli Zaretskii
2021-08-29 16:15       ` Clément Pit-Claudel
2021-08-29 21:13         ` Qiantan Hong
2021-08-29 23:47           ` Stefan Monnier
2021-08-30  0:41             ` Qiantan Hong
2021-08-30  8:18             ` Ihor Radchenko
2021-08-29 16:13 ` Clément Pit-Claudel

Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).