GNU bug report logs - #65632
30.0.50; Proposal to improve `faces--attribute-at-point'.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 65632 in the body.
You can then email your comments to 65632 AT debbugs.gnu.org in the normal way.

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

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

From: David Ponce <da_vid <at> orange.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; Proposal to improve `faces--attribute-at-point'.
Date: Wed, 30 Aug 2023 20:04:49 +0200

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

Hello,

I noticed that the functions `foreground-color-at-point' and
`background-color-at-point' don't return expected values when the face
at point includes anonymous face or is a nested list of face, for
example.

Here is a simple recipe that demonstrates the issue (emacs -Q):

In scratch buffer eval:
-----------------------

;; Display "TEST" in red, bold, italic on yellow background.
(insert
 (propertize
  "TEST" 'font-lock-face
  '(bold ((:background "yellow") "italic"
          ((foreground-color . "red") underline)))))
TESTnil

;; Then click to move point somewhere on TEST and run
M-: (foreground-color-at-point) RET
>>> result is "black" instead of "red"
M-: (background-color-at-point) RET
>>> result is "white" instead of "yellow"

I propose the attached patch to faces.el to improve things.
The patch introduce a new function `face-attribute-lookup' to lookup
face attribute, that works when face specification is complex like in
above example.  The function `faces--attribute-at-point' is simplified
to use it.

Here is a possible changelog:

	* faces.el: Improve attribute lookup of face at point.
	(face--unnamed-attributes): New constant.
	(face--attribute-unspecified-p)
	(face-attribute-lookup): New functions.
	(faces--attribute-at-point): Use it.  Remove useless argument.
	(foreground-color-at-point)
	(background-color-at-point): Update accordingly.

Thanks

[faces-attribute-lookup-V0.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: David Ponce <da_vid <at> orange.fr>
Cc: 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50;
 Proposal to improve `faces--attribute-at-point'.
Date: Wed, 30 Aug 2023 21:35:11 +0300

> Date: Wed, 30 Aug 2023 20:04:49 +0200
> From: David Ponce <da_vid <at> orange.fr>
> 
> I noticed that the functions `foreground-color-at-point' and
> `background-color-at-point' don't return expected values when the face
> at point includes anonymous face or is a nested list of face, for
> example.
> 
> Here is a simple recipe that demonstrates the issue (emacs -Q):
> 
> In scratch buffer eval:
> -----------------------
> 
> ;; Display "TEST" in red, bold, italic on yellow background.
> (insert
>   (propertize
>    "TEST" 'font-lock-face
>    '(bold ((:background "yellow") "italic"
>            ((foreground-color . "red") underline)))))

This is not a valid face, AFAIU.  That it works is sheer luck (because
Emacs is very lenient with this stuff).  The correct face definition
for what you want is this (see 'set-face-attribute's doc string):

  (insert
    (propertize
     "TEST" 'font-lock-face
     '(:weight bold :background "yellow" :slant italic
	     :foreground "red" :underline t)))

If you use the above, foreground-color-at-point etc. will work as
expected.

I'm not sure we want to go out of our way to support the kind of face
specifications that you used.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: da_vid <at> orange.fr
Cc: 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50;
 Proposal to improve `faces--attribute-at-point'.
Date: Wed, 30 Aug 2023 22:22:50 +0300

> Cc: 65632 <at> debbugs.gnu.org
> Date: Wed, 30 Aug 2023 21:35:11 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > ;; Display "TEST" in red, bold, italic on yellow background.
> > (insert
> >   (propertize
> >    "TEST" 'font-lock-face
> >    '(bold ((:background "yellow") "italic"
> >            ((foreground-color . "red") underline)))))
> 
> This is not a valid face, AFAIU.  That it works is sheer luck (because
> Emacs is very lenient with this stuff).  The correct face definition
> for what you want is this (see 'set-face-attribute's doc string):
> 
>   (insert
>     (propertize
>      "TEST" 'font-lock-face
>      '(:weight bold :background "yellow" :slant italic
> 	     :foreground "red" :underline t)))
> 
> If you use the above, foreground-color-at-point etc. will work as
> expected.
> 
> I'm not sure we want to go out of our way to support the kind of face
> specifications that you used.

However, if we do want that, we already have the technology:

  (face-attributes-as-vector (get-char-property (point) 'font-lock-face))

This will return a vector of face attribute values, where you can find
the value of any attribute you like.  For example, to get the
foreground color, evaluate:

  (aref 9
    (face-attributes-as-vector (get-char-property (point) 'font-lock-face)))

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50; Proposal to improve
 `faces--attribute-at-point'.
Date: Thu, 31 Aug 2023 00:30:43 +0200

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

On 30/08/2023 21:22, Eli Zaretskii wrote:
>> Cc: 65632 <at> debbugs.gnu.org
>> Date: Wed, 30 Aug 2023 21:35:11 +0300
>> From: Eli Zaretskii <eliz <at> gnu.org>
>>
>>> ;; Display "TEST" in red, bold, italic on yellow background.
>>> (insert
>>>    (propertize
>>>     "TEST" 'font-lock-face
>>>     '(bold ((:background "yellow") "italic"
>>>             ((foreground-color . "red") underline)))))
>>
>> This is not a valid face, AFAIU.  That it works is sheer luck (because
>> Emacs is very lenient with this stuff).  The correct face definition
>> for what you want is this (see 'set-face-attribute's doc string):
>>
>>    (insert
>>      (propertize
>>       "TEST" 'font-lock-face
>>       '(:weight bold :background "yellow" :slant italic
>> 	     :foreground "red" :underline t)))
>>
>> If you use the above, foreground-color-at-point etc. will work as
>> expected.
>>
>> I'm not sure we want to go out of our way to support the kind of face
>> specifications that you used.
> 
> However, if we do want that, we already have the technology:
> 
>    (face-attributes-as-vector (get-char-property (point) 'font-lock-face))
> 
> This will return a vector of face attribute values, where you can find
> the value of any attribute you like.  For example, to get the
> foreground color, evaluate:
> 
>    (aref 9
>      (face-attributes-as-vector (get-char-property (point) 'font-lock-face)))

Hi Eli,

Thank you very much for letting me know about `face-attributes-as-vector'
(maybe its doc string could be improved?).  It is exactly the function I need :-)

In case you are interested, I attached an updated patch to faces.el that use
this function to lookup face attribute, which improves and simplify  the
functions `faces--attribute-at-point', `foreground-color-at-point' and
`background-color-at-point'.

Regards

[faces-attribute-lookup-V1.patch (text/x-patch, attachment)]

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50; Proposal to improve
 `faces--attribute-at-point'.
Date: Thu, 31 Aug 2023 10:18:43 +0200

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

On 31/08/2023 00:30, David Ponce wrote:
> On 30/08/2023 21:22, Eli Zaretskii wrote:
>>> Cc: 65632 <at> debbugs.gnu.org
>>> Date: Wed, 30 Aug 2023 21:35:11 +0300
>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>
>>>> ;; Display "TEST" in red, bold, italic on yellow background.
>>>> (insert
>>>>    (propertize
>>>>     "TEST" 'font-lock-face
>>>>     '(bold ((:background "yellow") "italic"
>>>>             ((foreground-color . "red") underline)))))
>>>
>>> This is not a valid face, AFAIU.  That it works is sheer luck (because
>>> Emacs is very lenient with this stuff).  The correct face definition
>>> for what you want is this (see 'set-face-attribute's doc string):
>>>
>>>    (insert
>>>      (propertize
>>>       "TEST" 'font-lock-face
>>>       '(:weight bold :background "yellow" :slant italic
>>>          :foreground "red" :underline t)))
>>>
>>> If you use the above, foreground-color-at-point etc. will work as
>>> expected.
>>>
>>> I'm not sure we want to go out of our way to support the kind of face
>>> specifications that you used.
>>
>> However, if we do want that, we already have the technology:
>>
>>    (face-attributes-as-vector (get-char-property (point) 'font-lock-face))
>>
>> This will return a vector of face attribute values, where you can find
>> the value of any attribute you like.  For example, to get the
>> foreground color, evaluate:
>>
>>    (aref 9
>>      (face-attributes-as-vector (get-char-property (point) 'font-lock-face)))
> 
> Hi Eli,
> 
> Thank you very much for letting me know about `face-attributes-as-vector'
> (maybe its doc string could be improved?).  It is exactly the function I need :-)
> 
> In case you are interested, I attached an updated patch to faces.el that use
> this function to lookup face attribute, which improves and simplify  the
> functions `faces--attribute-at-point', `foreground-color-at-point' and
> `background-color-at-point'.
> 
> Regards

Please find attached a revised patch.  I used the name
`faces-attribute' instead of `face-attribute-lookup' for consistency
with `faces--attribute-at-point'.  I simplified this new function
because `face-attributes-as-vector' always returns the symbol
`unspecified' when an attribute is not specified (according to what I
understand of the implementation in xfaces.c).  I also improved the
doc string to include a link to the Elisp manual regarding the meaning
of face specification.

Here is an updated changelog:

	* faces.el: Improve attribute lookup of face at point.
	(face--attribute-index): New constant.
	(faces-attribute): New function.
	(faces--attribute-at-point): Use it.  Remove unused argument.
	(foreground-color-at-point)
	(background-color-at-point): Call accordingly.



Regards

[faces-attribute-lookup-V2.patch (text/x-patch, attachment)]

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

From: David Ponce <da_vid <at> orange.fr>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50; Proposal to improve
 `faces--attribute-at-point'.
Date: Thu, 31 Aug 2023 14:08:04 +0200

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

On 31/08/2023 10:18, David Ponce wrote:
> On 31/08/2023 00:30, David Ponce wrote:
>> On 30/08/2023 21:22, Eli Zaretskii wrote:
>>>> Cc: 65632 <at> debbugs.gnu.org
>>>> Date: Wed, 30 Aug 2023 21:35:11 +0300
>>>> From: Eli Zaretskii <eliz <at> gnu.org>
>>>>
>>>>> ;; Display "TEST" in red, bold, italic on yellow background.
>>>>> (insert
>>>>>    (propertize
>>>>>     "TEST" 'font-lock-face
>>>>>     '(bold ((:background "yellow") "italic"
>>>>>             ((foreground-color . "red") underline)))))
>>>>
>>>> This is not a valid face, AFAIU.  That it works is sheer luck (because
>>>> Emacs is very lenient with this stuff).  The correct face definition
>>>> for what you want is this (see 'set-face-attribute's doc string):
>>>>
>>>>    (insert
>>>>      (propertize
>>>>       "TEST" 'font-lock-face
>>>>       '(:weight bold :background "yellow" :slant italic
>>>>          :foreground "red" :underline t)))
>>>>
>>>> If you use the above, foreground-color-at-point etc. will work as
>>>> expected.
>>>>
>>>> I'm not sure we want to go out of our way to support the kind of face
>>>> specifications that you used.
>>>
>>> However, if we do want that, we already have the technology:
>>>
>>>    (face-attributes-as-vector (get-char-property (point) 'font-lock-face))
>>>
>>> This will return a vector of face attribute values, where you can find
>>> the value of any attribute you like.  For example, to get the
>>> foreground color, evaluate:
>>>
>>>    (aref 9
>>>      (face-attributes-as-vector (get-char-property (point) 'font-lock-face)))
>>
>> Hi Eli,
>>
>> Thank you very much for letting me know about `face-attributes-as-vector'
>> (maybe its doc string could be improved?).  It is exactly the function I need :-)
>>
>> In case you are interested, I attached an updated patch to faces.el that use
>> this function to lookup face attribute, which improves and simplify  the
>> functions `faces--attribute-at-point', `foreground-color-at-point' and
>> `background-color-at-point'.
>>
>> Regards
> 
> Please find attached a revised patch.  I used the name
> `faces-attribute' instead of `face-attribute-lookup' for consistency
> with `faces--attribute-at-point'.  I simplified this new function
> because `face-attributes-as-vector' always returns the symbol
> `unspecified' when an attribute is not specified (according to what I
> understand of the implementation in xfaces.c).  I also improved the
> doc string to include a link to the Elisp manual regarding the meaning
> of face specification.
> 
> Here is an updated changelog:
> 
>      * faces.el: Improve attribute lookup of face at point.
>      (face--attribute-index): New constant.
>      (faces-attribute): New function.
>      (faces--attribute-at-point): Use it.  Remove unused argument.
>      (foreground-color-at-point)
>      (background-color-at-point): Call accordingly.
> 
> 
> 
> Regards

Sorry, my previous patch was buggy.  Here is the correct one.

[faces-attribute-lookup-V3.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: David Ponce <da_vid <at> orange.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50;
 Proposal to improve `faces--attribute-at-point'.
Date: Sun, 23 Feb 2025 01:27:31 +0000

David Ponce <da_vid <at> orange.fr> writes:

> Sorry, my previous patch was buggy.  Here is the correct one.

(Sorry for the lack of a response for a long time here.)

Could you please provide a rationale for these changes?  IOW, what are
they trying to achieve?  Thanks in advance.

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

From: David Ponce <da_vid <at> orange.fr>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 65632 <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50; Proposal to improve
 `faces--attribute-at-point'.
Date: Mon, 24 Feb 2025 12:01:41 +0100

On 2025-02-23 02:27, Stefan Kangas wrote:
> David Ponce <da_vid <at> orange.fr> writes:
> 
>> Sorry, my previous patch was buggy.  Here is the correct one.
> 
> (Sorry for the lack of a response for a long time here.)
> 
> Could you please provide a rationale for these changes?  IOW, what are
> they trying to achieve?  Thanks in advance.

Hello Stefan,

Sincerely, I thought this bug was closed a long time ago.

The rationale of this proposal was to handle any possible face spec returned by (get-char-property (point) 'face) in `faces--attribute-at-point' (by using `face-attributes-as-vector'), because in some corner cases the current implementation can return a wrong attribute value, as illustrated by the simple recipe in my initial post.

However, if I correctly understand Eli's objection, I agree that the face spec in my recipe can be considered as invalid, and not supported by `faces--attribute-at-point', even if a such face spec is an accepted text property, probably for backward compatibility.

Thanks

Message #37 received at 65632-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: David Ponce <da_vid <at> orange.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 65632-done <at> debbugs.gnu.org
Subject: Re: bug#65632: 30.0.50;
 Proposal to improve `faces--attribute-at-point'.
Date: Mon, 24 Feb 2025 16:34:53 +0000

David Ponce <da_vid <at> orange.fr> writes:

> Sincerely, I thought this bug was closed a long time ago.

[...]

OK, thanks.  I'm therefore closing this bug report now.

Previous Next

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