Subject: Re: bug#76107: Consider image specs when scanning for a column

From: Eli Zaretskii <[email-masked]>
Date: Sat, 01 Mar 2025 14:07:36 +0200

Ping!  Did you have time to try the patch I suggested?

> Cc: [email-masked]
> Date: Sat, 15 Feb 2025 10:34:53 +0200
> From: Eli Zaretskii <[email-masked]>
> 
> > Cc: [email-masked]
> > Date: Fri, 14 Feb 2025 09:54:48 +0200
> > From: Eli Zaretskii <[email-masked]>
> > 
> > > From: Thuna <[email-masked]>
> > > Cc: [email-masked]
> > > Date: Thu, 13 Feb 2025 22:45:54 +0100
> > > 
> > > > Please tell me if you are or will be working on this, or you are
> > > > waiting for me to do it?  I'd like to avoid duplicate work.
> > > I am trying stuff, but the code looks roughly like what I sent before.
> > > It would probably be faster for you to do it.
> > 
> > OK, I will work on this.
> 
> The proposed patch is below.
> 
> You were right: the idea of using produce_image_glyph was not a good
> one, as the code would be too tedious.  So I've decided to go with
> higher-level functions instead, as you see below.
> 
> I did very little testing of the patch, so please test is as well as
> you can, both with images and slices.  The problem here is that image
> and slice specs can have different forms, so the code in
> check_display_width should detect at least the important and popular
> ones, and I'm not sure what's below is sufficient.
> 
> If you have any trouble with the patched Emacs, like crashes or
> incorrect results from current-column or move-to-column, please tell
> and show a recipe for reproducing the problems.
> 
> Thanks.
> 
> diff --git a/src/indent.c b/src/indent.c
> index 01cea22..c90d2f0 100644
> --- a/src/indent.c
> +++ b/src/indent.c
> @@ -466,13 +466,21 @@ current_column (void)
>  }
>  
>  
> +static void restore_window_buffer (Lisp_Object);
> +
>  /* Check the presence of a display property and compute its width.
> +   POS and POS_BYTE are the character and byte positions of the
> +   display property and COL is the column number at POS.
>     If a property was found and its width was found as well, return
>     its width (>= 0) and set the position of the end of the property
>     in ENDPOS.
> -   Otherwise just return -1.  */
> +   Otherwise just return -1.
> +   WINDOW is the window to use when it is important; nil stands for
> +   the selected window.  */
>  static int
> -check_display_width (ptrdiff_t pos, ptrdiff_t col, ptrdiff_t *endpos)
> +check_display_width (Lisp_Object window,
> +		     ptrdiff_t pos, ptrdiff_t pos_byte, ptrdiff_t col,
> +		     ptrdiff_t *endpos)
>  {
>    Lisp_Object val, overlay;
>  
> @@ -482,30 +490,76 @@ check_display_width (ptrdiff_t pos, ptrdiff_t col, ptrdiff_t *endpos)
>        int width = -1;
>        Lisp_Object plist = Qnil;
>  
> -      /* Handle '(space ...)' display specs.  */
> -      if (CONSP (val) && EQ (Qspace, XCAR (val)))
> -	{ /* FIXME: Use calc_pixel_width_or_height.  */
> -	  Lisp_Object prop;
> -	  EMACS_INT align_to_max =
> -	    (col < MOST_POSITIVE_FIXNUM - INT_MAX
> -	     ? (EMACS_INT) INT_MAX + col
> -	     : MOST_POSITIVE_FIXNUM);
> -
> -	  plist = XCDR (val);
> -	  if ((prop = plist_get (plist, QCwidth),
> -	       RANGED_FIXNUMP (0, prop, INT_MAX))
> -	      || (prop = plist_get (plist, QCrelative_width),
> -		  RANGED_FIXNUMP (0, prop, INT_MAX)))
> -	    width = XFIXNUM (prop);
> -	  else if (FLOATP (prop) && 0 <= XFLOAT_DATA (prop)
> -		   && XFLOAT_DATA (prop) <= INT_MAX)
> -	    width = (int)(XFLOAT_DATA (prop) + 0.5);
> -	  else if ((prop = plist_get (plist, QCalign_to),
> -		    RANGED_FIXNUMP (col, prop, align_to_max)))
> -	    width = XFIXNUM (prop) - col;
> -	  else if (FLOATP (prop) && col <= XFLOAT_DATA (prop)
> -		   && (XFLOAT_DATA (prop) <= align_to_max))
> -	    width = (int)(XFLOAT_DATA (prop) + 0.5) - col;
> +      if (CONSP (val))
> +	{
> +	  Lisp_Object xcar = XCAR (val);
> +
> +	  /* Handle '(space ...)' display specs.  */
> +	  if (EQ (Qspace, xcar))
> +	    { /* FIXME: Use calc_pixel_width_or_height.  */
> +	      Lisp_Object prop;
> +	      EMACS_INT align_to_max =
> +		(col < MOST_POSITIVE_FIXNUM - INT_MAX
> +		 ? (EMACS_INT) INT_MAX + col
> +		 : MOST_POSITIVE_FIXNUM);
> +
> +	      plist = XCDR (val);
> +	      if ((prop = plist_get (plist, QCwidth),
> +		   RANGED_FIXNUMP (0, prop, INT_MAX))
> +		  || (prop = plist_get (plist, QCrelative_width),
> +		      RANGED_FIXNUMP (0, prop, INT_MAX)))
> +		width = XFIXNUM (prop);
> +	      else if (FLOATP (prop) && 0 <= XFLOAT_DATA (prop)
> +		       && XFLOAT_DATA (prop) <= INT_MAX)
> +		width = (int)(XFLOAT_DATA (prop) + 0.5);
> +	      else if ((prop = plist_get (plist, QCalign_to),
> +			RANGED_FIXNUMP (col, prop, align_to_max)))
> +		width = XFIXNUM (prop) - col;
> +	      else if (FLOATP (prop) && col <= XFLOAT_DATA (prop)
> +		       && (XFLOAT_DATA (prop) <= align_to_max))
> +		width = (int)(XFLOAT_DATA (prop) + 0.5) - col;
> +	    }
> +	    /* Handle images.  */
> +	  else if (EQ (Qimage, xcar)
> +		   || EQ (Qslice, xcar)
> +		   /* 'insert-sliced-image' creates property of the form
> +                      ((slice ...) image ...) */
> +		   || (CONSP (xcar) && EQ (Qslice, XCAR (xcar))))
> +	    {
> +	      specpdl_ref count = SPECPDL_INDEX ();
> +	      struct window *w = decode_live_window (window);
> +
> +	      /* If needed, set the window's buffer temporarily to the
> +                 current buffer and its window-point to POS.  */
> +	      if (XBUFFER (w->contents) != current_buffer)
> +		{
> +		  Lisp_Object oldbuf
> +		    = list4 (window, w->contents,
> +			     make_fixnum (marker_position (w->pointm)),
> +			     make_fixnum (marker_byte_position (w->pointm)));
> +		  record_unwind_protect (restore_window_buffer, oldbuf);
> +		  wset_buffer (w, Fcurrent_buffer ());
> +		  set_marker_both (w->pointm, w->contents, pos, pos_byte);
> +		}
> +
> +	      struct text_pos startpos;
> +	      struct it it;
> +	      SET_TEXT_POS (startpos, pos, pos_byte);
> +	      void *itdata = bidi_shelve_cache ();
> +	      record_unwind_protect_void (unwind_display_working_on_window);
> +	      display_working_on_window_p = true;
> +	      start_display (&it, w, startpos);
> +	      it.last_visible_x = 1000000; /* prevent image clipping */
> +	      /* The POS+1 value is a trick: move_it_in_display_line
> +                 will not return until it finished processing the entire
> +                 image, even if it covers more than one buffer position.  */
> +	      move_it_in_display_line (&it, pos + 1, -1, MOVE_TO_POS);
> +	      /* The caller wants the width in units of the frame's
> +                 canonical character width.  */
> +	      width = ((double)it.current_x / FRAME_COLUMN_WIDTH (it.f)) + 0.5;
> +	      bidi_unshelve_cache (itdata, 0);
> +	      unbind_to (count, Qnil);
> +	    }
>  	}
>        /* Handle 'display' strings.   */
>        else if (STRINGP (val))
> @@ -652,7 +706,7 @@ scan_for_column (ptrdiff_t *endpos, EMACS_INT *goalcol,
>  
>        { /* Check display property.  */
>  	ptrdiff_t endp;
> -	int width = check_display_width (scan, col, &endp);
> +	int width = check_display_width (window, scan, scan_byte, col, &endp);
>  	if (width >= 0)
>  	  {
>  	    col += width;
> 
> 
> 
>