+face-remapping-20040505-0.patch

+face-remapping-20040505-0.patch

[Miles Bader]

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

"Robert J. Chassell" <bob@rattlesnake.com> wrote:
> in your patch for 2004 May  4
>     +face-remapping-20040504-0.patch
> one hunk failed:

Here's a new patch, which is re-diffed against the CVS trunk, and also
includes a bug-fix:



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: +face-remapping-20040505-0.patch --]
[-- Type: text/x-patch, Size: 18511 bytes --]

* looking for miles@gnu.org--gnu-2004/emacs--cvs-trunk--0--patch-276 to compare with
* comparing to miles@gnu.org--gnu-2004/emacs--cvs-trunk--0--patch-276
M  src/xfaces.c
M  src/dispextern.h
M  src/fontset.c
M  src/xdisp.c

* modified files

--- orig/src/dispextern.h
+++ mod/src/dispextern.h
@@ -2715,6 +2715,7 @@
 int xstricmp P_ ((const unsigned char *, const unsigned char *));
 int lookup_face P_ ((struct frame *, Lisp_Object *, int, struct face *));
 int lookup_named_face P_ ((struct frame *, Lisp_Object, int));
+int lookup_basic_face P_ ((struct frame *, int));
 int smaller_face P_ ((struct frame *, int, int));
 int face_with_height P_ ((struct frame *, int, int));
 int lookup_derived_face P_ ((struct frame *, Lisp_Object, int, int));
@@ -2731,6 +2732,8 @@
 extern char unspecified_fg[], unspecified_bg[];
 void free_realized_multibyte_face P_ ((struct frame *, int));
 
+extern Lisp_Object Vface_remapping_alist;
+
 /* Defined in xfns.c  */
 
 #ifdef HAVE_X_WINDOWS


--- orig/src/fontset.c
+++ mod/src/fontset.c
@@ -1252,7 +1252,7 @@
       CHECK_NATNUM (ch);
       c = XINT (ch);
       f = XFRAME (selected_frame);
-      face_id = DEFAULT_FACE_ID;
+      face_id = lookup_basic_face (f, DEFAULT_FACE_ID);
     }
   else
     {


--- orig/src/xdisp.c
+++ mod/src/xdisp.c
@@ -2027,6 +2027,7 @@
      enum face_id base_face_id;
 {
   int highlight_region_p;
+  enum face_id remapped_base_face_id = base_face_id;
 
   /* Some precondition checks.  */
   xassert (w != NULL && it != NULL);
@@ -2043,6 +2044,10 @@
       free_all_realized_faces (Qnil);
     }
 
+  /* Perhaps remap BASE_FACE_ID to a user-specified alternative.  */
+  if (! NILP (Vface_remapping_alist))
+    remapped_base_face_id = lookup_basic_face (XFRAME (w->frame), base_face_id);
+
   /* Use one of the mode line rows of W's desired matrix if
      appropriate.  */
   if (row == NULL)
@@ -2058,7 +2063,7 @@
   bzero (it, sizeof *it);
   it->current.overlay_string_index = -1;
   it->current.dpvec_index = -1;
-  it->base_face_id = base_face_id;
+  it->base_face_id = remapped_base_face_id;
   it->string = Qnil;
   IT_STRING_CHARPOS (*it) = IT_STRING_BYTEPOS (*it) = -1;
 
@@ -2242,11 +2247,11 @@
     {
       struct face *face;
 
-      it->face_id = base_face_id;
+      it->face_id = remapped_base_face_id;
 
       /* If we have a boxed mode line, make the first character appear
 	 with a left box line.  */
-      face = FACE_FROM_ID (it->f, base_face_id);
+      face = FACE_FROM_ID (it->f, remapped_base_face_id);
       if (face->box != FACE_NO_BOX)
 	it->start_of_box_run_p = 1;
     }
@@ -3481,7 +3486,8 @@
 	      /* Value is a multiple of the canonical char height.  */
 	      struct face *face;
 
-	      face = FACE_FROM_ID (it->f, DEFAULT_FACE_ID);
+	      face = FACE_FROM_ID (it->f,
+				   lookup_basic_face (it->f, DEFAULT_FACE_ID));
 	      new_height = (XFLOATINT (it->font_height)
 			    * XINT (face->lface[LFACE_HEIGHT_INDEX]));
 	    }
@@ -3581,7 +3587,7 @@
 	      || EQ (XCAR (prop), Qright_fringe))
 	  && CONSP (XCDR (prop)))
 	{
-	  unsigned face_id = DEFAULT_FACE_ID;
+	  unsigned face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
 
 	  /* Save current settings of IT so that we can restore them
 	     when we are finished with the glyph property value.  */


--- orig/src/xfaces.c
+++ mod/src/xfaces.c
@@ -396,6 +396,13 @@
 
 Lisp_Object Vface_new_frame_defaults;
 
+/* Alist of face mappings.  Each element is either of the form
+   (FACE . NEW-FACE), or (FACE NEW-FACE MERGE-FACE...),
+   where FACE is the named used for lookups, and NEW-FACE is the name
+   that actually gets looked up.  If present, MERGE-FACE... are merged
+   during display of FACE, with NEW-FACE.  */
+Lisp_Object Vface_remapping_alist;
+
 /* The next ID to assign to Lisp faces.  */
 
 static int next_lface_id;
@@ -471,7 +478,7 @@
 static int x_face_list_fonts P_ ((struct frame *, char *,
 				  struct font_name **, int, int));
 static int font_scalable_p P_ ((struct font_name *));
-static int get_lface_attributes P_ ((struct frame *, Lisp_Object, Lisp_Object *, int));
+static int get_lface_attributes P_ ((struct frame *, Lisp_Object, Lisp_Object *, int, Lisp_Object));
 static int load_pixmap P_ ((struct frame *, Lisp_Object, unsigned *, unsigned *));
 static unsigned char *xstrlwr P_ ((unsigned char *));
 static void signal_error P_ ((char *, Lisp_Object));
@@ -3178,24 +3185,19 @@
 
 
 /* Return the face definition of FACE_NAME on frame F.  F null means
-   return the definition for new frames.  FACE_NAME may be a string or
-   a symbol (apparently Emacs 20.2 allowed strings as face names in
-   face text properties; Ediff uses that).  If FACE_NAME is an alias
-   for another face, return that face's definition.  If SIGNAL_P is
-   non-zero, signal an error if FACE_NAME is not a valid face name.
-   If SIGNAL_P is zero, value is nil if FACE_NAME is not a valid face
-   name.  */
-
+   return the definition for new frames.  FACE_NAME may be a string or a
+   symbol (apparently Emacs 20.2 allowed strings as face names in face
+   text properties; Ediff uses that).  If SIGNAL_P is non-zero, signal
+   an error if FACE_NAME is not a valid face name.  If SIGNAL_P is zero,
+   value is nil if FACE_NAME is not a valid face name.  */
 static INLINE Lisp_Object
-lface_from_face_name (f, face_name, signal_p)
+lface_from_face_name_no_resolve (f, face_name, signal_p)
      struct frame *f;
      Lisp_Object face_name;
      int signal_p;
 {
   Lisp_Object lface;
 
-  face_name = resolve_face_name (face_name);
-
   if (f)
     lface = assq_no_quit (face_name, f->face_alist);
   else
@@ -3207,9 +3209,27 @@
     signal_error ("Invalid face", face_name);
 
   check_lface (lface);
+
   return lface;
 }
 
+/* Return the face definition of FACE_NAME on frame F.  F null means
+   return the definition for new frames.  FACE_NAME may be a string or
+   a symbol (apparently Emacs 20.2 allowed strings as face names in
+   face text properties; Ediff uses that).  If FACE_NAME is an alias
+   for another face, return that face's definition.  If SIGNAL_P is
+   non-zero, signal an error if FACE_NAME is not a valid face name.
+   If SIGNAL_P is zero, value is nil if FACE_NAME is not a valid face
+   name.  */
+static INLINE Lisp_Object
+lface_from_face_name (f, face_name, signal_p)
+     struct frame *f;
+     Lisp_Object face_name;
+     int signal_p;
+{
+  return lface_from_face_name_no_resolve (f, face_name, signal_p);
+}
+
 
 /* Get face attributes of face FACE_NAME from frame-local faces on
    frame F.  Store the resulting attributes in ATTRS which must point
@@ -3218,26 +3238,91 @@
    Otherwise, value is zero if FACE_NAME is not a face.  */
 
 static INLINE int
-get_lface_attributes (f, face_name, attrs, signal_p)
+get_lface_attributes_no_remap (f, face_name, attrs, signal_p)
      struct frame *f;
      Lisp_Object face_name;
      Lisp_Object *attrs;
      int signal_p;
 {
   Lisp_Object lface;
-  int success_p;
 
-  lface = lface_from_face_name (f, face_name, signal_p);
-  if (!NILP (lface))
-    {
-      bcopy (XVECTOR (lface)->contents, attrs,
-	     LFACE_VECTOR_SIZE * sizeof *attrs);
-      success_p = 1;
+  lface = lface_from_face_name_no_resolve (f, face_name, signal_p);
+
+  if (! NILP (lface))
+    bcopy (XVECTOR (lface)->contents, attrs,
+	   LFACE_VECTOR_SIZE * sizeof *attrs);
+
+  return !NILP (lface);
+}
+
+/* Get face attributes of face FACE_NAME from frame-local faces on frame
+   F.  Store the resulting attributes in ATTRS which must point to a
+   vector of Lisp_Objects of size LFACE_VECTOR_SIZE.  If FACE_NAME is an
+   alias for another face, use that face's definition.  If SIGNAL_P is
+   non-zero, signal an error if FACE_NAME does not name a face.
+   Otherwise, value is zero if FACE_NAME is not a face.  */
+
+static INLINE int
+get_lface_attributes (f, face_name, attrs, signal_p, cycle_check)
+     struct frame *f;
+     Lisp_Object face_name;
+     Lisp_Object *attrs;
+     int signal_p;
+     Lisp_Object cycle_check;
+{
+  Lisp_Object lface;
+  Lisp_Object face_remapping;
+
+  face_name = resolve_face_name (face_name);
+
+  /* See if SYMBOL has been remapped to some other face (usually this
+     is done buffer-locally).  */
+  face_remapping = assq_no_quit (face_name, Vface_remapping_alist);
+  if (CONSP (face_remapping))
+    {  
+      /* Make sure we're not in an mapping loop.  */
+      cycle_check = CYCLE_CHECK (cycle_check, face_name, 15);
+
+      if (! NILP (cycle_check))
+	{
+	  /* No cycle detected, lookup FACE_NAME's mapping instead.  */
+
+	  face_remapping = XCDR (face_remapping);
+
+	  /* A mapping may also contain a list of `merge faces', which
+	     we ignore in this function.  */
+	  if (CONSP (face_remapping))
+	    {
+	      int first_ok;
+	      Lisp_Object first_face;
+
+	      first_face = XCAR (face_remapping);
+
+	      /* See if this is a trivial recursion, and handle it
+		 properly without incuring a cycle-check penalty.  */
+	      if (EQ (first_face, face_name) || NILP (first_face))
+		first_ok = get_lface_attributes_no_remap (f, face_name, attrs,
+							  signal_p);
+	      else
+		first_ok = get_lface_attributes (f, first_face, attrs,
+						 signal_p, cycle_check);
+		
+	      if (first_ok)
+		{
+		  /* Merge in any remaining faces.  */
+
+		  face_remapping = XCDR (face_remapping);
+		  merge_face_inheritance (f, face_remapping, attrs,
+					  cycle_check);
+		}
+
+	      return first_ok;
+	    }
+	}
     }
-  else
-    success_p = 0;
 
-  return success_p;
+  /* Default case, no remapping.  */
+  return get_lface_attributes_no_remap (f, face_name, attrs, signal_p);
 }
 
 
@@ -3479,11 +3564,29 @@
   to[LFACE_INHERIT_INDEX] = Qnil;
 }
 
+/* Merge the named face FACE_NAME on frame F, into the vector of face
+   attributes TO CYCLE_CHECK is used to detect loops in face
+   inheritance.  Returns true if FACE_NAME is a valid face name, and
+   false otherwise.  */
+
+static int
+merge_named_face (f, face_name, to, cycle_check)
+     struct frame *f;
+     Lisp_Object face_name;
+     Lisp_Object *to;
+     Lisp_Object cycle_check;
+{
+  Lisp_Object from[LFACE_VECTOR_SIZE];
+  int ok = get_lface_attributes (f, face_name, from, 0, cycle_check);
+  if (ok)
+    merge_face_vectors (f, from, to, cycle_check);
+  return ok;
+}
+
 /* Merge face attributes from the face on frame F whose name is
    INHERITS, into the vector of face attributes TO; INHERITS may also be
    a list of face names, in which case they are applied in order.
-   CYCLE_CHECK is used to detect loops in face inheritance.
-   Returns true if any of the inherited attributes are `font-related'.  */
+   CYCLE_CHECK is used to detect loops in face inheritance.  */
 
 static void
 merge_face_inheritance (f, inherit, to, cycle_check)
@@ -3495,17 +3598,13 @@
   if (SYMBOLP (inherit) && !EQ (inherit, Qunspecified))
     /* Inherit from the named face INHERIT.  */
     {
-      Lisp_Object lface;
-
       /* Make sure we're not in an inheritance loop.  */
       cycle_check = CYCLE_CHECK (cycle_check, inherit, 15);
       if (NILP (cycle_check))
 	/* Cycle detected, ignore any further inheritance.  */
 	return;
 
-      lface = lface_from_face_name (f, inherit, 0);
-      if (!NILP (lface))
-	merge_face_vectors (f, XVECTOR (lface)->contents, to, cycle_check);
+      merge_named_face (f, inherit, to, cycle_check);
     }
   else if (CONSP (inherit))
     /* Handle a list of inherited faces by calling ourselves recursively
@@ -3736,11 +3835,8 @@
   else
     {
       /* PROP ought to be a face name.  */
-      Lisp_Object lface = lface_from_face_name (f, prop, 0);
-      if (NILP (lface))
+      if (! merge_named_face (f, prop, to, Qnil))
 	add_to_log ("Invalid face text property value: %s", prop, Qnil);
-      else
-	merge_face_vectors (f, XVECTOR (lface)->contents, to, Qnil);
     }
 }
 
@@ -5734,11 +5830,12 @@
    face couldn't be determined, which might happen if the default face
    isn't realized and cannot be realized.  */
 
-int
-lookup_named_face (f, symbol, c)
+static int
+lookup_named_face_1 (f, symbol, c, signal_p)
      struct frame *f;
      Lisp_Object symbol;
      int c;
+     int signal_p;
 {
   Lisp_Object attrs[LFACE_VECTOR_SIZE];
   Lisp_Object symbol_attrs[LFACE_VECTOR_SIZE];
@@ -5751,12 +5848,76 @@
       default_face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
     }
 
-  get_lface_attributes (f, symbol, symbol_attrs, 1);
+  if (! get_lface_attributes (f, symbol, symbol_attrs, signal_p, Qnil))
+    return -1;
+
   bcopy (default_face->lface, attrs, sizeof attrs);
   merge_face_vectors (f, symbol_attrs, attrs, Qnil);
+
   return lookup_face (f, attrs, c, NULL);
 }
 
+/* Return the face id of the realized face for named face SYMBOL on
+   frame F suitable for displaying character C.  Value is -1 if the
+   face couldn't be determined, which might happen if the default face
+   isn't realized and cannot be realized.  */
+
+int
+lookup_named_face (f, symbol, c)
+     struct frame *f;
+     Lisp_Object symbol;
+     int c;
+{
+  return lookup_named_face_1 (f, symbol, c);
+}
+
+
+/* Return the display face-id of the basic face who's canonical face-id
+   is FACE_ID.  The return value will usually simply be FACE_ID, unless that
+   basic face has bee remapped via Vface_remapping_alist.  This function is
+   conservative: if something goes wrong, it will simply return FACE_ID
+   rather than signal an error.   */
+
+int
+lookup_basic_face (f, face_id)
+     struct frame *f;
+     int face_id;
+{
+  Lisp_Object name, mapping;
+  int remapped_face_id;
+
+  if (NILP (Vface_remapping_alist))
+    return face_id;		/* Nothing to do.  */
+
+  switch (face_id)
+    {
+    case DEFAULT_FACE_ID:		name = Qdefault;		break;
+    case MODE_LINE_FACE_ID:		name = Qmode_line;		break;
+    case MODE_LINE_INACTIVE_FACE_ID:	name = Qmode_line_inactive;	break;
+    case HEADER_LINE_FACE_ID:		name = Qheader_line;		break;
+    case TOOL_BAR_FACE_ID:		name = Qtool_bar;		break;
+    case FRINGE_FACE_ID:		name = Qfringe;			break;
+    case SCROLL_BAR_FACE_ID:		name = Qscroll_bar;		break;
+    case BORDER_FACE_ID:		name = Qborder;			break;
+    case CURSOR_FACE_ID:		name = Qcursor;			break;
+    case MOUSE_FACE_ID:			name = Qmouse;			break;
+    case MENU_FACE_ID:			name = Qmenu;			break;
+
+    default:
+      return face_id;		/* Give up.  */
+    }
+
+  mapping = assq_no_quit (name, Vface_remapping_alist);
+  if (NILP (mapping))
+    return face_id;		/* Give up.  */
+
+  remapped_face_id = lookup_named_face_1 (f, name, 0, 0);
+  if (remapped_face_id < 0)
+    return face_id;		/* Give up. */
+
+  return remapped_face_id;
+}
+
 
 /* Return the ID of the realized ASCII face of Lisp face with ID
    LFACE_ID on frame F.  Value is -1 if LFACE_ID isn't valid.  */
@@ -5890,7 +6051,7 @@
   if (!default_face)
     abort ();
 
-  get_lface_attributes (f, symbol, symbol_attrs, 1);
+  get_lface_attributes (f, symbol, symbol_attrs, 1, Qnil);
   bcopy (default_face->lface, attrs, sizeof attrs);
   merge_face_vectors (f, symbol_attrs, attrs, Qnil);
   return lookup_face (f, attrs, c, default_face);
@@ -6753,7 +6914,7 @@
   struct face *new_face;
 
   /* The default face must exist and be fully specified.  */
-  get_lface_attributes (f, Qdefault, attrs, 1);
+  get_lface_attributes_no_remap (f, Qdefault, attrs, 1);
   check_lface_attrs (attrs);
   xassert (lface_fully_specified_p (attrs));
 
@@ -6766,7 +6927,7 @@
     }
 
   /* Merge SYMBOL's face with the default face.  */
-  get_lface_attributes (f, symbol, symbol_attrs, 1);
+  get_lface_attributes_no_remap (f, symbol, symbol_attrs, 1);
   merge_face_vectors (f, symbol_attrs, attrs, Qnil);
 
   /* Realize the face.  */
@@ -7321,13 +7482,18 @@
 
   *endptr = endpos;
 
-  default_face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
+
+  /* Perhaps remap BASE_FACE_ID to a user-specified alternative.  */
+  if (NILP (Vface_remapping_alist))
+    default_face = FACE_FROM_ID (f, DEFAULT_FACE_ID);
+  else
+    default_face = FACE_FROM_ID (f, lookup_basic_face (f, DEFAULT_FACE_ID));
 
   /* Optimize common cases where we can use the default face.  */
   if (noverlays == 0
       && NILP (prop)
       && !(pos >= region_beg && pos < region_end))
-    return DEFAULT_FACE_ID;
+    return default_face->id;
 
   /* Begin with attributes from the default face.  */
   bcopy (default_face->lface, attrs, sizeof attrs);
@@ -7356,8 +7522,7 @@
   /* If in the region, merge in the region face.  */
   if (pos >= region_beg && pos < region_end)
     {
-      Lisp_Object region_face = lface_from_face_name (f, Qregion, 0);
-      merge_face_vectors (f, XVECTOR (region_face)->contents, attrs, Qnil);
+      merge_named_face (f, Qregion, attrs);
 
       if (region_end < endpos)
 	endpos = region_end;
@@ -7459,10 +7624,7 @@
   if (bufpos
       && bufpos >= region_beg
       && bufpos < region_end)
-    {
-      Lisp_Object region_face = lface_from_face_name (f, Qregion, 0);
-      merge_face_vectors (f, XVECTOR (region_face)->contents, attrs, Qnil);
-    }
+    merge_named_face (f, Qregion, attrs, Qnil);
 
   /* Look up a realized face with the given face attributes,
      or realize a new one for ASCII characters.  */
@@ -7783,6 +7945,28 @@
 ignore.  */);
   Vface_ignored_fonts = Qnil;
 
+  DEFVAR_LISP ("face-remapping-alist", &Vface_remapping_alist,
+	       doc: /* Alist of face remappings.
+Each element is of the form:
+   (OLD-FACE REPLACEMENT-FACE...),
+which causes uses of the face OLD-FACE to use
+REPLACEMENT-FACE... instead.  If more than one replacement face is
+specified, they are merged together.
+
+Face-name remapping cycles are suppressed, causing the underlying face
+to be used instead, so a remapping of the form:
+   (OLD-FACE OLD-FACE EXTRA-FACE...)
+will cause EXTRA-FACE... to be _merged_ with the existing definition of
+OLD-FACE.  For conciseness, the form (OLD-FACE nil EXTRA-FACE....) is
+treated the same way.  Note that for the default face, this isn't
+necessary, as every face inherits from the default face.
+
+Making this variable buffer-local is a good way to allow buffer-specific
+face definitions, for instance, the mode my-mode could define a face
+`my-mode-default', and then in the mode setup function, do
+(set (make-local-variable 'face-remapping-alist) '((default my-mode-default)))).  */);
+  Vface_remapping_alist = Qnil;
+
   DEFVAR_LISP ("face-font-rescale-alist", &Vface_font_rescale_alist,
 	       doc: /* Alist of fonts vs the rescaling factors.
 Each element is a cons (FONT-NAME-PATTERN . RESCALE-RATIO), where




[-- Attachment #3: Type: text/plain, Size: 217 bytes --]



Thanks,

-Miles
-- 
`Cars give people wonderful freedom and increase their opportunities.
 But they also destroy the environment, to an extent so drastic that
 they kill all social life' (from _A Pattern Language_)

[-- Attachment #4: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

    +/* Alist of face mappings.  Each element is either of the form
    +   (FACE . NEW-FACE), or (FACE NEW-FACE MERGE-FACE...),

Specifying just NEW-FACE is a special case of specifying a list of
faces.  So I think it is redundant to have a special way to specify
just one face.  It's extra complexity that adds nothing really.

This specification uses the first face in the list, NEW-FACE, last.  I
think it should use these faces in the order they appear.

Re: +face-remapping-20040505-0.patch

[Miles Bader]

On Wed, May 05, 2004 at 04:20:35PM -0400, Richard Stallman wrote:
> Specifying just NEW-FACE is a special case of specifying a list of
> faces.  So I think it is redundant to have a special way to specify
> just one face.  It's extra complexity that adds nothing really.

Actually, I've already changed it to work the way you describe, it's just the
C-comment that's out of date (look at the docstring for a more up-to-date
description).

> This specification uses the first face in the list, NEW-FACE, last.  I
> think it should use these faces in the order they appear.

Yes, it does (even if that comment makes it sound otherwise).

-Miles
-- 
I'm beginning to think that life is just one long Yoko Ono album; no rhyme
or reason, just a lot of incoherent shrieks and then it's over.  --Ian Wolff

Re: +face-remapping-20040505-0.patch

[Stefan Monnier]

>> This specification uses the first face in the list, NEW-FACE, last.  I
>> think it should use these faces in the order they appear.

It should work the same way as when a list of faces is specified in the
`face' property, shouldn't it ?


        Stefan

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

    >> This specification uses the first face in the list, NEW-FACE, last.  I
    >> think it should use these faces in the order they appear.

    It should work the same way as when a list of faces is specified in the
    `face' property, shouldn't it ?

I think we're saying the same thing in different words.

Re: +face-remapping-20040505-0.patch

[Miles Bader]

On Fri, May 07, 2004 at 09:20:33PM -0400, Richard Stallman wrote:
>     >> This specification uses the first face in the list, NEW-FACE, last.  I
>     >> think it should use these faces in the order they appear.
> 
>     It should work the same way as when a list of faces is specified in the
>     `face' property, shouldn't it ?
> 
> I think we're saying the same thing in different words.

I'm not sure.

The code currently uses the same function that face-inheritance uses (a
face's :inherit attribute may be a list).  Unfortunately, the manner in which
that works is the _opposite_ of how a list of faces in a face property works.

 * In a face property, a list of faces is interpreted so that earlier faces in
   the list _override_ later faces in the list.

 * In an face-inheritance list, _later_ faces override earlier faces.

E.g., for a face property, the following face list:

   (variable-pitch fixed pitch)

will result in the display being `variable-pitch', but if the _same_ list
occurs in a face-inheritance property, the display will use `fixed-pitch'!

I'm not sure what to do about this; both behaviors kind of make sense.  The
:inherit behavior is somewhat closer to the implementation -- it just says
`merge these faces one by one' -- whereas perhaps the face-property behavior
of preferring earlier faces could seem more natural when viewed abstractly,
but is actually a bit more of a pain to implement (it uses recursion to apply
the face list in reverse order).

Use the same code as the face-property for `face-remapping-alist' would have
the advantage that you could use other `magic' face-property values for free,
e.g. directly embedded attributes like (:foreground "green"), but if that
were the case maybe the name `face-remapping-alist' would be slightly wrong?

Along the same lines, perhaps one or the other of the two existing types of
face-lists should be changed to be consistant with the other -- but which
one?

Any thoughts?

Thanks,

-Miles
-- 
Would you like fries with that?

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

    The code currently uses the same function that face-inheritance uses (a
    face's :inherit attribute may be a list).  Unfortunately, the manner in which
    that works is the _opposite_ of how a list of faces in a face property works.

     * In a face property, a list of faces is interpreted so that earlier faces in
       the list _override_ later faces in the list.

     * In an face-inheritance list, _later_ faces override earlier faces.

The order should be reversed for face-inheritance.  The face that
comes first should always override those that come after.

Re: +face-remapping-20040505-0.patch

[Stefan Monnier]

> The order should be reversed for face-inheritance.

Agreed, because it is the least disruptive change (multiple inheritance is
very rarely used, if ever).
I still think that face-remapping should rely on face-inheritance rather
than allow mapping a face to several (merged) faces.


        Stefan

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

    I still think that face-remapping should rely on face-inheritance rather
    than allow mapping a face to several (merged) faces.

I am not quite sure what change you are proposing.
If you are saying that the feature of remapping X to (Y Z)
should be removed, and instead the user should have to
define W as (Y Z) then remap X to W, I think that is
a loss of convenience with nearly zero gain in simplicity.

Re: +face-remapping-20040505-0.patch

[Stefan Monnier]

>     I still think that face-remapping should rely on face-inheritance rather
>     than allow mapping a face to several (merged) faces.
> I am not quite sure what change you are proposing.
> If you are saying that the feature of remapping X to (Y Z)
> should be removed, and instead the user should have to
> define W as (Y Z) then remap X to W, I think that is
> a loss of convenience with nearly zero gain in simplicity.

Well, maybe it doesn't make the code simpler, I don't know.
But it does simplify the explanation of what (face-attribute X :foo)
returns (since we've already dealt with the issue of whether it returns the
info corresponding to Y or to Y+Z or to Y+Z+default).

Also the added convenience of not having to define W might not be
that great.  After all, I expect 99% of the remapping to just be from
X to Y.  Also the intermediate W might/should be added anyway just to make
easier for the user to customize the resulting face.

Maybe it doesn't make the current implementation simple, but it at least
makes the concept simpler.  OTOH, that's assuming that "99% of the remapping
[will] just be from X to Y".  This 99% assumption assumes that mapping X
to (X Y) doesn't work, tho.

OTOH, I must say that the most important use of this remapping facility
would be to tweak faces in various modes, so mapping X to (X Y) would be
the most useful application, if not the only one.  So maybe we should
have face-addition-alist rather face-remapping-alist so we could just put
(region . bold) in order to add bold to the region face.


        Stefan

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

    But it does simplify the explanation of what (face-attribute X :foo)
    returns (since we've already dealt with the issue of whether it returns the
    info corresponding to Y or to Y+Z or to Y+Z+default).

You have lost me here.  How does explaining what (face-attribute X
:foo) means relate to this issue?  This remapping has no effect
on face-attribute.

    Maybe it doesn't make the current implementation simple, but it at least
    makes the concept simpler.  OTOH, that's assuming that "99% of the remapping
    [will] just be from X to Y".  This 99% assumption assumes that mapping X
    to (X Y) doesn't work, tho.

    OTOH, I must say that the most important use of this remapping facility
    would be to tweak faces in various modes, so mapping X to (X Y) would be
    the most useful application, if not the only one.  So maybe we should
    have face-addition-alist rather face-remapping-alist so we could just put
    (region . bold) in order to add bold to the region face.

The existing feature is just as simple as either of these,
and more powerful.  I see no reason to replace it with anything
less powerful.

Re: +face-remapping-20040505-0.patch

[Stefan Monnier]

>     But it does simplify the explanation of what (face-attribute X :foo)
>     returns (since we've already dealt with the issue of whether it
>     returns the info corresponding to Y or to Y+Z or to Y+Z+default).

> You have lost me here.  How does explaining what (face-attribute X
> :foo) means relate to this issue?  This remapping has no effect
> on face-attribute.

If face X is remapped to (Y Z) and face Y is just (:bold t) whereas
face Z is (:font "foo"), what does (face-attribute X :bold) and
(face-attribute X :font) return?


        Stefan

Re: +face-remapping-20040505-0.patch

[Miles Bader]

On Mon, May 10, 2004 at 02:25:05PM -0400, Stefan Monnier wrote:
> If face X is remapped to (Y Z) and face Y is just (:bold t) whereas
> face Z is (:font "foo"), what does (face-attribute X :bold) and
> (face-attribute X :font) return?

Face-remapping only affects redisplay; `face-attribute' will not see it.

[The original patch I posted _did_ affect face-attribute, but the second
patch changed that.]

-Miles
-- 
Ich bin ein Virus. Mach' mit und kopiere mich in Deine .signature.

Re: +face-remapping-20040505-0.patch

[Stefan Monnier]

> Face-remapping only affects redisplay; `face-attribute' will not see it.

Am I the only one who thinks this is wrong?


        Stefan

Re: +face-remapping-20040505-0.patch

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> > Face-remapping only affects redisplay; `face-attribute' will not see it.
> 
> Am I the only one who thinks this is wrong?

Probably.  :-)

-Miles
-- 
/\ /\
(^.^)
(")")
*This is the cute kitty virus, please copy it into your sig so it can spread.

Re: +face-remapping-20040505-0.patch

[Kevin Rodgers]

Stefan Monnier wrote:
 >>Face-remapping only affects redisplay; `face-attribute' will not see it.
 >
 > Am I the only one who thinks this is wrong?

No, it seems wrong to me as well.  Doesn't face-attribute exist to allow
the programmer to find out how a face will actually look?  If
(face-attribute X :weight) returns 'bold even though X isn't displayed
as bold, what's the point?

(face-attribute FACE ATTRIBUTE &optional FRAME IGNORE-REMAPPING) ;-)

-- 
Kevin Rodgers

Re: +face-remapping-20040505-0.patch

[Miles Bader]

Kevin Rodgers <ihs_4664@yahoo.com> writes:
>  >>Face-remapping only affects redisplay; `face-attribute' will not see it.
>
> No, it seems wrong to me as well.  Doesn't face-attribute exist to allow
> the programmer to find out how a face will actually look?

No.

It exists to let you find out the attributes of a face, the inverse of
`set-face-attribute' -- it's a fairly low-level operation.  Note that by
default, it will return `unspecified' for attributes that aren't
_directly_ specified in a face, and this is intentional.

The appearance of text, on the other hand, is not typically due to one
face, but rather the merging of several faces.

The `face-attribute' function _does_ have an optional argument to do
face-merging on the result, so perhaps an additional argument might be
added to do remapping too, analoguous to the current INHERIT argument.
E.g.:

   (face-attribute FACE ATTRIBUTE &optional FRAME INHERIT REMAP)
   ...
   If REMAP is nil, the default frame-global face definitions will be used.
   If REMAP is t, faces will be remapped according to the current
     definition of `face-remapping-alist'.
   If REMAP is an alist, faces will be remapped as if
     `face-remapping-alist' had that value.

-Miles
-- 
Somebody has to do something, and it's just incredibly pathetic that it
has to be us.  -- Jerry Garcia

Re: +face-remapping-20040505-0.patch

[Miles Bader]

Richard Stallman <rms@gnu.org> writes:
>      * In a face property, a list of faces is interpreted so that earlier faces in
>        the list _override_ later faces in the list.
> 
>      * In an face-inheritance list, _later_ faces override earlier faces.
> 
> The order should be reversed for face-inheritance.  The face that
> comes first should always override those that come after.

Ok; that's a slight incompability, so it should have a NEWS entry,
right?

BTW, I think perhaps the name `face-override-alist' is a better
(clearer) name than `face-remapping-alist', especially if it essentially
supports the same syntax as a face property.  What do you think?

-Miles
-- 
`Suppose Korea goes to the World Cup final against Japan and wins,' Moon said.
`All the past could be forgiven.'   [NYT]

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

    BTW, I think perhaps the name `face-override-alist' is a better
    (clearer) name than `face-remapping-alist', especially if it essentially
    supports the same syntax as a face property.  What do you think?

Don't we use the term "remapping" for other such features,
such as the keymap feature to remap one command into another command?

Re: +face-remapping-20040505-0.patch

[Miles Bader]

On Mon, May 10, 2004 at 01:54:20PM -0400, Richard Stallman wrote:
>     BTW, I think perhaps the name `face-override-alist' is a better
>     (clearer) name than `face-remapping-alist', especially if it essentially
>     supports the same syntax as a face property.  What do you think?
> 
> Don't we use the term "remapping" for other such features,
> such as the keymap feature to remap one command into another command?

The one instance I find (via apropos) is:

   command-remapping
     Function: Return the remapping for command COMMAND in current keymaps.

[`remap' yields the same result.]

The reasons I like override better are:

 (1) Its meaning is pretty obvious, and more visceral.  For instance I think
     if someone didn't know about this feature, and was trying to find out
     why their faces looked weird, they'd be more likely to chance upon or
     recognize `override' as being the likely culprit than `remapping'.

 (2) Remapping does seem more to imply a one-to-one replacement of one face
     name with another; if indeed the feature allows merging faces, or even
     un-named face attributes to be used, then I think override fits better.

-Miles
-- 
We have met the enemy, and he is us.  -- Pogo

Re: +face-remapping-20040505-0.patch

[Kim F. Storm]

Miles Bader <miles@gnu.org> writes:

> On Mon, May 10, 2004 at 01:54:20PM -0400, Richard Stallman wrote:
> >     BTW, I think perhaps the name `face-override-alist' is a better
> >     (clearer) name than `face-remapping-alist', especially if it essentially
> >     supports the same syntax as a face property.  What do you think?
> > 
> > Don't we use the term "remapping" for other such features,
> > such as the keymap feature to remap one command into another command?
> 
> The one instance I find (via apropos) is:
> 
>    command-remapping
>      Function: Return the remapping for command COMMAND in current keymaps.
> 
> [`remap' yields the same result.]
> 
> The reasons I like override better are:
> 
>  (1) Its meaning is pretty obvious, and more visceral.  For instance I think
>      if someone didn't know about this feature, and was trying to find out
>      why their faces looked weird, they'd be more likely to chance upon or
>      recognize `override' as being the likely culprit than `remapping'.
> 
>  (2) Remapping does seem more to imply a one-to-one replacement of one face
>      name with another; if indeed the feature allows merging faces, or even
>      un-named face attributes to be used, then I think override fits better.

IMO, override also seems better than remapping in this case.

We also have e.g. overriding-local-map for buffer local bindings.

Maybe overriding-face-attribute-alist or some such...


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: +face-remapping-20040505-0.patch

[Kim F. Storm]

Miles Bader <miles@gnu.org> writes:

> Here's a new patch, which is re-diffed against the CVS trunk, and also
> includes a bug-fix:

I don't know if you sent a more recent patch than this one, but there's
a bug in the patch:

> +/* Return the face id of the realized face for named face SYMBOL on
> +   frame F suitable for displaying character C.  Value is -1 if the
> +   face couldn't be determined, which might happen if the default face
> +   isn't realized and cannot be realized.  */
> +
> +int
> +lookup_named_face (f, symbol, c)
> +     struct frame *f;
> +     Lisp_Object symbol;
> +     int c;
> +{
> +  return lookup_named_face_1 (f, symbol, c);

           missing arg for signal_p here----+

> +}

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: +face-remapping-20040505-0.patch

[Miles Bader]

storm@cua.dk (Kim F. Storm) writes:
>            missing arg for signal_p here----+

Hmm, you're right.

I'm gonna rewrite things to use `merge_face_vector_with_property'
(or some variant) instead, and also change face inheritance to do the
same, so that that the merging order is more consistent.

-Miles
-- 
Freedom's just another word, for nothing left to lose   --Janis Joplin

Re: +face-remapping-20040505-0.patch

[Kim F. Storm]

Miles Bader <miles@lsi.nec.co.jp> writes:

> storm@cua.dk (Kim F. Storm) writes:
> >            missing arg for signal_p here----+
> 
> Hmm, you're right.
> 
> I'm gonna rewrite things to use `merge_face_vector_with_property'
> (or some variant) instead, and also change face inheritance to do the
> same, so that that the merging order is more consistent.

IMO, it would be very nice to have this added to 21.4 (despite the
feature freeze) -- but in that case, it should be done ASAP.

Likewise with the curry stuff, although it seems a less important
feature from a user's point of view.

WDYT?  Shall either or both of these features be included in 21.4?

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: +face-remapping-20040505-0.patch

[David Kastrup]

storm@cua.dk (Kim F. Storm) writes:

> Miles Bader <miles@lsi.nec.co.jp> writes:
> 
> > storm@cua.dk (Kim F. Storm) writes:
> > >            missing arg for signal_p here----+
> > 
> > Hmm, you're right.
> > 
> > I'm gonna rewrite things to use `merge_face_vector_with_property'
> > (or some variant) instead, and also change face inheritance to do the
> > same, so that that the merging order is more consistent.
> 
> IMO, it would be very nice to have this added to 21.4 (despite the
> feature freeze) -- but in that case, it should be done ASAP.
> 
> Likewise with the curry stuff, although it seems a less important
> feature from a user's point of view.
> 
> WDYT?  Shall either or both of these features be included in 21.4?

AFAICS, the currying stuff has little potential for destabilizing
normal operation and is pretty much limited in extent.  However, there
is also little reason for putting it in if I see it correctly: there
are no user-level features to be gained from it without adding
packages on top of it.  Those first-layer packages that would likely
benefit from the currying would hardly be external to Emacs, but
released as part of Emacs.  We don't have such packages right now, and
the context in which they would get developed would be CVS Emacs,
anyway.

So unless I am misestimating I see no point in putting this in right
now, in particular if we are supposed to be focusing about getting
something to release-ready state.

Similar things apply to the face merging stuff, even though the
chances for non-core Emacs packages picking this functionality up are
somewhat larger.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: +face-remapping-20040505-0.patch

[Richard Stallman]

I think the face remapping is a good feature and will be very
useful to users, so I think it should be installed now,
assuming the manual is properly updated at the same time.