bug#11068: 24.0.94; Face-remapped background does not extend to end of window

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Ivan Andrus]

Run emacs -Q and then evaluate (e.g. in *scratch*) the following

(face-remap-add-relative 'default '(:background "dark green"))

Notice that the dark green background only appears on the lines which
actually contain text.  Most of the window still has the white
background since the buffer only consists of a few lines.  This is
annoying for my use case of different backgrounds to signify different
modes (e.g. read only).

It is also a problem when using :stipple e.g.

(face-remap-add-relative 'default '(:stipple "/Users/gvol/.emacs.d/local/pic/b-rock-grey.jpg"))

Thanks,
Ivan Andrus

In GNU Emacs 24.0.94.6 (i386-apple-darwin10.8.0, NS apple-appkit-1038.36)
of 2012-03-19 on oroszlan.local
Windowing system distributor `Apple', version 10.3.1038
Configured using:
`configure '--with-ns' '-C''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: UTF-8
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: nil
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
<return> M-x C-g s-` <up> <up> <up> <up> <up> <up> 
C-x C-e <C-down> <C-down> <C-down> <C-down> <C-down> 
<C-down> <C-down> <C-down> <C-down> <C-down> <C-down> 
<right> <up> <C-up> <C-up> <C-up> <C-up> <C-up> <C-up> 
<C-up> <C-up> <C-up> <up> C-k C-y <C-down> <C-down> 
<return> <return> <up> C-y C-u <C-left> <C-left> <C-right> 
<C-right> <C-right> <C-backspace> s t i p p l e <C-right> 
<C-right> <C-backspace> <C-backspace> C-d <backspace> 
C-y C-e C-x C-e <C-down> <C-down> <C-down> <C-down> 
<C-down> <C-down> <C-down> <C-down> <C-down> <C-down> 
<down> <right> <up> <up> <down> C-e <return> <backspace> 
<C-up> <C-up> <C-up> <C-up> <C-up> <C-up> <C-up> <C-up> 
<C-up> <C-up> <C-up> <C-down> <C-down> <down> <down> 
C-e C-a <up> <return> <return> <up> I t SPC i s SPC 
a l o <backspace> s o SPC a SPC p r o b l e m SPC w 
i <backspace> h e n SPC u s i n g SPC s t i p p l e 
<C-left> : C-e SPC e . g . SPC w h i c h SPC m <backspace> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> <backspace> <C-down> <C-down> <C-up> <C-up> 
<C-up> <C-up> <C-up> C-SPC <C-down> <C-down> <C-down> 
<C-down> <C-down> C-w C-/ C-c C-c n o <return> <C-up> 
<C-up> <C-up> <C-up> <C-up> <C-up> <C-up> <C-up> <C-up> 
<C-up> <C-up> <C-up> <C-up> <C-up> C-e <C-left> <C-right> 
<C-backspace> <C-backspace> <C-backspace> d a r t h 
a n d r u s @ g m a i l . c o m <C-down> <down> <C-s-268632064> 
<C-down> <C-down> <C-down> <C-down> <C-down> C-w C-/ 
C-c C-c C-c C-c <help-echo> <down-mouse-1> <mouse-1> 
<wheel-up> <down-mouse-1> <mouse-1> <down-mouse-1> 
<mouse-1> <wheel-up> <double-wheel-up> <down-mouse-1> 
<mouse-1> C-h e <C-down> <C-down> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <help-echo> <help-echo> <tool-bar> <kill-buffer> 
<tool-bar> <kill-buffer> <down-mouse-1> <mouse-1> <tool-bar> 
<Kill Message> M-x r e p o <tab> r <tab> <return>

Recent messages:
Making completion list...

Load-path shadows:
None found.

Features:
(newcomment shadow sort gnus-util mail-extr emacsbug message format-spec
rfc822 mml mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail regexp-opt rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils help-mode easymenu view
face-remap vc-hg time-date tooltip ediff-hook vc-hooks lisp-float-type
mwheel ns-win tool-bar dnd fontset image fringe lisp-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer loaddefs
button faces cus-face files text-properties overlay sha1 md5 base64
format env code-pages mule custom widget hashtable-print-readable
backquote make-network-process ns multi-tty emacs)

bug#11068: Acknowledgement (24.0.94; Face-remapped background does not extend to end of window)

[Ivan Andrus]

Sorry, somehow this got sent twice.  #11069 is a duplicate of this (or vice versa).

-Ivan

On Mar 22, 2012, at 10:21 PM, GNU bug Tracking System wrote:

> Thank you for filing a new bug report with debbugs.gnu.org.
> 
> This is an automatically generated reply to let you know your message
> has been received.
> 
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
> 
> Your message has been sent to the package maintainer(s):
> bug-gnu-emacs@gnu.org
> 
> If you wish to submit further information on this problem, please
> send it to 11068@debbugs.gnu.org.
> 
> Please do not send mail to help-debbugs@gnu.org unless you wish
> to report a problem with the Bug-tracking system.
> 
> -- 
> 11068: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11068
> GNU Bug Tracking System
> Contact help-debbugs@gnu.org with problems

bug#11069: 24.0.94; Face-remapped background does not extend to end of window

[Glenn Morris]

This is a literal duplicate of
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11068

and will be removed. Please direct any future correspondence on this
matter to 11068 at debbugs.gnu.org.

bug#11068: Acknowledgement (24.0.94; Face-remapped background does not extend to end of window)

[Glenn Morris]

Ivan Andrus wrote:

> Sorry, somehow this got sent twice.  #11069 is a duplicate of this (or vice versa).

No problem, I removed #11069.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Ivan Andrus <darthandrus@gmail.com>
> Date: Thu, 22 Mar 2012 21:49:10 +0100
> 
> Run emacs -Q and then evaluate (e.g. in *scratch*) the following
> 
> (face-remap-add-relative 'default '(:background "dark green"))
> 
> Notice that the dark green background only appears on the lines which
> actually contain text.  Most of the window still has the white
> background since the buffer only consists of a few lines.  This is
> annoying for my use case of different backgrounds to signify different
> modes (e.g. read only).
> 
> It is also a problem when using :stipple e.g.
> 
> (face-remap-add-relative 'default '(:stipple "/Users/gvol/.emacs.d/local/pic/b-rock-grey.jpg"))

Thanks.

This is how Emacs display engine always worked: the non-text parts of
the window display are drawn in the default face.  (Remapping does not
change the default face, as far as Emacs internals are concerned, it
just substitutes a different face in its stead, leaving the literal
DEFAULT_FACE_ID intact.)

So this bug has rather low priority at this time, as it's not a
regression wrt Emacs 23.  Time permitting, I will at the very least
try to figure out what changes are needed to make this work as
expected.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Ivan Andrus]

On Mar 23, 2012, at 11:36 AM, Eli Zaretskii wrote:
From: Ivan Andrus <darthandrus@gmail.com>
>> Date: Thu, 22 Mar 2012 21:49:10 +0100
>> 
>> Run emacs -Q and then evaluate (e.g. in *scratch*) the following
>> 
>> (face-remap-add-relative 'default '(:background "dark green"))
>> 
>> Notice that the dark green background only appears on the lines which
>> actually contain text.  Most of the window still has the white
>> background since the buffer only consists of a few lines.  This is
>> annoying for my use case of different backgrounds to signify different
>> modes (e.g. read only).
>> 
>> It is also a problem when using :stipple e.g.
>> 
>> (face-remap-add-relative 'default '(:stipple "/Users/gvol/.emacs.d/local/pic/b-rock-grey.jpg"))
> 
> Thanks.
> 
> This is how Emacs display engine always worked: the non-text parts of
> the window display are drawn in the default face.  (Remapping does not
> change the default face, as far as Emacs internals are concerned, it
> just substitutes a different face in its stead, leaving the literal
> DEFAULT_FACE_ID intact.)

> So this bug has rather low priority at this time, as it's not a
> regression wrt Emacs 23.  Time permitting, I will at the very least
> try to figure out what changes are needed to make this work as
> expected.

I completely understand about priorities right now.  In fact, it's taken me a while to finally get around to reporting it.

-Ivan

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Fri, 23 Mar 2012 12:36:31 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 11068@debbugs.gnu.org
> 
> This is how Emacs display engine always worked: the non-text parts of
> the window display are drawn in the default face.  (Remapping does not
> change the default face, as far as Emacs internals are concerned, it
> just substitutes a different face in its stead, leaving the literal
> DEFAULT_FACE_ID intact.)
> 
> So this bug has rather low priority at this time, as it's not a
> regression wrt Emacs 23.  Time permitting, I will at the very least
> try to figure out what changes are needed to make this work as
> expected.

The patch below makes Emacs work as expected in this case.  Whether to
install this now or wait until after Emacs 24.1, is up to Stefan and
Chong.

=== modified file 'src/xdisp.c'
--- a/src/xdisp.c	2012-03-24 12:33:40 +0000
+++ b/src/xdisp.c	2012-03-24 12:34:53 +0000
@@ -18173,7 +18173,12 @@
 	  it->len = 1;
 
 	  if (default_face_p)
-	    it->face_id = DEFAULT_FACE_ID;
+	    {
+	      if (NILP (Vface_remapping_alist))
+		it->face_id = DEFAULT_FACE_ID;
+	      else
+		it->face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
+	    }
 	  else if (it->face_before_selective_p)
 	    it->face_id = it->saved_face_id;
 	  face = FACE_FROM_ID (it->f, it->face_id);
@@ -18209,7 +18214,7 @@
 static void
 extend_face_to_end_of_line (struct it *it)
 {
-  struct face *face;
+  struct face *face, *default_face;
   struct frame *f = it->f;
 
   /* If line is already filled, do nothing.  Non window-system frames
@@ -18223,11 +18228,16 @@
 	 && !it->glyph_row->continued_p))
     return;
 
+  /* The default face, possibly remapped. */
+  default_face = FACE_FROM_ID (f, lookup_basic_face (f, DEFAULT_FACE_ID));
+
   /* Face extension extends the background and box of IT->face_id
      to the end of the line.  If the background equals the background
      of the frame, we don't have to do anything.  */
   if (it->face_before_selective_p)
     face = FACE_FROM_ID (f, it->saved_face_id);
+  else if (it->face_id == DEFAULT_FACE_ID)
+    face = default_face;
   else
     face = FACE_FROM_ID (f, it->face_id);
 
@@ -18260,7 +18270,7 @@
       if (it->glyph_row->used[TEXT_AREA] == 0)
 	{
 	  it->glyph_row->glyphs[TEXT_AREA][0] = space_glyph;
-	  it->glyph_row->glyphs[TEXT_AREA][0].face_id = it->face_id;
+	  it->glyph_row->glyphs[TEXT_AREA][0].face_id = face->id;
 	  it->glyph_row->used[TEXT_AREA] = 1;
 	}
 #ifdef HAVE_WINDOW_SYSTEM
@@ -18296,7 +18306,7 @@
 		 face, to avoid painting the rest of the window with
 		 the region face, if the region ends at ZV.  */
 	      if (it->glyph_row->ends_at_zv_p)
-		it->face_id = DEFAULT_FACE_ID;
+		it->face_id = default_face->id;
 	      else
 		it->face_id = face->id;
 	      append_stretch_glyph (it, make_number (0), stretch_width,
@@ -18329,7 +18339,7 @@
 	 avoid painting the rest of the window with the region face,
 	 if the region ends at ZV.  */
       if (it->glyph_row->ends_at_zv_p)
-	it->face_id = DEFAULT_FACE_ID;
+	it->face_id = default_face->id;
       else
 	it->face_id = face->id;
 
@@ -18993,8 +19003,13 @@
 	  /* A row that displays right-to-left text must always have
 	     its last face extended all the way to the end of line,
 	     even if this row ends in ZV, because we still write to
-	     the screen left to right.  */
-	  if (row->reversed_p)
+	     the screen left to right.  We also need to extend the
+	     last face if the default face is remapped to some
+	     different face, otherwise the functions that clear
+	     portions of the screen will clear with the default face's
+	     background color.  */
+	  if (row->reversed_p
+	      || lookup_basic_face (it->f, DEFAULT_FACE_ID) != DEFAULT_FACE_ID)
 	    extend_face_to_end_of_line (it);
 	  break;
 	}

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > The patch below makes Emacs work as expected in this case.  Whether to
 > install this now or wait until after Emacs 24.1, is up to Stefan and
 > Chong.

Excellent!  This feature has been requested a number of times over the
past years (though nobody bothered to write a bug report before Ivan
helped out) and I'd strongly vote for including it in Emacs 24.1.

There's just one detail missing: A number of people asked for
highlighting _only_ the selected window and if a buffer appears in more
than one window the present mechanism won't help in this regard.  So
couldn't we have the mechanism also respect a window parameter
specifying some face as default window face with higher priority than
the face specified for the buffer?

Thanks for getting us this far, martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sat, 24 Mar 2012 14:42:09 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, Stefan Monnier <monnier@iro.umontreal.ca>, 
>  Chong Yidong <cyd@stupidchicken.com>,
>  11068@debbugs.gnu.org
> 
> There's just one detail missing: A number of people asked for
> highlighting _only_ the selected window and if a buffer appears in more
> than one window the present mechanism won't help in this regard.  So
> couldn't we have the mechanism also respect a window parameter
> specifying some face as default window face with higher priority than
> the face specified for the buffer?

I don't know, I would need to see some details, like how will this
preference be visible to the display engine.

Please note that the changes I've shown only notice when the `default'
face is being remapped, they change nothing in how that default face
is computed.  So I suspect we won't be able to use the same mechanism
to do what you want, because what you want changes the way the default
face is computed (IIUC).

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>    /* Face extension extends the background and box of IT->face_id
>       to the end of the line.  If the background equals the background
>       of the frame, we don't have to do anything.  */
>    if (it->face_before_selective_p)
>      face = FACE_FROM_ID (f, it->saved_face_id);
> +  else if (it->face_id == DEFAULT_FACE_ID)
> +    face = default_face;
>    else
>      face = FACE_FROM_ID (f, it->face_id);

This looks a bit strange.  Why is DEFAULT_FACE_ID handled specially
here?  Shouldn't we be checking for face remapping no matter what the
face_id is?

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: darthandrus@gmail.com,  Stefan Monnier <monnier@iro.umontreal.ca>,  11068@debbugs.gnu.org
> Date: Sat, 24 Mar 2012 22:17:41 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >    /* Face extension extends the background and box of IT->face_id
> >       to the end of the line.  If the background equals the background
> >       of the frame, we don't have to do anything.  */
> >    if (it->face_before_selective_p)
> >      face = FACE_FROM_ID (f, it->saved_face_id);
> > +  else if (it->face_id == DEFAULT_FACE_ID)
> > +    face = default_face;
> >    else
> >      face = FACE_FROM_ID (f, it->face_id);
> 
> This looks a bit strange.  Why is DEFAULT_FACE_ID handled specially
> here?

In my testing, I didn't see the need to do it even for the default
face, because it->face_id is already set to the ID of the remapped
face.  So it's just me being paranoiac.

Because of this condition, immediately below the fragment you cited:

  if (FRAME_WINDOW_P (f)
      && it->glyph_row->displays_text_p
      && face->box == FACE_NO_BOX
      && face->background == FRAME_BACKGROUND_PIXEL (f)
      && !face->stipple
      && !it->glyph_row->reversed_p)
    return;

Any non-default face would already penetrate this condition

> Shouldn't we be checking for face remapping no matter what the
> face_id is?

The remapping takes place before we hit this function, which is at the
end of displaying a line.  So the face is already remapped.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Stefan Monnier]

> There's just one detail missing: A number of people asked for
> highlighting _only_ the selected window and if a buffer appears in more
> than one window the present mechanism won't help in this regard.  So
> couldn't we have the mechanism also respect a window parameter
> specifying some face as default window face with higher priority than
> the face specified for the buffer?

If you want such a feature, I'd suggest you start by looking at every
place that observes Vface_remapping_alist and change it to also consider
Fwindow_parameter (selected_window, Qface_remapping_alist).
I can't guarantee this will be sufficient, but it should be a good
starting point.


        Stefan

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > I don't know, I would need to see some details, like how will this
 > preference be visible to the display engine.

Via some function that extracts a valid face from a window parameter.
Actually, the only interesting thing IMO is the background attribute.  I
don't think that changing any other attributes would make sense.  But
the same holds for the current patch as well: You simply fill up the
rest of the window with the background of the remapped default face.  Or
am I missing something?

 > Please note that the changes I've shown only notice when the `default'
 > face is being remapped, they change nothing in how that default face
 > is computed.  So I suspect we won't be able to use the same mechanism
 > to do what you want, because what you want changes the way the default
 > face is computed (IIUC).

I don't know how face remapping works so I can't tell.  I thought that
remapping does recompute the default face.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > If you want such a feature,

Personally I'd have no use for it.  But the request for such a feature
reappears from time to time on help-gnu-emacs.

 > I'd suggest you start by looking at every
 > place that observes Vface_remapping_alist and change it to also consider
 > Fwindow_parameter (selected_window, Qface_remapping_alist).
 > I can't guarantee this will be sufficient, but it should be a good
 > starting point.

These are all in xfaces.c and I doubt they have any idea about the
window currently drawn.

Anyway, I'll rather redraw my request - the feature as Eli currently
implements it is all I need (for more than 10 years, meanwhile).

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sat, 24 Mar 2012 20:48:13 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
>  > I don't know, I would need to see some details, like how will this
>  > preference be visible to the display engine.
> 
> Via some function that extracts a valid face from a window parameter.
> Actually, the only interesting thing IMO is the background attribute.  I
> don't think that changing any other attributes would make sense.

You might be surprised what the users night want ...

> But the same holds for the current patch as well: You simply fill up
> the rest of the window with the background of the remapped default
> face.

No, not only background.  The entire face with all its attributes is
used.  E.g., stipple (which doesn't work on MS-Windows), font
(i.e. the size of each line in pixels), etc.

And you cannot "fill the rest of the window" with some color, at least
not on the device-independent level of the display engine where I made
the changes.  The only way the display engine can dictate colors (and
other face attributes) is by creating glyphs which have these
attributes and adding those glyphs to the current glyph matrix.

What the changes I suggested do is add stretch glyphs of a suitably
computed width to each glyph row that has no text in it.  The changes
are small because the code that does that was already written for the
bidirectional display, which needs to display R2L lines flushed all
the way to the right margin of the window; it does that by prepending
such a stretch glyph to the R2L glyph rows, ahead (i.e to the left) of
the text.  All I needed was to activate that code for the case of
remapped default face, in addition to R2L lines.

>  > Please note that the changes I've shown only notice when the `default'
>  > face is being remapped, they change nothing in how that default face
>  > is computed.  So I suspect we won't be able to use the same mechanism
>  > to do what you want, because what you want changes the way the default
>  > face is computed (IIUC).
> 
> I don't know how face remapping works so I can't tell.  I thought that
> remapping does recompute the default face.

It does, indeed, but I didn't touch that part.  I only used the
already computed remapped face in a couple of places.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>> This looks a bit strange.  Why is DEFAULT_FACE_ID handled specially
>> here?
>
> In my testing, I didn't see the need to do it even for the default
> face, because it->face_id is already set to the ID of the remapped
> face.  So it's just me being paranoiac.

I'd just leave that bit out.

Also, in this hunk

@@ -18173,7 +18173,12 @@
 	  it->len = 1;
 
 	  if (default_face_p)
-	    it->face_id = DEFAULT_FACE_ID;
+	    {
+	      if (NILP (Vface_remapping_alist))
+		it->face_id = DEFAULT_FACE_ID;
+	      else
+		it->face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
+	    }
 	  else if (it->face_before_selective_p)
 	    it->face_id = it->saved_face_id;
 	  face = FACE_FROM_ID (it->f, it->face_id);

You should call lookup_basic_face without the surrounding if statement,
because lookup_basic_face returns its second arg immediately if
face-remapping-alist is nil, making the extra check redundant.

And here

+	  if (row->reversed_p
+	      || lookup_basic_face (it->f, DEFAULT_FACE_ID) != DEFAULT_FACE_ID)

shouldn't you just compare default_face->id to DEFAULT_FACE_ID instead
of making another lookup_basic_face call?

With these changes, the patch seems pretty safe; even if something
screws up, it will only affect how remapped faces extend to the end of
the buffer, which was broken before anyway.  So feel free to commit.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: darthandrus@gmail.com,  monnier@iro.umontreal.ca,  11068@debbugs.gnu.org
> Date: Sun, 25 Mar 2012 11:01:17 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This looks a bit strange.  Why is DEFAULT_FACE_ID handled specially
> >> here?
> >
> > In my testing, I didn't see the need to do it even for the default
> > face, because it->face_id is already set to the ID of the remapped
> > face.  So it's just me being paranoiac.
> 
> I'd just leave that bit out.

OK.

> Also, in this hunk
> 
> @@ -18173,7 +18173,12 @@
>  	  it->len = 1;
>  
>  	  if (default_face_p)
> -	    it->face_id = DEFAULT_FACE_ID;
> +	    {
> +	      if (NILP (Vface_remapping_alist))
> +		it->face_id = DEFAULT_FACE_ID;
> +	      else
> +		it->face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
> +	    }
>  	  else if (it->face_before_selective_p)
>  	    it->face_id = it->saved_face_id;
>  	  face = FACE_FROM_ID (it->f, it->face_id);
> 
> You should call lookup_basic_face without the surrounding if statement,
> because lookup_basic_face returns its second arg immediately if
> face-remapping-alist is nil, making the extra check redundant.

Right.

> And here
> 
> +	  if (row->reversed_p
> +	      || lookup_basic_face (it->f, DEFAULT_FACE_ID) != DEFAULT_FACE_ID)
> 
> shouldn't you just compare default_face->id to DEFAULT_FACE_ID instead
> of making another lookup_basic_face call?

Here I disagree: I think we should confine the internal details of
face remapping to lookup_basic_face, instead of spilling our knowledge
about that all over.  As you just said above, if face-remapping-alist
is nil, that function returns immediately, so there's nothing lost
here for the "usual" use-case.

> With these changes, the patch seems pretty safe; even if something
> screws up, it will only affect how remapped faces extend to the end of
> the buffer, which was broken before anyway.  So feel free to commit.

OK, but to be absolutely fair, I must point out that, while the patch
is very simple, it has one non-trivial consequence: when the default
face _is_ remapped, the empty space to the right of any L2R line and
the empty lines beyond EOB are filled with stretch glyphs of a
suitably computed width.  Before the patch, there were no glyphs in
those places.  So, from the POV of the content of the glyph matrices,
the change introduced by these patches is quite prominent.  In
particular, this could potentially affect any code that examines the
glyphs of a glyph matrix; previously, such stretch glyphs were only
present at the left side of R2L glyph rows.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> Here I disagree: I think we should confine the internal details of
> face remapping to lookup_basic_face, instead of spilling our knowledge
> about that all over.  As you just said above, if face-remapping-alist
> is nil, that function returns immediately, so there's nothing lost
> here for the "usual" use-case.

I don't know how making use of the return value from a previous call to
lookup_basic_face (when default_face was assigned) is "spilling our
knowledge", but whatever; it's not a important point.

> OK, but to be absolutely fair, I must point out that, while the patch
> is very simple, it has one non-trivial consequence: when the default
> face _is_ remapped, the empty space to the right of any L2R line and
> the empty lines beyond EOB are filled with stretch glyphs of a
> suitably computed width.  Before the patch, there were no glyphs in
> those places.  So, from the POV of the content of the glyph matrices,
> the change introduced by these patches is quite prominent.  In
> particular, this could potentially affect any code that examines the
> glyphs of a glyph matrix; previously, such stretch glyphs were only
> present at the left side of R2L glyph rows.

Hmm, you're right, that is a non-trivial change.  The implications of
that are not obvious to me.  Maybe we should delay this for post-24.1
then.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 >> But the same holds for the current patch as well: You simply fill up
 >> the rest of the window with the background of the remapped default
 >> face.
 >
 > No, not only background.  The entire face with all its attributes is
 > used.  E.g., stipple (which doesn't work on MS-Windows), font
 > (i.e. the size of each line in pixels), etc.
 >
 > And you cannot "fill the rest of the window" with some color, at least
 > not on the device-independent level of the display engine where I made
 > the changes.  The only way the display engine can dictate colors (and
 > other face attributes) is by creating glyphs which have these
 > attributes and adding those glyphs to the current glyph matrix.
 >
 > What the changes I suggested do is add stretch glyphs of a suitably
 > computed width to each glyph row that has no text in it.  The changes
 > are small because the code that does that was already written for the
 > bidirectional display, which needs to display R2L lines flushed all
 > the way to the right margin of the window; it does that by prepending
 > such a stretch glyph to the R2L glyph rows, ahead (i.e to the left) of
 > the text.  All I needed was to activate that code for the case of
 > remapped default face, in addition to R2L lines.

Consider the following example: In my .emacs I do for ages

(custom-set-faces
  '(font-lock-comment-face ((((class color) (background light)) (:background "Beige" :foreground "Black")))))

Evaluate this form with emacs -Q in *scratch* and you will see that the
background color extends from each buffer line end to the right window
edge.  This is butt ugly (and incorrect IMHO) and I had to write my own
`font-lock-fontify-syntactically-region' function in order to remove the
face property from the return character but I disgress ...

Anyway, my conclusion from this example is that even in normal L2R text
Emacs somehow constructs a stretch glyph (or whatever else) to simulate
the effect of text properties over some virtual text.  I assume that
your solution does something equivalent for every virtual buffer line
that is missing down to the bottom of the window.  And it takes the face
property not from the return character but from the remapped default
face.  Apart from that why do you need a solution from R2L lines here?

As an aside I earlier tried to achieve the effect of your patch by
adding an overlay with an after-string containing an appropriate number
of return characters to the last character of each buffer.  It didn't
work and I still consider that a bug.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > previously, such stretch glyphs were only
 > present at the left side of R2L glyph rows.

Are you taking into account the example from my other mail?  What does
the display engine use at the right side of L2R glyph rows?

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > Hmm, you're right, that is a non-trivial change.  The implications of
 > that are not obvious to me.  Maybe we should delay this for post-24.1
 > then.

If you delay it, please post the preliminary patch somewhere so I can
use and test it.

Thanks, martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sun, 25 Mar 2012 14:54:12 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
>  > What the changes I suggested do is add stretch glyphs of a suitably
>  > computed width to each glyph row that has no text in it.  The changes
>  > are small because the code that does that was already written for the
>  > bidirectional display, which needs to display R2L lines flushed all
>  > the way to the right margin of the window; it does that by prepending
>  > such a stretch glyph to the R2L glyph rows, ahead (i.e to the left) of
>  > the text.  All I needed was to activate that code for the case of
>  > remapped default face, in addition to R2L lines.
> 
> Consider the following example: In my .emacs I do for ages
> 
> (custom-set-faces
>   '(font-lock-comment-face ((((class color) (background light)) (:background "Beige" :foreground "Black")))))
> 
> Evaluate this form with emacs -Q in *scratch* and you will see that the
> background color extends from each buffer line end to the right window
> edge.

Yes, that's true (and well known).

> Anyway, my conclusion from this example is that even in normal L2R text
> Emacs somehow constructs a stretch glyph (or whatever else) to simulate
> the effect of text properties over some virtual text.

Correct.  But only for lines that end before EOB.  The line that ends
_at_ EOB and all the subsequent lines don't have this stretch glyph.
The changes I did introduce such stretch glyphs for _all_ lines in the
window than don't already have them.

> I assume that your solution does something equivalent for every
> virtual buffer line that is missing down to the bottom of the
> window.

Yes.

> And it takes the face property not from the return character but
> from the remapped default face.

No, the return character has no face, and in fact it has no glyphs.
The original code would reset the face to the (un-remapped) default
face when it hits the line at EOB, and would not draw past EOB at all,
instead relying on the display-specific code that clears the rest of
the window with the default color.  Since now there are stretch glyphs
there, we do draw in those areas, and we draw in the precise face that
remapping specifies.

> Apart from that why do you need a solution from R2L lines here?

Nothing, I just reused the machinery that was invented for R2L lines.

> As an aside I earlier tried to achieve the effect of your patch by
> adding an overlay with an after-string containing an appropriate number
> of return characters to the last character of each buffer.  It didn't
> work and I still consider that a bug.

If you show me the code, I will see if it's a bug or something else.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sun, 25 Mar 2012 14:54:53 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: Chong Yidong <cyd@gnu.org>, darthandrus@gmail.com, 
>  11068@debbugs.gnu.org
> 
>  > previously, such stretch glyphs were only
>  > present at the left side of R2L glyph rows.
> 
> Are you taking into account the example from my other mail?  What does
> the display engine use at the right side of L2R glyph rows?

Normally, nothing.  If the face of the last character of a line has
the same background as the default face and no stipple, there are no
stretch glyphs at the right side.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sun, 25 Mar 2012 14:55:15 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: Eli Zaretskii <eliz@gnu.org>, darthandrus@gmail.com, 
>  11068@debbugs.gnu.org
> 
>  > Hmm, you're right, that is a non-trivial change.  The implications of
>  > that are not obvious to me.  Maybe we should delay this for post-24.1
>  > then.
> 
> If you delay it, please post the preliminary patch somewhere so I can
> use and test it.

I will, as soon as I hear the final word.  Btw, I already posted a
version that "works for me", right here, albeit before implementing
Chong's suggestions.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: darthandrus@gmail.com,  monnier@iro.umontreal.ca,  11068@debbugs.gnu.org
> Date: Sun, 25 Mar 2012 14:20:57 +0800
> 
> Hmm, you're right, that is a non-trivial change.  The implications of
> that are not obvious to me.  Maybe we should delay this for post-24.1
> then.

Is that final, or do you want to hear from Stefan, or think about it?

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > Correct.  But only for lines that end before EOB.  The line that ends
 > _at_ EOB and all the subsequent lines don't have this stretch glyph.
 > The changes I did introduce such stretch glyphs for _all_ lines in the
 > window than don't already have them.

So you do use an established mechanism but for the fact that these lines
exist only virtually.  Or am I missing something?

 >> And it takes the face property not from the return character but
 >> from the remapped default face.
 >
 > No, the return character has no face,

But when drawing the stretch glyph at a non-EOB line end the display
engine does use the face of the return character.  In my
`font-lock-fontify-syntactically-region' function I strip all face
properties from the return character and the rest of the line has the
default background.

 > and in fact it has no glyphs.
 > The original code would reset the face to the (un-remapped) default
 > face when it hits the line at EOB, and would not draw past EOB at all,
 > instead relying on the display-specific code that clears the rest of
 > the window with the default color.

Couldn't we "clear" using the remapped default color as well?  Does
"clearing" care about character heights, for example?

 >> As an aside I earlier tried to achieve the effect of your patch by
 >> adding an overlay with an after-string containing an appropriate number
 >> of return characters to the last character of each buffer.  It didn't
 >> work and I still consider that a bug.
 >
 > If you show me the code, I will see if it's a bug or something else.

If you do, for example,

(let ((overlay (make-overlay (point-max) (point-max))))
   (overlay-put overlay 'after-string "\n\n\n\n"))

you can't move to the position before the overlay which makes the whole
thing worthless.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 >> Are you taking into account the example from my other mail?  What does
 >> the display engine use at the right side of L2R glyph rows?
 >
 > Normally, nothing.  If the face of the last character of a line has
 > the same background as the default face and no stipple, there are no
 > stretch glyphs at the right side.

I obviously meant the case where it has a different background from the
default face.  But I can't follow you anyway because in my experience
it's the return character that counts and not the last character on the
line.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > I will, as soon as I hear the final word.  Btw, I already posted a
 > version that "works for me", right here, albeit before implementing
 > Chong's suggestions.

I'm using that already and it works for me here too.  But I meant the
version incorporating Chong's suggestions.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Stefan Monnier]

> (let ((overlay (make-overlay (point-max) (point-max))))
>   (overlay-put overlay 'after-string "\n\n\n\n"))

Nitpick, you want (make-overlay (point-max) (point-max) nil t t) if you
want to create an overlay "at EOB" rather than "right after the char
which is currently at (point-max)".  Cursor positioning should be
sensitive to these kinds of stickiness details, although IIRC it
currently isn't.


        Stefan

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > Nitpick, you want (make-overlay (point-max) (point-max) nil t t) if you
 > want to create an overlay "at EOB" rather than "right after the char
 > which is currently at (point-max)".

In my use case I always deleted the overlay before redisplay and
recreated it anew to account for possible changes in window height.

 > Cursor positioning should be
 > sensitive to these kinds of stickiness details, although IIRC it
 > currently isn't.

Unfortunately it isn't.  I then played with all sorts of 'display
'before-string properties, arguments, and whatever I could apply
(some ten years ago).

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sun, 25 Mar 2012 21:19:43 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
>  > Correct.  But only for lines that end before EOB.  The line that ends
>  > _at_ EOB and all the subsequent lines don't have this stretch glyph.
>  > The changes I did introduce such stretch glyphs for _all_ lines in the
>  > window than don't already have them.
> 
> So you do use an established mechanism but for the fact that these lines
> exist only virtually.  Or am I missing something?

I don't understand what you mean by "exist only virtually".  We are
not talking about lines, we are talking about glyph matrices.  A glyph
matrix has at least one glyph for each screen line, no matter if there
is or isn't text on these lines; this includes empty lines beyond BOB.

The established mechanism I used was invented for extending the
(non-default) face of the last character of a line all the way to the
right margin of the window.  I made it do double duty for R2L lines,
because these need a stretch glyph at their left, so that the
device-specific drawing code could still draw left to right.  Then I
made it do triple duty when the default face is remapped, by adding
stretch glyphs even on L2R lines beyond EOB.  All it took was to
slightly tweak the conditions under which the function
extend_face_to_end_of_line is called, and what it does as part of its
job.

>  >> And it takes the face property not from the return character but
>  >> from the remapped default face.
>  >
>  > No, the return character has no face,
> 
> But when drawing the stretch glyph at a non-EOB line end the display
> engine does use the face of the return character.

Not the face of the return character, but the face of the last glyph
produced for the line.

> In my `font-lock-fontify-syntactically-region' function I strip all
> face properties from the return character and the rest of the line
> has the default background.

What that does is force redisplay to change the face at the line
boundary.  Normally, the last face used on the line is also used for
the first glyph of the next line.  The face of the newline itself has
no direct relation to this.

> Couldn't we "clear" using the remapped default color as well?

That's possible, but it's a much worse design, IMO, because it would
mean this needs to be implemented N times, one each for every terminal
type we support (TTY, X, w32, ns).  Worse, there's this:

> Does "clearing" care about character heights, for example?

No.  It also cannot support faces with a stipple.  So this design
isn't really up to the job at all.

> If you do, for example,
> 
> (let ((overlay (make-overlay (point-max) (point-max))))
>    (overlay-put overlay 'after-string "\n\n\n\n"))
> 
> you can't move to the position before the overlay which makes the whole
> thing worthless.

Try putting a `cursor' text property on the first newline of the
string, and if that doesn't work, I might agree it's a bug.
Otherwise, Emacs always puts the cursor after the overlay string.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sun, 25 Mar 2012 21:19:54 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: cyd@gnu.org, darthandrus@gmail.com, 11068@debbugs.gnu.org
> 
> I can't follow you anyway because in my experience it's the return
> character that counts and not the last character on the line.

The return character isn't drawn, so it cannot count.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Sun, 25 Mar 2012 23:49:35 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: darthandrus@gmail.com, 11068@debbugs.gnu.org
> 
> A glyph matrix has at least one glyph for each screen line, no
> matter if there is or isn't text on these lines; this includes empty
> lines beyond BOB.

Sorry, this part was wrong.  Empty L2R lines beyond EOB don't have any
glyphs at all.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> Is that final, or do you want to hear from Stefan, or think about it?

I've thought about it, and after staring at the code I'm pretty sure it
does the right thing.  If the extra glyphs break anything, we have a
regression bug on our hands for R2L lines anyway.  So I'll vouch for the
change.  Please go ahead and commit the fixed patch ASAP.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 >> So you do use an established mechanism but for the fact that these lines
 >> exist only virtually.  Or am I missing something?
 >
 > I don't understand what you mean by "exist only virtually".  We are
 > not talking about lines, we are talking about glyph matrices.  A glyph
 > matrix has at least one glyph for each screen line, no matter if there
 > is or isn't text on these lines; this includes empty lines beyond BOB.
								
With "virtual" I tried to describe the presence of a (space) character
on the screen which has no correspondence to a "real" character in a
buffer.

 > The established mechanism I used was invented for extending the
 > (non-default) face of the last character of a line all the way to the
 > right margin of the window.  I made it do double duty for R2L lines,
 > because these need a stretch glyph at their left, so that the
 > device-specific drawing code could still draw left to right.  Then I
 > made it do triple duty when the default face is remapped, by adding
 > stretch glyphs even on L2R lines beyond EOB.  All it took was to
 > slightly tweak the conditions under which the function
 > extend_face_to_end_of_line is called, and what it does as part of its
 > job.

That's what I thought.  Thanks for the precise explanation.

 >> But when drawing the stretch glyph at a non-EOB line end the display
 >> engine does use the face of the return character.
 >
 > Not the face of the return character, but the face of the last glyph
 > produced for the line.

I still can't follow you.  For years I use:


(defun font-lock-fontify-syntactically-region (start end &optional loudly ppss)
   "Put proper face on each string and comment between START and END.
START should be at the beginning of a line."
   (let (state face from to lep)
     (goto-char start)
     (setq state (or ppss (syntax-ppss start)))
     (while
         (progn
           (when (or (nth 3 state) (nth 4 state))
             (setq face (funcall font-lock-syntactic-face-function state))
             (setq from (max (nth 8 state) start))
             (setq state
                   (parse-partial-sexp (point) end nil nil state 'syntax-table))
             (setq to (point))
             (save-excursion
               (goto-char from)
               (while (< from to)
                 (setq lep (line-end-position))
                 (if (< lep to)
                     (progn
                       (put-text-property from lep 'face face)
                       (remove-text-properties
		       lep (1+ lep) '(face nil)) ; rear-nonsticky t))
                       (goto-char (setq from (1+ lep))))
                   (put-text-property from to 'face face)
                   (setq from to)))))
           (< (point) end))
       (setq state
         (parse-partial-sexp (point) end nil nil state 'syntax-table)))))

(custom-set-faces
  '(font-lock-comment-face ((((class color) (background light)) (:background "Beige" :foreground "Black"))))
  '(font-lock-string-face ((t (:foreground "green4"))))
  '(font-lock-doc-face ((t (:inherit font-lock-string-face :background "grey96")))))


Evaluate these in *scratch* with emacs -Q and redraw.  You will see that
the background of comments and doc-strings does _not_ extend to the
right window margin although the last character on each comment or
doc-string line _does_ have that background.  Now what constitutes "the
last glyph produced" for such a line?  That of the last visible
character on the buffer line?

 >> In my `font-lock-fontify-syntactically-region' function I strip all
 >> face properties from the return character and the rest of the line
 >> has the default background.
 >
 > What that does is force redisplay to change the face at the line
 > boundary.  Normally, the last face used on the line is also used for
 > the first glyph of the next line.  The face of the newline itself has
 > no direct relation to this.

But as you see from my code above, all I do is change the face of the
newline character.  The faces before and after it remain unchanged.  You
can easily verify for yourself in a not font-locked buffer: Change the
background of one newline character only and the value you specifiy
there will be used for drawing the space between the last visible
character on the line and the right window margin and for nothing else.

 >> Couldn't we "clear" using the remapped default color as well?
 >
 > That's possible, but it's a much worse design, IMO, because it would
 > mean this needs to be implemented N times, one each for every terminal
 > type we support (TTY, X, w32, ns).  Worse, there's this:
 >
 >> Does "clearing" care about character heights, for example?
 >
 > No.  It also cannot support faces with a stipple.  So this design
 > isn't really up to the job at all.

Aha.  This clears things up for me.

 >> If you do, for example,
 >>
 >> (let ((overlay (make-overlay (point-max) (point-max))))
 >>    (overlay-put overlay 'after-string "\n\n\n\n"))
 >>
 >> you can't move to the position before the overlay which makes the whole
 >> thing worthless.
 >
 > Try putting a `cursor' text property on the first newline of the
 > string, and if that doesn't work, I might agree it's a bug.
 > Otherwise, Emacs always puts the cursor after the overlay string.

Thanks, that works.  Now if only this had been described in the overlays
manual section ...

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Mon, 26 Mar 2012 09:05:43 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
>  >> So you do use an established mechanism but for the fact that these lines
>  >> exist only virtually.  Or am I missing something?
>  >
>  > I don't understand what you mean by "exist only virtually".  We are
>  > not talking about lines, we are talking about glyph matrices.  A glyph
>  > matrix has at least one glyph for each screen line, no matter if there
>  > is or isn't text on these lines; this includes empty lines beyond BOB.
> 								
> With "virtual" I tried to describe the presence of a (space) character
> on the screen which has no correspondence to a "real" character in a
> buffer.

Well, strictly speaking, it's a space only on a TTY.  On GUI
terminals, it's a special glyph that just looks like a space.

> Evaluate these in *scratch* with emacs -Q and redraw.  You will see that
> the background of comments and doc-strings does _not_ extend to the
> right window margin although the last character on each comment or
> doc-string line _does_ have that background.  Now what constitutes "the
> last glyph produced" for such a line?  That of the last visible
> character on the buffer line?

The point I'm stubbornly trying to make is that what matters for the
face extension is the last face loaded by the display iterator, _not_
the face of the newline or any other character.  The display iterator
changes faces at so-called "stop positions", where buffer contents of
text properties or overlays specify a different face.  Once a face is
resolved and loaded, it stays recorded in the iterator and affects
every glyph we deliver until another "stop position" is encountered.

Your code simply forces the display iterator to switch faces after the
last character of a line.  That's it.  The newline doesn't enter this
game at any point, because it is never drawn.  IOW, what's important
is the _position_ where the new face gets in effect, not what
character it is on.

>  >> (let ((overlay (make-overlay (point-max) (point-max))))
>  >>    (overlay-put overlay 'after-string "\n\n\n\n"))
>  >>
>  >> you can't move to the position before the overlay which makes the whole
>  >> thing worthless.
>  >
>  > Try putting a `cursor' text property on the first newline of the
>  > string, and if that doesn't work, I might agree it's a bug.
>  > Otherwise, Emacs always puts the cursor after the overlay string.
> 
> Thanks, that works.  Now if only this had been described in the overlays
> manual section ...

This _is_ described, but not in the section about overlays, because
`cursor' is a text property you should put on `display' property
strings or on overlay strings.  So this is described in "Special
Properties", and the description does mention overlay strings.  Maybe
an index entry should be added that starts with "overlay"; perhaps you
could suggest such an entry.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: darthandrus@gmail.com,  monnier@iro.umontreal.ca,  11068@debbugs.gnu.org
> Date: Mon, 26 Mar 2012 12:16:23 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is that final, or do you want to hear from Stefan, or think about it?
> 
> I've thought about it, and after staring at the code I'm pretty sure it
> does the right thing.  If the extra glyphs break anything, we have a
> regression bug on our hands for R2L lines anyway.  So I'll vouch for the
> change.  Please go ahead and commit the fixed patch ASAP.

OK.  "ASAP" will happen in a couple of days in this case: my main
development machine just died, after 7 years of continuous service,
and the heir is expected the day after tomorrow.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > The point I'm stubbornly trying to make is that what matters for the
 > face extension is the last face loaded by the display iterator, _not_
 > the face of the newline or any other character.  The display iterator
 > changes faces at so-called "stop positions", where buffer contents of
 > text properties or overlays specify a different face.  Once a face is
 > resolved and loaded, it stays recorded in the iterator and affects
 > every glyph we deliver until another "stop position" is encountered.

Earlier you said

   "No, the return character has no face, and in fact it has no glyphs."

and I stubbornly believe that it has a face I can specify, that face has
an effect on what I see on the screen and apparently even produces a
stretch glyph I can see.  But obviously the display engine has its own
mechanism for translating face specifications and our different POVs get
lost in that translation.

 > Your code simply forces the display iterator to switch faces after the
 > last character of a line.  That's it.  The newline doesn't enter this
 > game at any point, because it is never drawn.  IOW, what's important
 > is the _position_ where the new face gets in effect, not what
 > character it is on.

Fully agreed.  But since I switch faces back immediately _after_ the
newline character it does not affect visible characters but only what
appears between them.

 > This _is_ described, but not in the section about overlays, because
 > `cursor' is a text property you should put on `display' property
 > strings or on overlay strings.  So this is described in "Special
 > Properties", and the description does mention overlay strings.

I read that already but apparently missed it on previous readings.

 > Maybe
 > an index entry should be added that starts with "overlay"; perhaps you
 > could suggest such an entry.

I'm afraid that wouldn't help much.  Maybe a separate special properties
table based on an alphabetic listing of property names, a reference to
where the property is described in detail, and whether it affects text,
overlays or both would help.

But some part of the text is IMO confusing: In

      "Normally, the cursor is displayed at the end of any overlay and
      text property strings present at the current buffer position."

I understand that an "overlay string" is a before- or after-string or a
display property string.  But what is a "text property string"?  A
display property of the text?  Also

      "it specifies the number of buffer's
      character positions associated with the overlay string"

clashes with my understanding that this number is already specified by
the start and end position of the overlay.  And

      "this way,
      Emacs will display the cursor on the character with that property
      regardless of whether the current buffer position is actually
      covered by the overlay."

doesn't make it clearer for me because what is "the character with that
property" and what is "the current buffer position" here?  I tried to
play around with it but don't see what this means for an overlay with a
display, before- or after-string property.  So the fact that overlays
with such a property are affected by the `cursor' property remains
obscure to me after reading this text.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Tue, 27 Mar 2012 11:23:28 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
> Earlier you said
> 
>    "No, the return character has no face, and in fact it has no glyphs."
> 
> and I stubbornly believe that it has a face I can specify, that face has
> an effect on what I see on the screen and apparently even produces a
> stretch glyph I can see.

That's my sloppy wording, sorry.  I meant to say that a newline has no
face _on_display_, because it simply isn't displayed.  It certainly
has a face _in_the_buffer_, which I believe is what you mean.

>  > Your code simply forces the display iterator to switch faces after the
>  > last character of a line.  That's it.  The newline doesn't enter this
>  > game at any point, because it is never drawn.  IOW, what's important
>  > is the _position_ where the new face gets in effect, not what
>  > character it is on.
> 
> Fully agreed.  But since I switch faces back immediately _after_ the
> newline character it does not affect visible characters but only what
> appears between them.

Right.

> But some part of the text is IMO confusing: In
> 
>       "Normally, the cursor is displayed at the end of any overlay and
>       text property strings present at the current buffer position."
> 
> I understand that an "overlay string" is a before- or after-string or a
> display property string.  But what is a "text property string"?  A
> display property of the text?

Yes, a `display' text property on buffer text.

>       "it specifies the number of buffer's
>       character positions associated with the overlay string"
> 
> clashes with my understanding that this number is already specified by
> the start and end position of the overlay.

That's because you think that the code which positions the cursor has
the overlay in hand to get this information.  But that assumption is
false: the code in question, which needs this property on overlay
strings (set_cursor_from_row) doesn't know what overlay produced the
glyphs in the glyph rows it considers as candidates for cursor
positioning.  All it has is the glyphs, whereby each glyph includes a
reference to the object from which it came.  For glyphs that came from
an overlay string, that object is the string, but the information
about the overlay where the string came from is lost.  By putting the
integer property on the string itself, you allow the
cursor-positioning code to find out how many buffer positions are
covered by the overlay string, without knowing what overlay is that.

>       "this way,
>       Emacs will display the cursor on the character with that property
>       regardless of whether the current buffer position is actually
>       covered by the overlay."
> 
> doesn't make it clearer for me because what is "the character with that
> property" and what is "the current buffer position" here?

"the character" is the one from the overlay string on which you put
the `cursor' property; "current buffer position" is point (which
normally determines where to display the cursor).

> I tried to play around with it but don't see what this means for an
> overlay with a display, before- or after-string property.  So the
> fact that overlays with such a property are affected by the `cursor'
> property remains obscure to me after reading this text.

It's a hard issue to explain, and I obviously failed.  You can see
this feature in action in cua-rectangle; after you play with it,
perhaps you could suggest how to improve the documentation.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > ... By putting the
 > integer property on the string itself, you allow the
 > cursor-positioning code to find out how many buffer positions are
 > covered by the overlay string, without knowing what overlay is that.
 >
 >>       "this way,
 >>       Emacs will display the cursor on the character with that property
 >>       regardless of whether the current buffer position is actually
 >>       covered by the overlay."
 >>
 >> doesn't make it clearer for me because what is "the character with that
 >> property" and what is "the current buffer position" here?
 >
 > "the character" is the one from the overlay string on which you put
 > the `cursor' property; "current buffer position" is point (which
 > normally determines where to display the cursor).

I still don't get it.  Suppose I have the following code in a buffer, no
spaces before the first visible character, one newline after the last:


(defvar my-overlay nil)
(defvar my-string nil)

(progn
   (when (overlayp my-overlay)
     (delete-overlay my-overlay))
   (setq my-overlay (make-overlay 0 0))
   (setq my-string
	(propertize "      " 'face 'lazy-highlight 'cursor 1))
   (overlay-put my-overlay 'display my-string)
   (move-overlay my-overlay (- (point-max) 1) (point-max)))


Evaluating this and moving `point' between buffer positions 336 and 337
oscillates the cursor around the overlay.  If I instead use the line

	(propertize "      " 'face 'lazy-highlight 'cursor 2))

in the above and reevaluate, moving `point' doesn't oscillate the cursor
any more.  But if I additionally replace the last line as

   (move-overlay my-overlay (- (point-max) 2) (point-max)))

the cursor oscillates again (but now between positions 335 and 337).

I find the behavior good but am too silly to understand what's going on.
What do I really specify when I write a form like

	(propertize "      " 'face 'lazy-highlight 'cursor 1))

Apparently the "1" doesn't refer to a position within the "      " but
to some position within the buffer from (- (point-max) 1) (point-max))
where that string is eventually placed by the overlay movement.

So apparently that specification neither serves to

(1) move the cursor visually to some position within the string (it
always remains before or after it), nor to

(2) move `point' to some position within the buffer (it's always at the
start and end positions of the overlay).

But what does it do?

 > You can see
 > this feature in action in cua-rectangle; after you play with it,
 > perhaps you could suggest how to improve the documentation.

IIUC in the Emacs sources the 'cursor property is never used in
connection with overlays.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Fri, 30 Mar 2012 09:35:20 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
> (defvar my-overlay nil)
> (defvar my-string nil)
> 
> (progn
>    (when (overlayp my-overlay)
>      (delete-overlay my-overlay))
>    (setq my-overlay (make-overlay 0 0))
>    (setq my-string
> 	(propertize "      " 'face 'lazy-highlight 'cursor 1))

What are you trying to accomplish by putting the `cursor' property on
more than a single character of the overlay string?  The display
engine doesn't expect it to be on more than one character in the
overlay string (it will put the cursor on the first and ignore the
rest).

> Evaluating this and moving `point' between buffer positions 336 and 337
> oscillates the cursor around the overlay.  If I instead use the line
> 
> 	(propertize "      " 'face 'lazy-highlight 'cursor 2))
> 
> in the above and reevaluate, moving `point' doesn't oscillate the cursor
> any more.  But if I additionally replace the last line as
> 
>    (move-overlay my-overlay (- (point-max) 2) (point-max)))
> 
> the cursor oscillates again (but now between positions 335 and 337).
> 
> I find the behavior good but am too silly to understand what's going on.
> What do I really specify when I write a form like
> 
> 	(propertize "      " 'face 'lazy-highlight 'cursor 1))
> 
> Apparently the "1" doesn't refer to a position within the "      " but
> to some position within the buffer from (- (point-max) 1) (point-max))
> where that string is eventually placed by the overlay movement.

The string character on which you put the `cursor' property of any
non-nil value is the character where you want the cursor displayed.
The value says for which buffer positions to display the cursor
there.  If the value is an integer number, the cursor is displayed
there when the buffer position is between N and N + n, where n is the
value of the property and N is the overlay-start position.  If the
value is anything else and non-nil, the cursor is displayed there only
when point is at overlay-start.

>  > this feature in action in cua-rectangle; after you play with it,
>  > perhaps you could suggest how to improve the documentation.
> 
> IIUC in the Emacs sources the 'cursor property is never used in
> connection with overlays.

??? I clearly see it in this snippet from cua-rect.el:

		     (if (cua--rectangle-right-side)
			 (put-text-property (1- (length ms)) (length ms) 'cursor 2 ms) <<<<<<<<<<<
		       (put-text-property 0 1 'cursor 2 ms)) <<<<<<<<<<<<<
		     (setq bs (concat bs ms)) <<<<<<<<<<<<<<<<
		     (setq rface nil))
 		    (t
		     (setq as (propertize
			       (make-string
				(- r cr0 (if (= le pr) 1 0))
				(if cua--virtual-edges-debug ?~ ?\s))
			       'face rface))
		     (if (cua--rectangle-right-side)
			 (put-text-property (1- (length as)) (length as) 'cursor 2 as) <<<<<<<<<<<
		       (put-text-property 0 1 'cursor 2 as)) <<<<<<<<<<<<<
		     (if (/= pr le)
			 (setq e (1- e))))))))
	     ;; Trim old leading overlays.
             (while (and old
                         (setq overlay (car old))
                         (< (overlay-start overlay) s)
                         (/= (overlay-end overlay) e))
               (delete-overlay overlay)
               (setq old (cdr old)))
             ;; Reuse an overlay if possible, otherwise create one.
             (if (and old
                      (setq overlay (car old))
                      (or (= (overlay-start overlay) s)
                          (= (overlay-end overlay) e)))
                 (progn
                   (move-overlay overlay s e)
                   (setq old (cdr old)))
               (setq overlay (make-overlay s e)))
 	     (overlay-put overlay 'before-string bs)  <<<<<<<<<<<<<<<
	     (overlay-put overlay 'after-string as)   <<<<<<<<<<<<<<<

In general, this feature was invented (By Kim Storm) specifically for
his cua-rect wizardry.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: darthandrus@gmail.com,  monnier@iro.umontreal.ca,  11068@debbugs.gnu.org
> Date: Mon, 26 Mar 2012 12:16:23 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is that final, or do you want to hear from Stefan, or think about it?
> 
> I've thought about it, and after staring at the code I'm pretty sure it
> does the right thing.  If the extra glyphs break anything, we have a
> regression bug on our hands for R2L lines anyway.  So I'll vouch for the
> change.  Please go ahead and commit the fixed patch ASAP.

Done.

I'm now closing this bug.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

 > What are you trying to accomplish by putting the `cursor' property on
 > more than a single character of the overlay string?

I thought I _have_ to do that in order to make the overlay know that for
every character covered by the 'cursor property the appropriate position
where to display the cursor is "1".  Apparently, the display engine does
derive that all by itself (which is good) but for me it was difficult to
understand.

 > The display
 > engine doesn't expect it to be on more than one character in the
 > overlay string (it will put the cursor on the first and ignore the
 > rest).

It's a good reaction but difficult to analyze.

 > The string character on which you put the `cursor' property of any
 > non-nil value is the character where you want the cursor displayed.

Aha.  It's now clear to me from the manual.  Initially, I thought that

   (overlay-put my-overlay 'cursor 3)

would put the cursor on the third character after the overlay's start
and after noticing that this does not happen I got lost.

 > The value says for which buffer positions to display the cursor
 > there.  If the value is an integer number, the cursor is displayed
 > there when the buffer position is between N and N + n, where n is the
 > value of the property and N is the overlay-start position.  If the
 > value is anything else and non-nil, the cursor is displayed there only
 > when point is at overlay-start.

I see that now.  It's not intuitive for me why I would want such
behavior (and it apparently doesn't work for negative numbers) but I
suppose it's needed by cua.  So the documentation is correct (once you
understand how it works ;-) ) just that the following part

      it specifies the number of buffer's
      character positions associated with the overlay string;

is misleading.  Maybe "associated with" could become "affected by"?

 > ??? I clearly see it in this snippet from cua-rect.el:

Another oversight.  I didn't notice that `ms' and `as' are put on
overlays.

Thanks, martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[martin rudalics]

Amendment: I see now why specifying a number for the cursor property
_is_ useful.  It seems that I can get what I initially mentioned (color
just the selected window) by doing


(defvar my-end-overlay nil)
(defvar my-start-overlay nil)
(defvar my-string nil)
(defvar my-window (selected-window))

(progn
   (when (overlayp my-end-overlay)
     (delete-overlay my-end-overlay))
   (when (overlayp my-start-overlay)
     (delete-overlay my-start-overlay))
   (setq my-end-overlay (make-overlay 0 0))
   (setq my-start-overlay (make-overlay 0 0))
   (overlay-put my-start-overlay 'face 'lazy-highlight)
   (overlay-put my-start-overlay 'window my-window)
   (move-overlay my-start-overlay (point-min) (point-max))

   (setq my-string (propertize (make-string 100 10) 'face 'lazy-highlight))
   (put-text-property 0 1 'cursor 100 my-string)
   (overlay-put my-end-overlay 'after-string my-string)
   (overlay-put my-end-overlay 'window my-window)
   (move-overlay my-end-overlay (point-max) (point-max)))


(ignoring stickyness, insertion, and change of selected window) just
that moving to the end of the buffer doesn't yet seem quite correct.

martin

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Fri, 30 Mar 2012 12:14:37 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: darthandrus@gmail.com, monnier@iro.umontreal.ca, 
>  cyd@stupidchicken.com, 11068@debbugs.gnu.org
> 
>  > The value says for which buffer positions to display the cursor
>  > there.  If the value is an integer number, the cursor is displayed
>  > there when the buffer position is between N and N + n, where n is the
>  > value of the property and N is the overlay-start position.  If the
>  > value is anything else and non-nil, the cursor is displayed there only
>  > when point is at overlay-start.
> 
> I see that now.  It's not intuitive for me why I would want such
> behavior (and it apparently doesn't work for negative numbers) but I
> suppose it's needed by cua.

You need this if you want a tight control on cursor position for
several buffer positions.

> So the documentation is correct (once you
> understand how it works ;-) ) just that the following part
> 
>       it specifies the number of buffer's
>       character positions associated with the overlay string;
> 
> is misleading.  Maybe "associated with" could become "affected by"?

I see no difference between "associated with" and "affected by", they
are both equally obscure.  I think I wrote that when I myself didn't
have a clear notion of what that number means.  I will try to come up
with a clearer wording, thanks for pointing this out.

bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> Date: Fri, 30 Mar 2012 14:42:59 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: darthandrus@gmail.com, 11068@debbugs.gnu.org
> 
> > So the documentation is correct (once you
> > understand how it works ;-) ) just that the following part
> > 
> >       it specifies the number of buffer's
> >       character positions associated with the overlay string;
> > 
> > is misleading.  Maybe "associated with" could become "affected by"?
> 
> I see no difference between "associated with" and "affected by", they
> are both equally obscure.  I think I wrote that when I myself didn't
> have a clear notion of what that number means.  I will try to come up
> with a clearer wording, thanks for pointing this out.

Done in trunk revision 107711.