GNU bug report logs - #76107
Consider image specs when scanning for a column

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 76107 in the body.
You can then email your comments to 76107 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Consider image specs when scanning for a column
Date: Fri, 07 Feb 2025 00:29:38 +0100

[Message part 1 (text/plain, inline)]

This patch handles display specs within `scan_for_column', which mainly
works to handle it in `current-column' and `move-to-column', which then
effects the filling functions and commands, and visual-fill-column, and
so on.

The patch seems to be working, as far as I can tell, but it would be
nice if someone could take a look.  Specifically, I copied the code to
convert the Lisp_Object holding the width into an integer from the code
above handling the space display properties, so that needs
double-checking.

I decided to use `Fcar_safe' instead of `XCAR' when taking the `car' of
`Fimage_size' just to be safe but it doesn't look to be necessary?  If
that's fine, then feel free to make the change.

[0001-Consider-image-specs-when-scanning-for-a-column.patch (text/x-patch, attachment)]

Message #8 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Fri, 07 Feb 2025 10:10:45 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Date: Fri, 07 Feb 2025 00:29:38 +0100
> 
> This patch handles display specs within `scan_for_column', which mainly
> works to handle it in `current-column' and `move-to-column', which then
> effects the filling functions and commands, and visual-fill-column, and
> so on.
> 
> The patch seems to be working, as far as I can tell, but it would be
> nice if someone could take a look.  Specifically, I copied the code to
> convert the Lisp_Object holding the width into an integer from the code
> above handling the space display properties, so that needs
> double-checking.
> 
> I decided to use `Fcar_safe' instead of `XCAR' when taking the `car' of
> `Fimage_size' just to be safe but it doesn't look to be necessary?  If
> that's fine, then feel free to make the change.

Thanks, but I don't see how this could work with just this code.  The
display property that uses images could be much more complex than just
giving the image file name or data.  First, there's the 'slice'
display spec, which shows a slice of an image.  More importantly, the
IMAGE-SPEC part of the display property could include properties that
affect the size of the image on display, and Fimage_size doesn't
necessarily take them into consideration, AFAIR.  Which image
properties that can appear in an image spec did you try with this
patch?  And finally, images are clipped on display when they exceed
the length of a screen line, which Fimage_size doesn't take into
consideration, because it doesn't know at which horizontal coordinate
the image will be shown.

So I'm afraid fixing check_display_width to support images will need
to simulate the display, i.e. use the move_it_* functions defined in
xdisp.c; just taking the image's size is not enough.  Or maybe you
could use Fbuffer_text_pixel_size (which needs a window to do its
calculations, which is another complication of supporting images in
this case).

Message #11 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Fri, 07 Feb 2025 17:28:01 +0100

> Thanks, but I don't see how this could work with just this code.  The
> display property that uses images could be much more complex than just
> giving the image file name or data.  First, there's the 'slice'
> display spec, which shows a slice of an image.  More importantly, the
> IMAGE-SPEC part of the display property could include properties that
> affect the size of the image on display, and Fimage_size doesn't
> necessarily take them into consideration, AFAIR.  Which image
> properties that can appear in an image spec did you try with this
> patch?  And finally, images are clipped on display when they exceed
> the length of a screen line, which Fimage_size doesn't take into
> consideration, because it doesn't know at which horizontal coordinate
> the image will be shown.
>
> So I'm afraid fixing check_display_width to support images will need
> to simulate the display, i.e. use the move_it_* functions defined in
> xdisp.c; just taking the image's size is not enough.  Or maybe you
> could use Fbuffer_text_pixel_size (which needs a window to do its
> calculations, which is another complication of supporting images in
> this case).

Mm, that's pretty bad.  I was mostly hoping that Fimage_size would do
all the heavylifting for me.

So, let me see if I have the components effecting the display size of an
image right:

1. The image spec properties need to be calculated properly.  It seems
   to me - from looking at the code and checking the image-size of a
   newly created image with various properties specified - that
   Fimage_size does all the calculations given the specs properly.  So,
   unless something in xdisp.c effects the display width of an image
   (note that I don't mean _visible_; we're (I am) assuming we can see
   everything here), it _should_ be correct to just rely on Fimage_size
   for this bit.

2. Slice display spec needs to be handled.  I'll add support for this in
   a bit.

3. Off-screen clipping is unlikely to be useful, and in fact it is
   better if we calculate the width without considering it, since when
   moving across columns we (at least I) behave as though everything was
   visible.

Is that correct?  Is there anything I'm missing here?  If not, I can go
ahead and try tackle the slice specs.

Message #14 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Fri, 07 Feb 2025 19:13:12 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Fri, 07 Feb 2025 17:28:01 +0100
> 
> > Thanks, but I don't see how this could work with just this code.  The
> > display property that uses images could be much more complex than just
> > giving the image file name or data.  First, there's the 'slice'
> > display spec, which shows a slice of an image.  More importantly, the
> > IMAGE-SPEC part of the display property could include properties that
> > affect the size of the image on display, and Fimage_size doesn't
> > necessarily take them into consideration, AFAIR.  Which image
> > properties that can appear in an image spec did you try with this
> > patch?  And finally, images are clipped on display when they exceed
> > the length of a screen line, which Fimage_size doesn't take into
> > consideration, because it doesn't know at which horizontal coordinate
> > the image will be shown.
> >
> > So I'm afraid fixing check_display_width to support images will need
> > to simulate the display, i.e. use the move_it_* functions defined in
> > xdisp.c; just taking the image's size is not enough.  Or maybe you
> > could use Fbuffer_text_pixel_size (which needs a window to do its
> > calculations, which is another complication of supporting images in
> > this case).
> 
> Mm, that's pretty bad.  I was mostly hoping that Fimage_size would do
> all the heavylifting for me.
> 
> So, let me see if I have the components effecting the display size of an
> image right:
> 
> 1. The image spec properties need to be calculated properly.  It seems
>    to me - from looking at the code and checking the image-size of a
>    newly created image with various properties specified - that
>    Fimage_size does all the calculations given the specs properly.  So,
>    unless something in xdisp.c effects the display width of an image
>    (note that I don't mean _visible_; we're (I am) assuming we can see
>    everything here), it _should_ be correct to just rely on Fimage_size
>    for this bit.
> 
> 2. Slice display spec needs to be handled.  I'll add support for this in
>    a bit.
> 
> 3. Off-screen clipping is unlikely to be useful, and in fact it is
>    better if we calculate the width without considering it, since when
>    moving across columns we (at least I) behave as though everything was
>    visible.

But everything is _not_ visible.  For example, if the image is at the
left edge of the window and the window is hscrolled, part of the image
will be invisible, and the columns of characters after it will be
incorrect.

> Is that correct?  Is there anything I'm missing here?  If not, I can go
> ahead and try tackle the slice specs.

No, you've misunderstood me.  The _only_ reliable way of knowing what
will be the width of an image on display is to simulate display.  We
could do what you have in mind, but then we'd need to document that
the result might not be accurate in the presence of images, and I'm
not sure this would be useful enough.

If image-size is enough, then Lisp programs that want to calculate
columns in the presence of images could simply add the value it
returns.  The result will be the same, and as (in)accurate as what
your code produces.

Message #17 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Fri, 07 Feb 2025 18:37:00 +0100

> But everything is _not_ visible.  For example, if the image is at the
> left edge of the window and the window is hscrolled, part of the image
> will be invisible, and the columns of characters after it will be
> incorrect.

Maybe I have this all wrong, but shouldn't we be scanning from the very
beginning of the line, and not just the _visible_ beginning?

When we have a row with the contents

  "some long text with a lot of characters that we need to scroll to see properly"

and we have it hscrolled until the visible row starts at "characters",
current-column at the start of "characters" is still 29 and not 0.

Similarly, I would think that given

  "#<image :width 10em>foo bar"

with the buffer hscrolled until it only shows "foo bar", we would want
current-column at the start of "foo" to be 10 and not 0, even though the
display width of the image is 0 (since it's not visible), because the
"actual" width of the image (that is, what its width _would_ be if we
could see it all) is 10.

Message #20 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Fri, 07 Feb 2025 21:55:56 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Fri, 07 Feb 2025 18:37:00 +0100
> 
> > But everything is _not_ visible.  For example, if the image is at the
> > left edge of the window and the window is hscrolled, part of the image
> > will be invisible, and the columns of characters after it will be
> > incorrect.
> 
> Maybe I have this all wrong, but shouldn't we be scanning from the very
> beginning of the line, and not just the _visible_ beginning?
> 
> When we have a row with the contents
> 
>   "some long text with a lot of characters that we need to scroll to see properly"
> 
> and we have it hscrolled until the visible row starts at "characters",
> current-column at the start of "characters" is still 29 and not 0.
> 
> Similarly, I would think that given
> 
>   "#<image :width 10em>foo bar"
> 
> with the buffer hscrolled until it only shows "foo bar", we would want
> current-column at the start of "foo" to be 10 and not 0, even though the
> display width of the image is 0 (since it's not visible), because the
> "actual" width of the image (that is, what its width _would_ be if we
> could see it all) is 10.

Maybe you are right.  But anyway, this is not the main point I was
trying to make.  The main point is that using image-size is not
accurately reproduce the actual horizontal coordinate on the screen.

Can you tell why you need image support in current-column and friends?
E.g., why posn-col-row and vertical-motion cannot do what you need?

Message #23 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 08 Feb 2025 19:56:45 +0100

> Maybe you are right.  But anyway, this is not the main point I was
> trying to make.  The main point is that using image-size is not
> accurately reproduce the actual horizontal coordinate on the screen.
>
> Can you tell why you need image support in current-column and friends?
> E.g., why posn-col-row and vertical-motion cannot do what you need?

I'm trying to get fill-paragraph (and visual-fill-column-mode) to work
properly in Org buffers with lots of previewed inline latex fragments.
fill-paragraph internally uses move-to-column, so that's what I'm
patching.  This isn't limited to Org, though.  Better image handling (or
at all) by default is a good thing to have in general.

Message #26 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 08 Feb 2025 21:28:58 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Sat, 08 Feb 2025 19:56:45 +0100
> 
> > Maybe you are right.  But anyway, this is not the main point I was
> > trying to make.  The main point is that using image-size is not
> > accurately reproduce the actual horizontal coordinate on the screen.
> >
> > Can you tell why you need image support in current-column and friends?
> > E.g., why posn-col-row and vertical-motion cannot do what you need?
> 
> I'm trying to get fill-paragraph (and visual-fill-column-mode) to work
> properly in Org buffers with lots of previewed inline latex fragments.
> fill-paragraph internally uses move-to-column, so that's what I'm
> patching.  This isn't limited to Org, though.  Better image handling (or
> at all) by default is a good thing to have in general.

I understand the reason now, but my opinion is still the same: proper
support for columns requires simulating the display, and the two
functions I mentioned already do that.  So if we want fill-paragraph
to work in the presence of images and other display features, we need
to have a specialized fill-paragraph-function which uses posn-col-row
instead of current-column and vertical-motion instead of
move-to-column.  I don't see a good reason for adding semi-working
image support to current-column and move-to-column.

Message #29 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 08 Feb 2025 23:17:02 +0100

> I understand the reason now, but my opinion is still the same: proper
> support for columns requires simulating the display, 

I would _really_ appreciate it if you would point me to the problem
point where calculating the image sizes (not the visible section - the
_whole_ thing) becomes infeasible without access to the full display (or
a simulation thereof).

> and the two functions I mentioned already do that.

I am not very familiar with events, but I don't see any way to do what
`move-to-column' does using any of the things you mentioned.

> So if we want fill-paragraph to work in the presence of images and
> other display features, we need to have a specialized
> fill-paragraph-function which uses posn-col-row instead of
> current-column and vertical-motion instead of move-to-column.

If we can have a version of fill-paragraph which can calculate image
widths, then that should just be _the_ fill-paragraph function.

> I don't see a good reason for adding semi-working image support to
> current-column and move-to-column.

Yes, I also don't want to have a semi-working result, but what part is
not working, other than `slice' support?

As far as I can see, in xdisp.c, `calc_pixel_width_or_height' is what
determines the display width of the image, which returns the basically
same thing Fimage_width does.  If there is some other step that I'm
missing, could you _please_ tell me where or what it is?

Message #32 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sun, 09 Feb 2025 09:08:37 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Sat, 08 Feb 2025 23:17:02 +0100
> 
> > I understand the reason now, but my opinion is still the same: proper
> > support for columns requires simulating the display, 
> 
> I would _really_ appreciate it if you would point me to the problem
> point where calculating the image sizes (not the visible section - the
> _whole_ thing) becomes infeasible without access to the full display (or
> a simulation thereof).

Look at the way images are processed in xdisp.c (starting at
handle_single_display_spec and then in produce_image_glyph and its
subroutines).  It isn't a single place, and the processing is complex.
Because of this complex processing spread over several functions, I
have hard time believing that just calling image-size will produce a
reliable result, and I don't see how will you prove it does produce a
reliable result without a lot of testing, unless you use the same code
as the display engine does.  Which is why I suggested to use other
functions, which do use the display code for this purpose.

> > and the two functions I mentioned already do that.
> 
> I am not very familiar with events, but I don't see any way to do what
> `move-to-column' does using any of the things you mentioned.

The call

  (vertical-motion '(COLUMN . 0))

will move point to the specified COLUMN in the current screen line.
If you don't have to handle continuation lines, this is all you need;
otherwise, you need some more code using display--line-is-continued-p,
because vertical-motion moves only within the current screen line, and
will stop at line continuation.

Similarly, the call

  (car (posn-col-row (posn-at-point POS)))

will return the visual column of the buffer position POS.  (If this is
on a continuation line, you need to add the columns of the previous
continued lines.)

Both of these functions use display code to calculate the column, so
they take images into account.

I'm sorry, but please look at this from my POV: I don't want us to be
left with a semi-working implementation that we will need to keep
fixing and maintaining for the years to come.  Adding support for
images to move-to-column means we promise it to work correctly and
will be expected to fix any inaccuracy people report.  Using the
display code frees us from this problem, because that's what Emacs
uses to display the images in the first place, so it will always be as
correct as what we show on display.

So from my POV, either using posn-col-row and vertical-motion in a
specialized fill-paragraph-function, or using move_it_* functions in
check_display_width for measuring the column size of an image, are the
ways of making this improvement that I very much prefer.  The latter
is I think a simpler and better solution, because it doesn't have the
complications with continuation lines.  I don't understand why you
insist so much on going with Fimage_width and not with the method I
proposed, I don't think the code will be much more complex.

> > So if we want fill-paragraph to work in the presence of images and
> > other display features, we need to have a specialized
> > fill-paragraph-function which uses posn-col-row instead of
> > current-column and vertical-motion instead of move-to-column.
> 
> If we can have a version of fill-paragraph which can calculate image
> widths, then that should just be _the_ fill-paragraph function.

Images are not the only display complication that current-column and
move-to-column doesn't handle.  For example, they don't support
xwidgets, and also produce incorrect results when there are display
strings with embedded newlines.  Because of that, fill-paragraph
doesn't work properly in the presence of these display features.
Therefore, adding images to it will still not make it work correctly
in all the cases.

> > I don't see a good reason for adding semi-working image support to
> > current-column and move-to-column.
> 
> Yes, I also don't want to have a semi-working result, but what part is
> not working, other than `slice' support?
> 
> As far as I can see, in xdisp.c, `calc_pixel_width_or_height' is what
> determines the display width of the image, which returns the basically
> same thing Fimage_width does.  If there is some other step that I'm
> missing, could you _please_ tell me where or what it is?

See the two functions mentioned above, handle_single_display_spec and
produce_image_glyph.  If you use the move_it_* functions, they call
these automatically.

Message #35 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Mon, 10 Feb 2025 16:18:34 +0100

[Message part 1 (text/plain, inline)]

> Look at the way images are processed in xdisp.c (starting at
> handle_single_display_spec and then in produce_image_glyph and its
> subroutines).  It isn't a single place, and the processing is complex.
> Because of this complex processing spread over several functions, I
> have hard time believing that just calling image-size will produce a
> reliable result, and I don't see how will you prove it does produce a
> reliable result without a lot of testing, unless you use the same code
> as the display engine does.  Which is why I suggested to use other
> functions, which do use the display code for this purpose.

I've attached the code for handle_single_display_spec and
produce_image_glyph, with (what I found to be) irrelevant code elided
and with my comments of what's happening where.  If there's nothing else
that's happening between these two steps (and I don't think there is - I
at least can't find any), then the display width (of unsliced images) is
exactly what Fimage_size returns, with the exception of width added due
to `box' face attributes.  The calculation is simple even when the image
is sliced:

  width = min(<slice width>, <image width> - <slice x>)
          [+ <image hmargin>, if slice is leftmost]
          [+ <image hmargin>, if slice is rightmost]

It is still meaningful to not duplicate the calculations being done, but
I don't think that the way to go is for the width calculators to use the
display code but for the display code to use the width calculators.  The
important question for me is this: Does the _actual_ width of a glyph
depend on where it is located in the display?  If not (and I imagine
not), then we should not need to simulate the display.

> I'm sorry, but please look at this from my POV: I don't want us to be
> left with a semi-working implementation that we will need to keep
> fixing and maintaining for the years to come.  Adding support for
> images to move-to-column means we promise it to work correctly and
> will be expected to fix any inaccuracy people report.  Using the
> display code frees us from this problem, because that's what Emacs
> uses to display the images in the first place, so it will always be as
> correct as what we show on display. [...] I don't understand why you
> insist so much on going with Fimage_width and not with the method I
> proposed, I don't think the code will be much more complex.

I understand where you're coming from, I also do not want to leave
behind an unmaintainable mess.  But I do not know xdisp.c enough to
actually be able to make use of it, and I cannot imagine that anyone
else has the motivation.

Still, here's my best shot.  I have attached a version of
`scan_for_column' which uses `move_it_in_display_line', which I've
written with my very limited understanding of how this is all supposed
to work.  I can't even test it, since I don't know how I would go about
recovering all the `prev*' values, but is it at least _somewhat_ in the
right direction?

> So from my POV, either using posn-col-row and vertical-motion in a
> specialized fill-paragraph-function, or using move_it_* functions in
> check_display_width for measuring the column size of an image, are the
> ways of making this improvement that I very much prefer.  The latter
> is I think a simpler and better solution, because it doesn't have the
> complications with continuation lines.
>
>> > So if we want fill-paragraph to work in the presence of images and
>> > other display features, we need to have a specialized
>> > fill-paragraph-function which uses posn-col-row instead of
>> > current-column and vertical-motion instead of move-to-column.
>> 
>> If we can have a version of fill-paragraph which can calculate image
>> widths, then that should just be _the_ fill-paragraph function.
>
> Images are not the only display complication that current-column and
> move-to-column doesn't handle.  For example, they don't support
> xwidgets, and also produce incorrect results when there are display
> strings with embedded newlines.  Because of that, fill-paragraph
> doesn't work properly in the presence of these display features.
> Therefore, adding images to it will still not make it work correctly
> in all the cases.

I think you misunderstood me; what I mean here is that if there is a
"specialized" fill-paragraph that handles display features, then that
should be _the_ fill-paragraph, and not be its own separate thing.

[commented xdisp.c code.c (text/plain, attachment)]

[scan_for_column alternative.c (text/plain, attachment)]

Message #38 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Mon, 10 Feb 2025 18:42:10 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Mon, 10 Feb 2025 16:18:34 +0100
> 
> I've attached the code for handle_single_display_spec and
> produce_image_glyph, with (what I found to be) irrelevant code elided
> and with my comments of what's happening where.  If there's nothing else
> that's happening between these two steps (and I don't think there is - I
> at least can't find any), then the display width (of unsliced images) is
> exactly what Fimage_size returns, with the exception of width added due
> to `box' face attributes.

Ok, so you have already discovered that the box attribute can change
the dimensions.  My point was exactly that: using the display code
will make sure we don't risk such omissions.

> I understand where you're coming from, I also do not want to leave
> behind an unmaintainable mess.  But I do not know xdisp.c enough to
> actually be able to make use of it, and I cannot imagine that anyone
> else has the motivation.
> 
> Still, here's my best shot.  I have attached a version of
> `scan_for_column' which uses `move_it_in_display_line', which I've
> written with my very limited understanding of how this is all supposed
> to work.  I can't even test it, since I don't know how I would go about
> recovering all the `prev*' values, but is it at least _somewhat_ in the
> right direction?

It is in the right direction, yes.  But I see now that we can make it
even simpler: we don't need move_it_in_display_line, we only need to
call produce_image_glyph (after setting up the 'struct it' object as
produce_image_glyph expects).  Then the width of the image on display
will be in it->pixel_width, and all that's left is convert that to
columns.

Then we just need to call this new code from check_display_width when
we find an image display property.  The rest of the code in
scan_for_column is okay and doesn't need to be replaced with move_it_*
functions.

So if you can implement this idea, feel free to show the resulting
patch.  Alternatively, wait for a few days until I find enough free
time to sit down and code this myself.

> I think you misunderstood me; what I mean here is that if there is a
> "specialized" fill-paragraph that handles display features, then that
> should be _the_ fill-paragraph, and not be its own separate thing.

Agreed, but only if the resulting fill-paragraph will not be
significantly slower.  And it could be significantly slower, since the
move_it_* functions are slower than just walking the buffer text one
character at a time.  If using the "specialized" fill-paragraph
becomes significantly slower due to support of images, it would make
sense not to use that method if the buffer is known to be devoid of
images.

Message #43 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: thuna.cing <at> gmail.com
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Thu, 13 Feb 2025 11:28:40 +0200

> Cc: 76107 <at> debbugs.gnu.org
> Date: Mon, 10 Feb 2025 18:42:10 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > Still, here's my best shot.  I have attached a version of
> > `scan_for_column' which uses `move_it_in_display_line', which I've
> > written with my very limited understanding of how this is all supposed
> > to work.  I can't even test it, since I don't know how I would go about
> > recovering all the `prev*' values, but is it at least _somewhat_ in the
> > right direction?
> 
> It is in the right direction, yes.  But I see now that we can make it
> even simpler: we don't need move_it_in_display_line, we only need to
> call produce_image_glyph (after setting up the 'struct it' object as
> produce_image_glyph expects).  Then the width of the image on display
> will be in it->pixel_width, and all that's left is convert that to
> columns.
> 
> Then we just need to call this new code from check_display_width when
> we find an image display property.  The rest of the code in
> scan_for_column is okay and doesn't need to be replaced with move_it_*
> functions.
> 
> So if you can implement this idea, feel free to show the resulting
> patch.  Alternatively, wait for a few days until I find enough free
> time to sit down and code this myself.

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.

Message #46 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
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.

I have two main problems, in case these help in some way:

1. When the current buffer isn't displayed in any window, you need
  _something_ for `w', but I don't know what.  Maybe make a copy(?) of
  the selected window and display the buffer there?  I don't see
  anything that would achieve that in window.c, though.

2. `gui_produce_glyphs' (which as I understand is a general version of
   `produce_image_glyph') doesn't seem to be moving CHARPOS, and I don't
   know how to recover that information from `it'.

Message #49 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Fri, 14 Feb 2025 09:54:48 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> 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.

> I have two main problems, in case these help in some way:
> 
> 1. When the current buffer isn't displayed in any window, you need
>   _something_ for `w', but I don't know what.  Maybe make a copy(?) of
>   the selected window and display the buffer there?  I don't see
>   anything that would achieve that in window.c, though.

The selected window (or the window in which the current buffer is
displayed, if any, as an optimization) should do, yes.

> 2. `gui_produce_glyphs' (which as I understand is a general version of
>    `produce_image_glyph') doesn't seem to be moving CHARPOS, and I don't
>    know how to recover that information from `it'.

Why do you need to move CHARPOS?  The character position should be set
to the beginning of the display property whose value is the image
spec, if the position at all matters.

Anyway, I will work on this soon.

Message #52 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: thuna.cing <at> gmail.com
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 15 Feb 2025 10:34:53 +0200

> Cc: 76107 <at> debbugs.gnu.org
> Date: Fri, 14 Feb 2025 09:54:48 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Thuna <thuna.cing <at> gmail.com>
> > Cc: 76107 <at> debbugs.gnu.org
> > 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;

Message #55 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: thuna.cing <at> gmail.com
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 01 Mar 2025 14:07:36 +0200

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

> Cc: 76107 <at> debbugs.gnu.org
> Date: Sat, 15 Feb 2025 10:34:53 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > Cc: 76107 <at> debbugs.gnu.org
> > Date: Fri, 14 Feb 2025 09:54:48 +0200
> > From: Eli Zaretskii <eliz <at> gnu.org>
> > 
> > > From: Thuna <thuna.cing <at> gmail.com>
> > > Cc: 76107 <at> debbugs.gnu.org
> > > 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;
> 
> 
> 
> 

Message #60 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Tue, 04 Mar 2025 22:13:06 +0100

> Ping!  Did you have time to try the patch I suggested?
Sorry, I wasn't able to find the time.

I also couldn't apply the patch, could you attach it as a separate file?

Message #63 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Wed, 05 Mar 2025 14:36:38 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Tue, 04 Mar 2025 22:13:06 +0100
> 
> > Ping!  Did you have time to try the patch I suggested?
> Sorry, I wasn't able to find the time.
> 
> I also couldn't apply the patch, could you attach it as a separate file?

The patch is below, thanks in advance for any feedback.

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;

Message #66 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Tue, 18 Mar 2025 02:59:37 +0100

I have been using the patch for a while now.  I have just been using it
for regular tasks, so I have not been explicitly testing any edge cases.

I have not yet had any crashes, and as far as I can see, image sizes are
being calculated correctly.

So far, I have only had two related issues: The width calculation is
incorrect when dealing with characters which are not full-sized (they
are being calculated as though they were full-sized), and left margin[1]
is being included in the column number, but only when there is an image
from line start to point.

For the second situation, here's the MRE:
1. emacs -q
2. Switch to a new buffer, turn on org-mode, insert the text
   > foo \( \mathbb{R}^{*} \)
3. Call org-latex-preview on the latex fragment.
4. Eval (current-column) at the start of line, end of "foo", and the end
   of line, you should see 0, 3, and 6.
5. Turn on display-line-numbers-mode, and repeat above, you should now
   see 0, 3, and 10.

Alternatively, you could have the buffer contents be
> --------
> * Foo
> foo \( \mathbb{R}^{*} \)
and turn on org-indent-mode (but not display-line-numbers-mode).  You'll
then see that the column at the end of the latex preview is the same as
if it were calculated from the left edge of the buffer (using the dashes
as reference).

[1] That's probably what it is?  The specific thing I am
referring to is org-indent-mode and display-line-numbers-mode, which I
assume works using left margin.

Message #69 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Thu, 20 Mar 2025 13:17:03 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Tue, 18 Mar 2025 02:59:37 +0100
> 
> I have been using the patch for a while now.  I have just been using it
> for regular tasks, so I have not been explicitly testing any edge cases.
> 
> I have not yet had any crashes, and as far as I can see, image sizes are
> being calculated correctly.

Thanks, that's good news.

> So far, I have only had two related issues: The width calculation is
> incorrect when dealing with characters which are not full-sized (they
> are being calculated as though they were full-sized)

Sorry, I don't understand: is this due to my patch?  My patch isn't
supposed to touch the case of characters, only that of inline images.
Was this problem present before my patch?  If not, can you show an
example of it?

> and left margin[1] is being included in the column number, but only
> when there is an image from line start to point.
> 
> For the second situation, here's the MRE:
> 1. emacs -q
> 2. Switch to a new buffer, turn on org-mode, insert the text
>    > foo \( \mathbb{R}^{*} \)
> 3. Call org-latex-preview on the latex fragment.
> 4. Eval (current-column) at the start of line, end of "foo", and the end
>    of line, you should see 0, 3, and 6.
> 5. Turn on display-line-numbers-mode, and repeat above, you should now
>    see 0, 3, and 10.
> 
> Alternatively, you could have the buffer contents be
> > --------
> > * Foo
> > foo \( \mathbb{R}^{*} \)
> and turn on org-indent-mode (but not display-line-numbers-mode).  You'll
> then see that the column at the end of the latex preview is the same as
> if it were calculated from the left edge of the buffer (using the dashes
> as reference).

Hmm... you say "there is an image from line start to point", but in
the example you show the image is not at the beginning of a line.  Did
I misunderstand what you are describing?

Anyway, I think I fixed this subtle issue in the patch below, which
should _replace_ the previous one.  If you want to apply the new
change incrementally, then just add this one line:

	      it.hpos = 1;

after this line in the patched code you were using until now:

	      it.last_visible_x = 1000000; /* prevent image clipping */

> [1] That's probably what it is?  The specific thing I am
> referring to is org-indent-mode and display-line-numbers-mode, which I
> assume works using left margin.

No, that's not the margin.  In org-indent-mode, the whitespace is the
line-prefix, and in display-line-numbers-mode it's the space reserved
for the line numbers.

Thanks again for testing this improvement.  Here's the full patch
relative to the current master:

diff --git a/src/indent.c b/src/indent.c
index 01cea22..cb09f3e 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,80 @@ 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 */
+	      /* Forcing HPOS non-zero prevents the display code from
+                 generating line/wrap-prefix and line numbers, which
+                 will skew the X coordinate we use below.  */
+	      it.hpos = 1;
+	      /* 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 +710,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;

Message #72 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Thu, 20 Mar 2025 18:14:35 +0100

>> So far, I have only had two related issues: The width calculation is
>> incorrect when dealing with characters which are not full-sized (they
>> are being calculated as though they were full-sized)
>
> Sorry, I don't understand: is this due to my patch?  My patch isn't
> supposed to touch the case of characters, only that of inline images.
> Was this problem present before my patch?  If not, can you show an
> example of it?

No, sorry, I didn't mean to imply that.  This was happening before as
well, but it's similar enough, so I figured I would bring it up in case
a small tweak is enough to fix it.

> Hmm... you say "there is an image from line start to point", but in
> the example you show the image is not at the beginning of a line.  Did
> I misunderstand what you are describing?

I was referring to an image being present somewhere in the line before
the point, but the patch seems to have fixed it, so that's fine.

Message #75 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Thu, 20 Mar 2025 22:46:11 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Thu, 20 Mar 2025 18:14:35 +0100
> 
> >> So far, I have only had two related issues: The width calculation is
> >> incorrect when dealing with characters which are not full-sized (they
> >> are being calculated as though they were full-sized)
> >
> > Sorry, I don't understand: is this due to my patch?  My patch isn't
> > supposed to touch the case of characters, only that of inline images.
> > Was this problem present before my patch?  If not, can you show an
> > example of it?
> 
> No, sorry, I didn't mean to imply that.  This was happening before as
> well, but it's similar enough, so I figured I would bring it up in case
> a small tweak is enough to fix it.

Ah, okay.  But in that case, this is the intended behavior:
current-column disregards the pixel size of the characters, and counts
each character as one column regardless of its size on display.

> > Hmm... you say "there is an image from line start to point", but in
> > the example you show the image is not at the beginning of a line.  Did
> > I misunderstand what you are describing?
> 
> I was referring to an image being present somewhere in the line before
> the point, but the patch seems to have fixed it, so that's fine.

OK, then the one-line addition to the patch that I sent should solve
this.

Message #78 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: thuna.cing <at> gmail.com
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 05 Apr 2025 11:34:20 +0300

> Cc: 76107 <at> debbugs.gnu.org
> Date: Thu, 20 Mar 2025 22:46:11 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Thuna <thuna.cing <at> gmail.com>
> > Cc: 76107 <at> debbugs.gnu.org
> > Date: Thu, 20 Mar 2025 18:14:35 +0100
> > 
> > >> So far, I have only had two related issues: The width calculation is
> > >> incorrect when dealing with characters which are not full-sized (they
> > >> are being calculated as though they were full-sized)
> > >
> > > Sorry, I don't understand: is this due to my patch?  My patch isn't
> > > supposed to touch the case of characters, only that of inline images.
> > > Was this problem present before my patch?  If not, can you show an
> > > example of it?
> > 
> > No, sorry, I didn't mean to imply that.  This was happening before as
> > well, but it's similar enough, so I figured I would bring it up in case
> > a small tweak is enough to fix it.
> 
> Ah, okay.  But in that case, this is the intended behavior:
> current-column disregards the pixel size of the characters, and counts
> each character as one column regardless of its size on display.
> 
> > > Hmm... you say "there is an image from line start to point", but in
> > > the example you show the image is not at the beginning of a line.  Did
> > > I misunderstand what you are describing?
> > 
> > I was referring to an image being present somewhere in the line before
> > the point, but the patch seems to have fixed it, so that's fine.
> 
> OK, then the one-line addition to the patch that I sent should solve
> this.

Did you have an opportunity to test the fixed code?  If there are no
problems with it you could report, I'd like to install the change.

Thanks.

Message #81 received at 76107 <at> debbugs.gnu.org (full text, mbox):

From: Thuna <thuna.cing <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 76107 <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 05 Apr 2025 10:38:01 +0200

> Did you have an opportunity to test the fixed code?  If there are no
> problems with it you could report, I'd like to install the change.

Yes, I have been using it without any visible issues so far, so it
should hopefully be good to go.

Message #86 received at 76107-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 76107-done <at> debbugs.gnu.org
Subject: Re: bug#76107: Consider image specs when scanning for a column
Date: Sat, 05 Apr 2025 13:30:32 +0300

> From: Thuna <thuna.cing <at> gmail.com>
> Cc: 76107 <at> debbugs.gnu.org
> Date: Sat, 05 Apr 2025 10:38:01 +0200
> 
> > Did you have an opportunity to test the fixed code?  If there are no
> > problems with it you could report, I'd like to install the change.
> 
> Yes, I have been using it without any visible issues so far, so it
> should hopefully be good to go.

Thanks, now installed on the master branch, and closing the bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.