unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* face-attribute and face-remapping-alist
@ 2021-03-30 18:53 gliao.tw
  2021-03-30 19:05 ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: gliao.tw @ 2021-03-30 18:53 UTC (permalink / raw)
  To: emacs-devel@gnu.org

The `face-attribute' function defined in `faces.el' is the foundation of many face property inquiry functions such as `face-background'. However, `face-attribute' is not aware of buffer-local variable 'face-remapping-alist' which is used for buffer-specific theming.

Since there are many modes, such as `term-mode', rely on `face-attribute' to obtain face information while `face-attribute' is only capabale of obtaining frame-specific, rather than buffer-specific face infomation, these modes cannot display faces sufficiently well while a buffer-local theme is applied via `face-remap-add-relative' or `face-remap-set-base' (both functions, defined in `face-remap.el', add items to buffer-specific variable `face-remapping-alist').

Therefore, I propose the following prototype of an enhanced version of `face-attribute' function that is aware of the existence of `face-remapping-alist' in current buffer and return face property from `face-remapping-alist' instead of  frame-specific (single frame also means global) face property.

Any comments are greatly appreciated, thanks.

Kiong-Gē.

---------------------------


;;
(defun face-attribute (face attribute &optional frame inherit)
  "Return the value of FACE's ATTRIBUTE on FRAME or current buffer.
If the optional argument FRAME is given, report on face FACE in that frame.
If FRAME is t, report on the defaults for face FACE (for new frames).
If FRAME is omitted or nil, use the selected frame.

If INHERIT is nil, only attributes directly defined by FACE are considered,
  so the return value may be `unspecified', or a relative value.
If INHERIT is non-nil, FACE's definition of ATTRIBUTE is merged with the
  faces specified by its `:inherit' attribute; however the return value
  may still be `unspecified' or relative.
If INHERIT is a face or a list of faces, then the result is further merged
  with that face (or faces), until it becomes specified and absolute.

To ensure that the return value is always specified and absolute, use a
value of `default' for INHERIT; this will resolve any unspecified or
relative values by merging with the `default' face (which is always
completely specified)."
  ;; check if `face-remapping-alist' exist as a buffer-local variable
  ;; in current buffer
  (let* ((local-faces (if (local-variable-p 'face-remapping-alist
					    (current-buffer))
			  ;; if so, take faces information from it
			  (buffer-local-value 'face-remapping-alist
					      (current-buffer))))
	 ;; try to fetch buffer-local face proprety from face-remapping-alist
	 (local-face-prop (if local-faces
			      (plist-get (car (alist-get face local-faces))
					 attribute))))
    ;; if there is no inquired face informatio available in
    ;; face-remapping-alist, fallback to frame-specific values
    (or local-face-prop
	(let ((value (internal-get-lisp-face-attribute face attribute frame)))
	  (when (and inherit (face-attribute-relative-p attribute value))
	    ;; VALUE is relative, so merge with inherited faces
	    (let ((inh-from (face-attribute face :inherit frame)))
	      (unless (or (null inh-from) (eq inh-from 'unspecified))
		(condition-case nil
		    (setq value
			  (face-attribute-merged-with attribute value inh-from frame))
		  ;; The `inherit' attribute may point to non existent faces.
		  (error nil)))))
	  (when (and inherit
		     (not (eq inherit t))
		     (face-attribute-relative-p attribute value))
	    ;; We should merge with INHERIT as well
	    (setq value (face-attribute-merged-with attribute value inherit frame)))
	  value))))


----------------------------------------







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

* Re: face-attribute and face-remapping-alist
  2021-03-30 18:53 face-attribute and face-remapping-alist gliao.tw
@ 2021-03-30 19:05 ` Eli Zaretskii
  2021-03-30 19:13   ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2021-03-30 19:05 UTC (permalink / raw)
  To: gliao.tw@pm.me; +Cc: emacs-devel

> Date: Tue, 30 Mar 2021 18:53:28 +0000
> From: "gliao.tw@pm.me" <gliao.tw@pm.me>
> 
> The `face-attribute' function defined in `faces.el' is the foundation of many face property inquiry functions such as `face-background'. However, `face-attribute' is not aware of buffer-local variable 'face-remapping-alist' which is used for buffer-specific theming.
> 
> Since there are many modes, such as `term-mode', rely on `face-attribute' to obtain face information while `face-attribute' is only capabale of obtaining frame-specific, rather than buffer-specific face infomation, these modes cannot display faces sufficiently well while a buffer-local theme is applied via `face-remap-add-relative' or `face-remap-set-base' (both functions, defined in `face-remap.el', add items to buffer-specific variable `face-remapping-alist').
> 
> Therefore, I propose the following prototype of an enhanced version of `face-attribute' function that is aware of the existence of `face-remapping-alist' in current buffer and return face property from `face-remapping-alist' instead of  frame-specific (single frame also means global) face property.

Thanks, but unconditionally changing the behavior of face-attribute in
such fundamental ways is a non-starter.  There must be a way to still
get the frame-specific face attributes, un-effected by buffer-specific
remapping.  face-attribute is quite a low-level API, and face
remapping is a higher-level feature.

So if we want face-attribute to pay attention to face remapping, that
must be an optional behavior under control of some optional argument
or a variable that could be let-bound.

Alternatively, we could introduce a new API for what you want.



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

* Re: face-attribute and face-remapping-alist
  2021-03-30 19:05 ` Eli Zaretskii
@ 2021-03-30 19:13   ` Eli Zaretskii
  2021-03-31  3:05     ` gliao.tw
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2021-03-30 19:13 UTC (permalink / raw)
  To: gliao.tw; +Cc: emacs-devel

> Date: Tue, 30 Mar 2021 22:05:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Thanks, but unconditionally changing the behavior of face-attribute in
> such fundamental ways is a non-starter.  There must be a way to still
> get the frame-specific face attributes, un-effected by buffer-specific
> remapping.  face-attribute is quite a low-level API, and face
> remapping is a higher-level feature.
> 
> So if we want face-attribute to pay attention to face remapping, that
> must be an optional behavior under control of some optional argument
> or a variable that could be let-bound.
> 
> Alternatively, we could introduce a new API for what you want.

And btw, I don't really understand the difficulty: fetching the
remapped face from face-remapping-alist is trivial.  Thereafter,
face-attribute will do what you want.  So I don't even think I
understand the problem you are trying to solve.  I guess a detailed
description of some specific example would help here.

In any case, please don't forget that frame-local and buffer-local
values cannot be intermixed without producing undesirable effects.
Face remapping was introduced to allow finer resolution in face
attributes than the per-frame one, under the control of the
application.  Your proposal pushes the buffer-local aspects to the
lower levels, and out of the application's control, which I don't
think is TRT.



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

* Re: face-attribute and face-remapping-alist
  2021-03-30 19:13   ` Eli Zaretskii
@ 2021-03-31  3:05     ` gliao.tw
  2021-03-31  3:34       ` gliao.tw
  2021-03-31  6:58       ` Eli Zaretskii
  0 siblings, 2 replies; 10+ messages in thread
From: gliao.tw @ 2021-03-31  3:05 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel@gnu.org

[-- Attachment #1: Type: text/plain, Size: 7097 bytes --]


Eli,

thanks for the comments and questions. I would like to ues the following
an example to expalint why I want to  propose such a change in `face-attribute'.

* The issue

In the following emacs lisp transcript, I set `leuven' as the global theme and
load but not yet enable `tango' theme, and then ask for its `default'
face attributes under `leuven' globla them. The  corresponding display
are shown in attacthed "Pic_1_global_leuven_theme_applied.png".

(load-theme 'leuven t nil)

(load-theme 'tango t t)

(face-attribute 'default :background)

(face-attribute 'default :foreground)

Next, I set the `ielm' buffer's default face using `wombat' theme's `default'
face via `face-remap-add-relative' and inquire the 'default' face's foreground
and background via `face-attribute` again using the following command:

(face-attribute 'default :background)

(face-attribute 'default :foreground)

As we can see the results shown in the attached
"Pic_2_local_wombat_theme_applied.png", although the backgound and
foreground have been changed according to `wombat' theme's settings ,
but `face-attribute' function still returns the 'default face attribute of
`leuven', instead of `wombat' theme's.

From the above example, we can see that `face-attribute' will return results
 *incosistent* with what we see in a buffer which has been updated with
a theme in buffer-local manner via functions implemented in `face-remap.el'.

Another aspect of this issue is that many modes rely on `face-attribute' to
obtain appropriate face setting and it is known that `face-attribute' is unlikely
to return correct results in buffer whose appearence is specificed by
buffer-local `face-remapping-alist'.

The following code are extracted from `term.el', which shows a heavy
dependency on `face-attribute'-based `face-foreground` and
`face-background' functions to extract face information.

      (setq term-current-face
            (list :foreground
                  (face-foreground
                   (elt ansi-term-color-vector term-ansi-current-color)
                   nil 'default)
                  :background
                  (face-background
                   (elt ansi-term-color-vector term-ansi-current-bg-color)
                   nil 'default)
                  :inverse-video term-ansi-current-reverse))

In the attacched  "Pic_3_invalid_local_theme_rendering_in_ansi_term.png"
screenshot, we can see that `ansi-term' from `term.el' is unable to render
buffer local theme properly (the top) while eshell (the bottom) is able to
render the buffer local color theme (relatively) properly.

* Potential solutions/directions

  The following table summarizes some directions/solutions I can think of
  at the time being:

  |-------------------------+---------------+-----------------|
  | Approach                | Benefit       | Risk / Cost     |
  |-------------------------+---------------+-----------------|
  | make `face-attribute'   | No need       | (Potentially)   |
  | `face-remapping-alist'- | to change     | mix frame-local |
  | aware                   | any exisitng  | and buffer-     |
  |                         | libraries/    | local settings  |
  |                         | packages      |                 |
  |                         |               |                 |
  |-------------------------+---------------+-----------------|
  | New Emacs APIs that are | No changes    | Require porting |
  | `face-remapping-alist'- | in existing   | efforts from    |
  | aware, potentially use  | Emacs API     | libraries /     |
  | current                 |               | packages        |
  | `face-attribute' as the |               | developers;     |
  | fallback option         |               | marketing cost  |
  |                         |               | on the new APIs |
  |                         |               |                 |
  |-------------------------+---------------+-----------------|
  | No change in Emacs at   | Users pay     | Current issues  |
  | all: package/library    | as they go:   | remain; wild    |
  | developers come up with | spend your    | west of wheel   |
  | their own solutions to  | own resources | re-invention.   |
  | deal with buffer-local  | on features   |                 |
  | face setting            | you want.     |                 |
  |                         |               |                 |
  |-------------------------+---------------+-----------------|

The above table just represents my current best, and definitely
incomprehensive, thoughts on how can we make buffer-local face
setting functions and face information functions work more
consistently.

The attached
"Pic_4_buffer_local_aware_fa_ansi_term_and_eshell_consistent.png" shows
a result using appoarch #1.  The top `eshell' buffer and bottom `ansi-term'
buffer can have very cosistent (although some difference in bold face),
almost identical  appearence with same buffer-local theme applied. Compare
to "Pic_3_invalid_local_theme_rendering_in_ansi_term.png", we can
see approach #1 does yield some favorable results (comes with the risk
stated in above tabel).

I hope above material can do some little help in clarifying my questions
and thoughts on issues caused by  current `face-remapping-alist'-unaware
implementation of `face-attribute'.

Any comments are greatly appreciated.

Thanks,
Kiong-Gē.




‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, March 30, 2021 2:13 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Tue, 30 Mar 2021 22:05:00 +0300
>
> > From: Eli Zaretskii eliz@gnu.org
> > Cc: emacs-devel@gnu.org
> > Thanks, but unconditionally changing the behavior of face-attribute in
> > such fundamental ways is a non-starter. There must be a way to still
> > get the frame-specific face attributes, un-effected by buffer-specific
> > remapping. face-attribute is quite a low-level API, and face
> > remapping is a higher-level feature.
> > So if we want face-attribute to pay attention to face remapping, that
> > must be an optional behavior under control of some optional argument
> > or a variable that could be let-bound.
> > Alternatively, we could introduce a new API for what you want.
>
> And btw, I don't really understand the difficulty: fetching the
> remapped face from face-remapping-alist is trivial. Thereafter,
> face-attribute will do what you want. So I don't even think I
> understand the problem you are trying to solve. I guess a detailed
> description of some specific example would help here.
>
> In any case, please don't forget that frame-local and buffer-local
> values cannot be intermixed without producing undesirable effects.
> Face remapping was introduced to allow finer resolution in face
> attributes than the per-frame one, under the control of the
> application. Your proposal pushes the buffer-local aspects to the
> lower levels, and out of the application's control, which I don't
> think is TRT.


[-- Attachment #2: Pic_1_global_leuven_theme_applied.png --]
[-- Type: image/png, Size: 49079 bytes --]

[-- Attachment #3: Pic_4_buffer_local_aware_fa_ansi_term_and_eshell_consistent.png --]
[-- Type: image/png, Size: 23160 bytes --]

[-- Attachment #4: Pic_3_invalid_local_theme_rendering_in_ansi_term.png --]
[-- Type: image/png, Size: 26768 bytes --]

[-- Attachment #5: Pic_2_local_wombat_theme_applied.png --]
[-- Type: image/png, Size: 73758 bytes --]

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

* Re: face-attribute and face-remapping-alist
  2021-03-31  3:05     ` gliao.tw
@ 2021-03-31  3:34       ` gliao.tw
  2021-03-31  6:58       ` Eli Zaretskii
  1 sibling, 0 replies; 10+ messages in thread
From: gliao.tw @ 2021-03-31  3:34 UTC (permalink / raw)
  To: eliz; +Cc: emacs-devel

[-- Attachment #1: Type: text/plain, Size: 6625 bytes --]

Sorry, in the first example, tango theme should be wombat theme.

Sent from ProtonMail mobile

-------- Original Message --------
On Mar 30, 2021, 22:05, gliao.tw@pm.me wrote:

> Eli,
>
> thanks for the comments and questions. I would like to ues the following
> an example to expalint why I want to propose such a change in `face-attribute'.
>
> * The issue
>
> In the following emacs lisp transcript, I set `leuven' as the global theme and
> load but not yet enable `tango' theme, and then ask for its `default'
> face attributes under `leuven' globla them. The corresponding display
> are shown in attacthed "Pic_1_global_leuven_theme_applied.png".
>
> (load-theme 'leuven t nil)
>
> (load-theme 'tango t t)
>
> (face-attribute 'default :background)
>
> (face-attribute 'default :foreground)
>
> Next, I set the `ielm' buffer's default face using `wombat' theme's `default'
> face via `face-remap-add-relative' and inquire the 'default' face's foreground
> and background via `face-attribute` again using the following command:
>
> (face-attribute 'default :background)
>
> (face-attribute 'default :foreground)
>
> As we can see the results shown in the attached
> "Pic_2_local_wombat_theme_applied.png", although the backgound and
> foreground have been changed according to `wombat' theme's settings ,
> but `face-attribute' function still returns the 'default face attribute of
> `leuven', instead of `wombat' theme's.
>
> From the above example, we can see that `face-attribute' will return results
> *incosistent* with what we see in a buffer which has been updated with
> a theme in buffer-local manner via functions implemented in `face-remap.el'.
>
> Another aspect of this issue is that many modes rely on `face-attribute' to
> obtain appropriate face setting and it is known that `face-attribute' is unlikely
> to return correct results in buffer whose appearence is specificed by
> buffer-local `face-remapping-alist'.
>
> The following code are extracted from `term.el', which shows a heavy
> dependency on `face-attribute'-based `face-foreground` and
> `face-background' functions to extract face information.
>
> (setq term-current-face
> (list :foreground
> (face-foreground
> (elt ansi-term-color-vector term-ansi-current-color)
> nil 'default)
> :background
> (face-background
> (elt ansi-term-color-vector term-ansi-current-bg-color)
> nil 'default)
> :inverse-video term-ansi-current-reverse))
>
> In the attacched "Pic_3_invalid_local_theme_rendering_in_ansi_term.png"
> screenshot, we can see that `ansi-term' from `term.el' is unable to render
> buffer local theme properly (the top) while eshell (the bottom) is able to
> render the buffer local color theme (relatively) properly.
>
> * Potential solutions/directions
>
> The following table summarizes some directions/solutions I can think of
> at the time being:
>
> |-------------------------+---------------+-----------------|
> | Approach | Benefit | Risk / Cost |
> |-------------------------+---------------+-----------------|
> | make `face-attribute' | No need | (Potentially) |
> | `face-remapping-alist'- | to change | mix frame-local |
> | aware | any exisitng | and buffer- |
> | | libraries/ | local settings |
> | | packages | |
> | | | |
> |-------------------------+---------------+-----------------|
> | New Emacs APIs that are | No changes | Require porting |
> | `face-remapping-alist'- | in existing | efforts from |
> | aware, potentially use | Emacs API | libraries / |
> | current | | packages |
> | `face-attribute' as the | | developers; |
> | fallback option | | marketing cost |
> | | | on the new APIs |
> | | | |
> |-------------------------+---------------+-----------------|
> | No change in Emacs at | Users pay | Current issues |
> | all: package/library | as they go: | remain; wild |
> | developers come up with | spend your | west of wheel |
> | their own solutions to | own resources | re-invention. |
> | deal with buffer-local | on features | |
> | face setting | you want. | |
> | | | |
> |-------------------------+---------------+-----------------|
>
> The above table just represents my current best, and definitely
> incomprehensive, thoughts on how can we make buffer-local face
> setting functions and face information functions work more
> consistently.
>
> The attached
> "Pic_4_buffer_local_aware_fa_ansi_term_and_eshell_consistent.png" shows
> a result using appoarch #1. The top `eshell' buffer and bottom `ansi-term'
> buffer can have very cosistent (although some difference in bold face),
> almost identical appearence with same buffer-local theme applied. Compare
> to "Pic_3_invalid_local_theme_rendering_in_ansi_term.png", we can
> see approach #1 does yield some favorable results (comes with the risk
> stated in above tabel).
>
> I hope above material can do some little help in clarifying my questions
> and thoughts on issues caused by current `face-remapping-alist'-unaware
> implementation of `face-attribute'.
>
> Any comments are greatly appreciated.
>
> Thanks,
> Kiong-Gē.
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Tuesday, March 30, 2021 2:13 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > Date: Tue, 30 Mar 2021 22:05:00 +0300
>>
>> > From: Eli Zaretskii eliz@gnu.org
>> > Cc: emacs-devel@gnu.org
>> > Thanks, but unconditionally changing the behavior of face-attribute in
>> > such fundamental ways is a non-starter. There must be a way to still
>> > get the frame-specific face attributes, un-effected by buffer-specific
>> > remapping. face-attribute is quite a low-level API, and face
>> > remapping is a higher-level feature.
>> > So if we want face-attribute to pay attention to face remapping, that
>> > must be an optional behavior under control of some optional argument
>> > or a variable that could be let-bound.
>> > Alternatively, we could introduce a new API for what you want.
>>
>> And btw, I don't really understand the difficulty: fetching the
>> remapped face from face-remapping-alist is trivial. Thereafter,
>> face-attribute will do what you want. So I don't even think I
>> understand the problem you are trying to solve. I guess a detailed
>> description of some specific example would help here.
>>
>> In any case, please don't forget that frame-local and buffer-local
>> values cannot be intermixed without producing undesirable effects.
>> Face remapping was introduced to allow finer resolution in face
>> attributes than the per-frame one, under the control of the
>> application. Your proposal pushes the buffer-local aspects to the
>> lower levels, and out of the application's control, which I don't
>> think is TRT.

[-- Attachment #2: Type: text/html, Size: 7613 bytes --]

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

* Re: face-attribute and face-remapping-alist
  2021-03-31  3:05     ` gliao.tw
  2021-03-31  3:34       ` gliao.tw
@ 2021-03-31  6:58       ` Eli Zaretskii
  2021-03-31 13:00         ` Clément Pit-Claudel
  2021-03-31 22:41         ` gliao.tw
  1 sibling, 2 replies; 10+ messages in thread
From: Eli Zaretskii @ 2021-03-31  6:58 UTC (permalink / raw)
  To: gliao.tw@pm.me; +Cc: emacs-devel

> Date: Wed, 31 Mar 2021 03:05:42 +0000
> From: "gliao.tw@pm.me" <gliao.tw@pm.me>
> Cc: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> 
> >From the above example, we can see that `face-attribute' will return results
>  *incosistent* with what we see in a buffer which has been updated with
> a theme in buffer-local manner via functions implemented in `face-remap.el'.

Sure, but the only change needed in all the cases you described in
order to return attributes that are aware of the remapping is to look
up the face in face-remapping-alist, before calling face-attribute.
This solution is so easy that I don't understand the need for changing
the behavior of face-attribute in such fundamental and incompatible
ways to produce the same effect.

Am I missing something?



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

* Re: face-attribute and face-remapping-alist
  2021-03-31  6:58       ` Eli Zaretskii
@ 2021-03-31 13:00         ` Clément Pit-Claudel
  2021-03-31 13:27           ` Eli Zaretskii
  2021-03-31 22:41         ` gliao.tw
  1 sibling, 1 reply; 10+ messages in thread
From: Clément Pit-Claudel @ 2021-03-31 13:00 UTC (permalink / raw)
  To: emacs-devel

On 3/31/21 2:58 AM, Eli Zaretskii wrote:
> Sure, but the only change needed in all the cases you described in
> order to return attributes that are aware of the remapping is to look
> up the face in face-remapping-alist, before calling face-attribute.

You need to look it up recursively, which adds to the complexity.  And it's more complicated than that if you use the "inherit" argument, since you need to check whether the default face is remapped as well, and merge that with the real default face.



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

* Re: face-attribute and face-remapping-alist
  2021-03-31 13:00         ` Clément Pit-Claudel
@ 2021-03-31 13:27           ` Eli Zaretskii
  0 siblings, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2021-03-31 13:27 UTC (permalink / raw)
  To: Clément Pit-Claudel; +Cc: emacs-devel

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Wed, 31 Mar 2021 09:00:34 -0400
> 
> On 3/31/21 2:58 AM, Eli Zaretskii wrote:
> > Sure, but the only change needed in all the cases you described in
> > order to return attributes that are aware of the remapping is to look
> > up the face in face-remapping-alist, before calling face-attribute.
> 
> You need to look it up recursively, which adds to the complexity.  And it's more complicated than that if you use the "inherit" argument, since you need to check whether the default face is remapped as well, and merge that with the real default face.

The issue described by the OP was of accessing the attribute's value,
not merging face attributes.

Merging faces is a separate issue.  It is rare that an application
should need to merge faces, as Emacs does that automatically.



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

* Re: face-attribute and face-remapping-alist
  2021-03-31  6:58       ` Eli Zaretskii
  2021-03-31 13:00         ` Clément Pit-Claudel
@ 2021-03-31 22:41         ` gliao.tw
  2021-04-01  7:08           ` Eli Zaretskii
  1 sibling, 1 reply; 10+ messages in thread
From: gliao.tw @ 2021-03-31 22:41 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel@gnu.org



#+begin_src emacs-lisp

  (setq term-current-face
	      (list :foreground
		    (face-foreground
		     (elt ansi-term-color-vector term-ansi-current-color)
		     nil 'default)
		    :background
		    (face-background
		     (elt ansi-term-color-vector term-ansi-current-bg-color)
		     nil 'default)
		    :inverse-video term-ansi-current-reverse))

#+end_src

  And we already know that if an `ansi-term' buffer has its
  `term-color-*' faces have been customized through `face-remap-add-relative',
  `face-foreground' and `face-background' in above code won't
  return correct face information due to `face-attribute' is not
  aware of the `ansi-term' buffer's `face-remapping-alist' local variable.

  Therefore, to make `ansi-term' receive correct face information,
  we need to

  1. change 'face-attribute' to make it aware of `face-remapping-alist'
     in a way that is similar to my earlier proposal; or

  2. do not change any existing face-related functions but define new
     functions to retrieve buffer-local face settings such like

#+begin_src emacs-lisp

  (defun face-attribute-blfa (face attribute &optional frame inherit buffer)
    "Get face attribute with buffer-local faces aware"
    ;; retrieve buffer-local `face-remapping-alist', if it exists
    (or (let ((bl-fra (buffer-local-value
		       'face-remapping-alist
		       (or (and buffer (get-buffer buffer))
			   (current-buffer)))))
	  (and bl-fra
	       (plist-get (car (alist-get face bl-fra)) attribute)))
	(face-attribute face attribute frame inherit)))

  (defun face-foreground-blfa (face &optional frame inherit buffer)
    (face-attribute-blfa face :foreground frame inherit buffer))

  (defun face-background-blfa (face &optional frame inherit buffer)
    (face-attribute-blfa face :background frame inherit buffer))

  (setq term-current-face
	(list :foreground
	      (face-foreground-blfa
	       (elt ansi-term-color-vector term-ansi-current-color)
	       nil 'default)
	      :background
	      (face-background-blfa
	       (elt ansi-term-color-vector term-ansi-current-bg-color)
	       nil 'default)
	      :inverse-video term-ansi-current-reverse))

#+end_src

As mentioned in my previous mails, introducing new APIs (without changing existing
`face-attribute'-based functions) to make buffer-local faces work sufficiently well in
various modes do require library/package developers to adapt the new APIs and change,
potentially considerable, part of their existing code base to accommodate that.

So my question here is that which of the following options

1. make `face-attribute' return buffer-local face attributes if the face has been
   customized (via `face-remapping-alist) in that buffer otherwise return
   frame-local face attributes
2. Introducing new face attribute getting/setting APIs to Emacs code base;
   these new APIs  are `face-remapping-alist' aware and use unchanged
   `face-attribute' as the fallback dispatch option
3.  Do nothing to Emacs code base: let the library/package developer to decide
    whether buffer local face settings should be accommodated or not

is the most economical one?

I personally believe the option #1 is the most favorable one because, ideally,
`face-attribute' should always return face attributes those are consistent with
what have been rendered in any given buffer. Also, this option does not require
changing any existing code base of libraries/packages/modes those manipulate
faces (for example, `term.el'). However, I don't know how much risk of
potential breakage brought by option #1. Therefore, we may treat this option
as a convenient (only need to change `face-attribute') but also a dangerous one.

Option #2 is an acceptable choice to me although it require porting efforts
from the mode/library/package developers to adapt new APIs in Emacs,
but at least we have an official APIs that can be used by all Emacs library/package
developers without re-inventing wheels.

Option #3 is the least favorable option to me, as it requires developers to come
up with lots of solutions just to solve the common problem of retrieving
face information those are consistent with what have been rendered in any
given buffers. This wild west approach seems not helpful in solving the
problem.

Thanks.
Kiong-Gē.





‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, March 31, 2021 1:58 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Wed, 31 Mar 2021 03:05:42 +0000
>
> > From: "gliao.tw@pm.me" gliao.tw@pm.me
> > Cc: "emacs-devel@gnu.org" emacs-devel@gnu.org
> >
> > > From the above example, we can see that `face-attribute' will return results *incosistent* with what we see in a buffer which has been updated with a theme in buffer-local manner via functions implemented in`face-remap.el'.
>
> Sure, but the only change needed in all the cases you described in
> order to return attributes that are aware of the remapping is to look
> up the face in face-remapping-alist, before calling face-attribute.
> This solution is so easy that I don't understand the need for changing
> the behavior of face-attribute in such fundamental and incompatible
> ways to produce the same effect.
>
> Am I missing something?





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

* Re: face-attribute and face-remapping-alist
  2021-03-31 22:41         ` gliao.tw
@ 2021-04-01  7:08           ` Eli Zaretskii
  0 siblings, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2021-04-01  7:08 UTC (permalink / raw)
  To: gliao.tw@pm.me; +Cc: emacs-devel

> Date: Wed, 31 Mar 2021 22:41:24 +0000
> From: "gliao.tw@pm.me" <gliao.tw@pm.me>
> Cc: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> 
>   Therefore, to make `ansi-term' receive correct face information,
>   we need to
> 
>   1. change 'face-attribute' to make it aware of `face-remapping-alist'
>      in a way that is similar to my earlier proposal; or
> 
>   2. do not change any existing face-related functions but define new
>      functions to retrieve buffer-local face settings such like

    3. do not change any existing face-related functions but look up
       the relevant face in face-remapping-alist before calling
       face-attribute.

> So my question here is that which of the following options
> 
> 1. make `face-attribute' return buffer-local face attributes if the face has been
>    customized (via `face-remapping-alist) in that buffer otherwise return
>    frame-local face attributes
> 2. Introducing new face attribute getting/setting APIs to Emacs code base;
>    these new APIs  are `face-remapping-alist' aware and use unchanged
>    `face-attribute' as the fallback dispatch option
> 3.  Do nothing to Emacs code base: let the library/package developer to decide
>     whether buffer local face settings should be accommodated or not
> 
> is the most economical one?

"Economical" in what sense?

Changing long-standing default behavior of face-attribute is out of
the question: we cannot make such changes in such old APIs.  Item 3 is
so easy to implement in those packages that really want to cater to
buffer-local face customizations is so easy that I see no need for any
change in core.  Which then makes option 3 "the most economical" from
my POV.

And that's even before we discussed whether sensitivity to
buffer-local face changes is at all a good idea in general, something
that I'm not at all convinced in.  Faces are more often than not
related to major and minor modes, so having them change depending on
buffer-local customizations would mean 2 buffers under the same mode
will look differently.  Why is that a good idea?



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

end of thread, other threads:[~2021-04-01  7:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 18:53 face-attribute and face-remapping-alist gliao.tw
2021-03-30 19:05 ` Eli Zaretskii
2021-03-30 19:13   ` Eli Zaretskii
2021-03-31  3:05     ` gliao.tw
2021-03-31  3:34       ` gliao.tw
2021-03-31  6:58       ` Eli Zaretskii
2021-03-31 13:00         ` Clément Pit-Claudel
2021-03-31 13:27           ` Eli Zaretskii
2021-03-31 22:41         ` gliao.tw
2021-04-01  7:08           ` Eli Zaretskii

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