FRAME arg in face-equal and internal-lisp-face-equal-p

FRAME arg in face-equal and internal-lisp-face-equal-p

[Juanma Barranquero]

The implementation of `internal-lisp-face-equal-p' does not seem to
use the FRAME argument:

  int equal_p;
  struct frame *f;
  Lisp_Object lface1, lface2;

  if (EQ (frame, Qt))
    f = NULL;
  else
    /* Don't use check_x_frame here because this function is called
       before X frames exist.  At that time, if FRAME is nil,
       selected_frame will be used which is the frame dumped with
       Emacs.  That frame is not an X frame.  */
    f = frame_or_selected_frame (frame, 2);

  lface1 = lface_from_face_name (NULL, face1, 1);
  lface2 = lface_from_face_name (NULL, face2, 1);
  equal_p = lface_equal_p (XVECTOR (lface1)->contents,
			   XVECTOR (lface2)->contents);
  return equal_p ? Qt : Qnil;

The fix it is trivial, and in my tests works OK, but I wonder whether
there is a reason not to have fixed it (the code is unchanged since
Gerd's initial rewrite on 1999-09-23), or just was overlooked?

(The patch includes also fixes for the docstrings of `face-equal' and
`internal-lisp-equal-p')

-- 
                    /L/e/k/t/u


Index: src/xfaces.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/xfaces.c,v
retrieving revision 1.322
diff -u -2 -r1.322 xfaces.c
--- src/xfaces.c	1 Jun 2005 08:21:25 -0000	1.322
+++ src/xfaces.c	1 Jun 2005 12:57:43 -0000
@@ -5023,6 +5023,6 @@
        Sinternal_lisp_face_equal_p, 2, 3, 0,
        doc: /* True if FACE1 and FACE2 are equal.
-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 the optional argument FRAME is given, report on FACE1 and FACE2 in
that frame.
+If FRAME is t, report on the defaults for FACE1 and FACE2 (for new frames).
 If FRAME is omitted or nil, use the selected frame.  */)
      (face1, face2, frame)
@@ -5042,6 +5042,6 @@
     f = frame_or_selected_frame (frame, 2);
 
-  lface1 = lface_from_face_name (NULL, face1, 1);
-  lface2 = lface_from_face_name (NULL, face2, 1);
+  lface1 = lface_from_face_name (f, face1, 1);
+  lface2 = lface_from_face_name (f, face2, 1);
   equal_p = lface_equal_p (XVECTOR (lface1)->contents,
  			   XVECTOR (lface2)->contents);
Index: lisp/faces.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/faces.el,v
retrieving revision 1.311
diff -u -2 -r1.311 faces.el
--- lisp/faces.el	1 Jun 2005 10:42:47 -0000	1.311
+++ lisp/faces.el	1 Jun 2005 13:00:36 -0000
@@ -232,6 +232,6 @@
   "Non-nil if faces FACE1 and FACE2 are equal.
 Faces are considered equal if all their attributes are equal.
-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 the optional argument FRAME is given, report on FACE1 and FACE2 in
that frame.
+If FRAME is t, report on the defaults for FACE1 and FACE2 (for new frames).
 If FRAME is omitted or nil, use the selected frame."
   (internal-lisp-face-equal-p face1 face2 frame))

Re: FRAME arg in face-equal and internal-lisp-face-equal-p

[Juanma Barranquero]

BTW, having `internal-lisp-face-equal-p' doing this:

>   lface1 = lface_from_face_name (NULL, face1, 1);
>   lface2 = lface_from_face_name (NULL, face2, 1);

means that, in fact, `equal-face' erroneously compares faces based on
their default values...

-- 
                    /L/e/k/t/u

Re: FRAME arg in face-equal and internal-lisp-face-equal-p

[Juanma Barranquero]

> The implementation of `internal-lisp-face-equal-p' does not seem to
> use the FRAME argument:

I've installed the fix to Finternal_lisp_face_equal_p.

I don't expect any trouble because `face-equal' is not widely used, at
least not on the Emacs sources: only `facemenu-add-new-face', which in
turn is just used from `make-face'. OTOH, `facemenu-add-new-face'
calls `face-equal' with no FRAME argument, which was stated to use the
value of the faces for the selected frame, and was really using the
default value of the faces. Thus, it is potentially a change in
behavior.

So, if some unexpected problem arises with faces, try removing this patch first.

-- 
                    /L/e/k/t/u