Adding new variable for face-list in internal-make-lisp-face.

Adding new variable for face-list in internal-make-lisp-face.

[Keith David Bershatsky]

In implementing feature requests crosshairs (17684) and multiple fake cursors (22873), I thought it would be nifty to identify the background color (if applicable) associated with a tab ('\t') glyph code in the buffer-display-table.  In doing so, I discovered that the Lisp function face-list is way too slow.  Even after I ported the face-list function over to C, it was still way too slow.  So, I tried my luck at adding a new variable in xfaces.c for the face-list and am setting it from within the function internal-make-lisp-face, like so:

    Vface_list = Fcons (face, Vface_list);

and, it is done within the same section where we already have:

      Vface_new_frame_defaults = Fcons (Fcons (face, global_lface),
        Vface_new_frame_defaults);

Accessing the tab face in the face-list variable works just fine UNTIL I manually `eval-defun` on the `defface` for the tab face to change its background color.  Thereafter, when I try to get information about that tab face in the face-list from within xdisp.c, I get:

Error during redisplay: (my-custom-face-attribute #<EMACS BUG: INVALID DATATYPE (MISC 0xc9ff) Save your buffers immediately and please report this bug> :background [frame information] default) signaled (error "Invalid face" #<EMACS BUG: INVALID DATATYPE (MISC 0xc9ff) Save your buffers immediately and please report this bug>)

Here is the gdb breakpoint information when I try to access that face (in the face-list variable) after manually changing the background color with eval-defun -- I get INVALID_LISP_OBJECT:

Breakpoint 3, Fmc_face_attribute (face=..., attribute=..., frame=..., 
    inherit=...) at xdisp.c:1119
1119	  Lisp_Object value = Finternal_get_lisp_face_attribute (face, attribute, frame);
(gdb) pp face
#<INVALID_LISP_OBJECT 0x10013c199>

How can I properly implement a new face-list variable so that the lisp object will be valid after manually running `eval-defun` on a defface?

Thanks,

Keith

Re: Adding new variable for face-list in internal-make-lisp-face.

[Eli Zaretskii]

> Date: Thu, 02 Nov 2017 20:50:47 -0700
> From: Keith David Bershatsky <esq@lawlist.com>
> 
> In implementing feature requests crosshairs (17684) and multiple fake cursors (22873), I thought it would be nifty to identify the background color (if applicable) associated with a tab ('\t') glyph code in the buffer-display-table.  In doing so, I discovered that the Lisp function face-list is way too slow.  Even after I ported the face-list function over to C, it was still way too slow.  So, I tried my luck at adding a new variable in xfaces.c for the face-list and am setting it from within the function internal-make-lisp-face, like so:

I don't understand why you need face-list for this.  For each glyph in
the display table, you get its face ID by using the GLYPH_FACE macro.
Why isn't that sufficient for your needs?

Re: Adding new variable for face-list in internal-make-lisp-face.

[Keith David Bershatsky]

Thank you, Eli, for reading this particular thread.

I have a tab buffer-display-table entry that looks like:

[(187 . 120) (9 . 121)]

Your suggestion to use GLYPH_FACE would be more efficient than my using the C equivalent of:

(aref buffer-display-table ?\t)

... and then iterating over the vector elements looking for a cons cell containing the number 9.

I will work on incorporating GLYPH_FACE into my code, instead of using the above iteration.  Thank you for the suggestion.

Once the face id is obtained, I have been using C implementations of the following:

;;; Example assumes that 121 is a valid Lisp face id.
(let* ((tab-cons-cell (cons 9 121))
       (face (glyph-face tab-cons-cell))
       (bg-color (mc-face-attribute
                   face
                   :background
                   (selected-frame)
                   'default)))
  bg-color)

The function `glyph-face' calls the function `face-list' which is too slow.  I saw no improvement in speed when calling Fmapcar (Qcar, Vface_new_frame_defaults) from C, versus the same thing in Lisp.

In a nutshell, I am looking for that background color and trying to implement a faster way of getting the result for (mapcar #'car face-new-frame-defaults).  That need for speed is what inspired me to think about creating a new Vface_list variable and populating it inside of internal-make-lisp-face at the same time that Vface_new_frame_defaults gets populated.  [xfaces.c]  The Vface_list variable would be (when implemented correctly) identical to the result returned by the Lisp function `face-list'.

I am unaware of another way to quickly get the background color for the above-situation, but would be open to new ideas of how to achieve that goal.

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-03-2017 03:17:05] <03 Nov 2017 12:17:05 +0200>
FROM:  Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Thu, 02 Nov 2017 20:50:47 -0700
> > From: Keith David Bershatsky <esq@lawlist.com>
> > 
> > In implementing feature requests crosshairs (17684) and multiple fake cursors (22873), I thought it would be nifty to identify the background color (if applicable) associated with a tab ('\t') glyph code in the buffer-display-table.  In doing so, I discovered that the Lisp function face-list is way too slow.  Even after I ported the face-list function over to C, it was still way too slow.  So, I tried my luck at adding a new variable in xfaces.c for the face-list and am setting it from within the function internal-make-lisp-face, like so:
> 
> I don't understand why you need face-list for this.  For each glyph in
> the display table, you get its face ID by using the GLYPH_FACE macro.
> Why isn't that sufficient for your needs?

Re: Adding new variable for face-list in internal-make-lisp-face.

[Keith David Bershatsky]

It has been a long day, but I wanted to give you an update on my findings thus far this evening before turning off the computer.

There is no good reason (that I can see) why the Lisp function glyph-face should be using face-list.  Instead, the Lisp function glyph-face should be mapping through face-new-frame-defaults and comparing the car of each element to the face-id.  Using that new method of just checking the car of face-new-frame-defaults works sufficiently quickly in C (as far as I can tell).  So the need for speed appears to be resolved in my early tests tonight.

The problem with an invalid lisp object happens after manually calling eval-defun on a defface when the background has been changed by the user.  In that situation, the face-id changes.  However, the buffer-display-table does NOT update to reflect the new face id.  In my situation, the original face had an id of 121.  After changing the background color (with eval-defun on a defface), the face id changed from 121 to 1196.

So we cannot rely on the buffer-display-table cons cell with the glyph code for a tab face after calling eval-defun, because it still has the number 121.

So, the question is how to get the correct new face id number, which in this case changed from 121 to 1196 after calling eval-defun.  I have only just begun to experiment with GLYPH_FACE to try and extract the face-id for a tab that is assigned a face in the buffer-display-table.  The first attempt ended up returning a face-id of 0 which is wrong, so I need to see why I got the default face instead of 121 or 1196.  I will work on this again over the weekend.

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-03-2017 03:17:05] <03 Nov 2017 12:17:05 +0200>
FROM:  Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Thu, 02 Nov 2017 20:50:47 -0700
> > From: Keith David Bershatsky <esq@lawlist.com>
> > 
> > In implementing feature requests crosshairs (17684) and multiple fake cursors (22873), I thought it would be nifty to identify the background color (if applicable) associated with a tab ('\t') glyph code in the buffer-display-table.  In doing so, I discovered that the Lisp function face-list is way too slow.  Even after I ported the face-list function over to C, it was still way too slow.  So, I tried my luck at adding a new variable in xfaces.c for the face-list and am setting it from within the function internal-make-lisp-face, like so:
> 
> I don't understand why you need face-list for this.  For each glyph in
> the display table, you get its face ID by using the GLYPH_FACE macro.
> Why isn't that sufficient for your needs?

Re: Adding new variable for face-list in internal-make-lisp-face.

[Eli Zaretskii]

> Date:  Fri, 03 Nov 2017 15:01:34 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  emacs-devel@gnu.org
> 
> I have a tab buffer-display-table entry that looks like:
> 
> [(187 . 120) (9 . 121)]
> 
> Your suggestion to use GLYPH_FACE would be more efficient than my using the C equivalent of:
> 
> (aref buffer-display-table ?\t)
> 
> ... and then iterating over the vector elements looking for a cons cell containing the number 9.
> 
> I will work on incorporating GLYPH_FACE into my code, instead of using the above iteration.  Thank you for the suggestion.
> 
> Once the face id is obtained, I have been using C implementations of the following:

I believe I already explained how to get the fore- and back-ground
colors of a face given its ID in my message Re: Can we use FRAME_RIF
to return a Lisp_Object result?  I think you should use the same
technique here.

Re: Adding new variable for face-list in internal-make-lisp-face.

[Eli Zaretskii]

> Date:  Fri, 03 Nov 2017 23:56:53 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  emacs-devel@gnu.org
> 
> It has been a long day, but I wanted to give you an update on my findings thus far this evening before turning off the computer.
> 
> There is no good reason (that I can see) why the Lisp function glyph-face should be using face-list.  Instead, the Lisp function glyph-face should be mapping through face-new-frame-defaults and comparing the car of each element to the face-id.  Using that new method of just checking the car of face-new-frame-defaults works sufficiently quickly in C (as far as I can tell).  So the need for speed appears to be resolved in my early tests tonight.
> 
> The problem with an invalid lisp object happens after manually calling eval-defun on a defface when the background has been changed by the user.  In that situation, the face-id changes.  However, the buffer-display-table does NOT update to reflect the new face id.  In my situation, the original face had an id of 121.  After changing the background color (with eval-defun on a defface), the face id changed from 121 to 1196.

Face IDs are ephemeral, because Emacs frees all faces and realizes
them anew from time to time, when it thinks the previous definitions
might be invalid.  E.g., when the attributes of the default face
change, all the faces need to be recomputed, because many of them
inherit from the default face.

So if you use the face ID, you need to recompute it each time you need
it.  You cannot compute it once and then reuse the same value, because
sooner or later it will become invalid.

> So we cannot rely on the buffer-display-table cons cell with the glyph code for a tab face after calling eval-defun, because it still has the number 121.

See next_element_from_display_vector for how the faces of glyphs from
a display table are used to display those glyphs.  And because of what
I said above, once you have the face ID, you need to use lookup_*_face
functions to get the face corresponding to that ID.

Re: Adding new variable for face-list in internal-make-lisp-face.

[Keith David Bershatsky]

What a wonderful treat it was to check my emails this morning and find the ingredients to properly calculating faces and its components!  :)

I ran into one little `emacs_abort` snag this morning while trying the following two test snippets, which came about when I changed the face background with `face-remap-add-relative` on a face that had been previously defined with `defface`.

Should I run a pre-test for all 15 cases in lookup_basic_face to see if it would throw an `emacs_abort`, and then *only* call lookup_basic_face if success is guaranteed?

Alternatively, perhaps using it.face_id obviates the need to use lookup_basic_face?

TEST SNIPPETS:

  struct face *tab_face = FACE_FROM_ID (f, lookup_basic_face (f, it.face_id));
  Lisp_Object tab_bg = tab_face->lface[LFACE_BACKGROUND_INDEX];
  AUTO_STRING (my_string_one, "IT background:  %s");
  CALLN (Fmessage, my_string_one, tab_bg);

#ifdef GLYPH_DEBUG
  debug_method_add (w, "face_id: %d", it.face_id);
#endif


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-04-2017 01:13:33] <04 Nov 2017 10:13:33 +0200>
FROM:  Eli Zaretskii <eliz@gnu.org>
> 
> * * *
> 
> Face IDs are ephemeral, because Emacs frees all faces and realizes
> them anew from time to time, when it thinks the previous definitions
> might be invalid.  E.g., when the attributes of the default face
> change, all the faces need to be recomputed, because many of them
> inherit from the default face.
> 
> So if you use the face ID, you need to recompute it each time you need
> it.  You cannot compute it once and then reuse the same value, because
> sooner or later it will become invalid.
> 
> * * *
> 
> See next_element_from_display_vector for how the faces of glyphs from
> a display table are used to display those glyphs.  And because of what
> I said above, once you have the face ID, you need to use lookup_*_face
> functions to get the face corresponding to that ID.

> * * *
> 
> I believe I already explained how to get the fore- and back-ground
> colors of a face given its ID in my message Re: Can we use FRAME_RIF
> to return a Lisp_Object result?  I think you should use the same
> technique here.

Re: Adding new variable for face-list in internal-make-lisp-face.

[Eli Zaretskii]

> Date:  Sat, 04 Nov 2017 10:46:17 -0700
> From:  Keith David Bershatsky <esq@lawlist.com>
> Cc:  emacs-devel@gnu.org
> 
> I ran into one little `emacs_abort` snag this morning while trying the following two test snippets, which came about when I changed the face background with `face-remap-add-relative` on a face that had been previously defined with `defface`.
> 
> Should I run a pre-test for all 15 cases in lookup_basic_face to see if it would throw an `emacs_abort`, and then *only* call lookup_basic_face if success is guaranteed?
> 
> Alternatively, perhaps using it.face_id obviates the need to use lookup_basic_face?
> 
> TEST SNIPPETS:
> 
>   struct face *tab_face = FACE_FROM_ID (f, lookup_basic_face (f, it.face_id));
>   Lisp_Object tab_bg = tab_face->lface[LFACE_BACKGROUND_INDEX];
>   AUTO_STRING (my_string_one, "IT background:  %s");
>   CALLN (Fmessage, my_string_one, tab_bg);

You only need to call lookup_basic_face if it.face_id is a basic face
AND face-remapping-alist is non-nil.  If it.face_id is not a basic
face, lookup_basic_face will abort, and you should call lookup_face
instead.  For a remapped non-basic face you will have to look up
face-remapping-alist manually and then use lookup_named_face.