GNU bug report logs - #37858
27.0.50; Ensure a minimum width for `space` display prop

Previous Next

To reply to this bug, email your comments to 37858 AT debbugs.gnu.org.

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Ensure a minimum width for `space` display prop
Date: Mon, 21 Oct 2019 16:03:58 -0400

Package: Emacs
Version: 27.0.50


For text displayed in columns, alignment is generally obtained with
a `display` text-property of the form

    (space :align-to FOO)

This works great when the previous text ends before FOO, but when we
mis-calculate (or didn't calculate at all) and the previous text already
extends further than the desired alignment of the following text, such
space is reduced down to 0 pixels which is often not what we want.

Sometimes one can workaround this by placing 2 spaces in the buffer: one
with the :align-to and another fixed size space.  But it can be
cumbersome to do that and it leads to undesirable artifacts (e.g. the
cursor can be placed between the two space).

So, I'd like to extend our `space` specifications so as to be able to
specify a minimum width.  I came up with the patch below which lets you
write:

    (space :align-to FOO :min-width BAR)

which seems to work fine, but while trying to update the Elisp doc for
it I realized that maybe a better option is to extend the acceptable
forms for FOO so it can be of the form:

    (space :align-to (max FOO (+ BAR current-x)))

WDYT?


        Stefan


diff --git a/src/xdisp.c b/src/xdisp.c
index be1c209553..fd825d2dfe 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -29218,6 +29218,28 @@ append_stretch_glyph (struct it *it, Lisp_Object object,
 
 #endif	/* HAVE_WINDOW_SYSTEM */
 
+static int
+compute_relative_width (struct it *it, Lisp_Object prop)
+{
+  struct it it2;
+  unsigned char *p = BYTE_POS_ADDR (IT_BYTEPOS (*it));
+
+  it2 = *it;
+  if (it->multibyte_p)
+    it2.c = it2.char_to_display = STRING_CHAR_AND_LENGTH (p, it2.len);
+  else
+    {
+      it2.c = it2.char_to_display = *p, it2.len = 1;
+      if (! ASCII_CHAR_P (it2.c))
+	it2.char_to_display = BYTE8_TO_CHAR (it2.c);
+    }
+
+  it2.glyph_row = NULL;
+  it2.what = IT_CHARACTER;
+  PRODUCE_GLYPHS (&it2);
+  return NUMVAL (prop) * it2.pixel_width;
+}
+
 /* Produce a stretch glyph for iterator IT.  IT->object is the value
    of the glyph property displayed.  The value must be a list
    `(space KEYWORD VALUE ...)' with the following KEYWORD/VALUE pairs
@@ -29288,23 +29310,7 @@ produce_stretch_glyph (struct it *it)
       /* Relative width `:relative-width FACTOR' specified and valid.
 	 Compute the width of the characters having the `glyph'
 	 property.  */
-      struct it it2;
-      unsigned char *p = BYTE_POS_ADDR (IT_BYTEPOS (*it));
-
-      it2 = *it;
-      if (it->multibyte_p)
-	it2.c = it2.char_to_display = STRING_CHAR_AND_LENGTH (p, it2.len);
-      else
-	{
-	  it2.c = it2.char_to_display = *p, it2.len = 1;
-	  if (! ASCII_CHAR_P (it2.c))
-	    it2.char_to_display = BYTE8_TO_CHAR (it2.c);
-	}
-
-      it2.glyph_row = NULL;
-      it2.what = IT_CHARACTER;
-      PRODUCE_GLYPHS (&it2);
-      width = NUMVAL (prop) * it2.pixel_width;
+      width = compute_relative_width (it, prop);
     }
   else if ((prop = Fplist_get (plist, QCalign_to), !NILP (prop))
 	   && calc_pixel_width_or_height (&tem, it, prop, font, true,
@@ -29323,6 +29329,21 @@ produce_stretch_glyph (struct it *it)
     /* Nothing specified -> width defaults to canonical char width.  */
     width = FRAME_COLUMN_WIDTH (it->f);
 
+  if ((prop = Fplist_get (plist, QCmin_width), !NILP (prop))
+      && calc_pixel_width_or_height (&tem, it, prop, font, true, 0))
+    {
+      /* Absolute minimum width `:min-width WIDTH' specified and valid.  */
+      if (width < tem)
+        width = tem;
+    }
+  else if (prop = Fplist_get (plist, QCmin_relative_width), NUMVAL (prop) > 0)
+    {
+      /* Relative width `:min-relative-width FACTOR' specified and valid.  */
+      int tem = compute_relative_width (it, prop);
+      if (width < tem)
+        width = tem;
+    }
+
   if (width <= 0 && (width < 0 || !zero_width_ok_p))
     width = 1;
 
@@ -34275,6 +34296,8 @@ syms_of_xdisp (void)
   DEFSYM (QCalign_to, ":align-to");
   DEFSYM (QCrelative_width, ":relative-width");
   DEFSYM (QCrelative_height, ":relative-height");
+  DEFSYM (QCmin_relative_width, ":min-relative-width");
+  DEFSYM (QCmin_width, ":min-width");
   DEFSYM (QCeval, ":eval");
   DEFSYM (QCpropertize, ":propertize");
   DEFSYM (QCfile, ":file");

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

From: Robert Pluim <rpluim <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 37858 <at> debbugs.gnu.org
Subject: Re: bug#37858: 27.0.50; Ensure a minimum width for `space` display
 prop
Date: Tue, 22 Oct 2019 10:03:46 +0200

>>>>> On Mon, 21 Oct 2019 16:03:58 -0400, Stefan Monnier <monnier <at> iro.umontreal.ca> said:

    Stefan> Package: Emacs
    Stefan> Version: 27.0.50


    Stefan> For text displayed in columns, alignment is generally obtained with
    Stefan> a `display` text-property of the form

    Stefan>     (space :align-to FOO)

    Stefan> This works great when the previous text ends before FOO, but when we
    Stefan> mis-calculate (or didn't calculate at all) and the previous text already
    Stefan> extends further than the desired alignment of the following text, such
    Stefan> space is reduced down to 0 pixels which is often not what we want.

    Stefan> Sometimes one can workaround this by placing 2 spaces in the buffer: one
    Stefan> with the :align-to and another fixed size space.  But it can be
    Stefan> cumbersome to do that and it leads to undesirable artifacts (e.g. the
    Stefan> cursor can be placed between the two space).

    Stefan> So, I'd like to extend our `space` specifications so as to be able to
    Stefan> specify a minimum width.  I came up with the patch below which lets you
    Stefan> write:

    Stefan>     (space :align-to FOO :min-width BAR)

    Stefan> which seems to work fine, but while trying to update the Elisp doc for
    Stefan> it I realized that maybe a better option is to extend the acceptable
    Stefan> forms for FOO so it can be of the form:

    Stefan>     (space :align-to (max FOO (+ BAR current-x)))

Hmm, I think I probably prefer the former. Perhaps when you've
documented the semantics of your :min-relative-width addition Iʼll be
able to judge better :-)

Robert

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 37858 <at> debbugs.gnu.org
Subject: Re: bug#37858: 27.0.50;
 Ensure a minimum width for `space` display prop
Date: Tue, 22 Oct 2019 18:08:43 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Date: Mon, 21 Oct 2019 16:03:58 -0400
> 
> So, I'd like to extend our `space` specifications so as to be able to
> specify a minimum width.  I came up with the patch below which lets you
> write:
> 
>     (space :align-to FOO :min-width BAR)
> 
> which seems to work fine, but while trying to update the Elisp doc for
> it I realized that maybe a better option is to extend the acceptable
> forms for FOO so it can be of the form:
> 
>     (space :align-to (max FOO (+ BAR current-x)))

Since :align-to already supports expressions of the forms described in
the node "Pixel Specification", to have the latter you'd need:

  . implement a new OP called 'max' (and probably also 'min' for a
    good measure);
  . implement a new POS called, say, 'beg', which will evaluate to the
    x coordinate of where the space display property begins

and then you will have your feature for free, I think.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
Subject: Re: bug#37880: 27.0.50; Changing font size in Info-mode messes up
 formatting
Date: Sat, 07 May 2022 14:07:24 +0200

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> So, I'd like to extend our `space` specifications so as to be able to
> specify a minimum width.  I came up with the patch below which lets you
> write:
>
>     (space :align-to FOO :min-width BAR)
>
> which seems to work fine, but while trying to update the Elisp doc for
> it I realized that maybe a better option is to extend the acceptable
> forms for FOO so it can be of the form:
>
>     (space :align-to (max FOO (+ BAR current-x)))

I think I'd actually prefer the first form -- it's easy to reason about,
and does what most usage cases want (i.e., align if possible, but if
not, then at least leave some space so that things don't run into each
other).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
Subject: Re: bug#37880: 27.0.50; Changing font size in Info-mode messes up
 formatting
Date: Sat, 07 May 2022 10:29:54 -0400

Lars Ingebrigtsen [2022-05-07 14:07:24] wrote:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> So, I'd like to extend our `space` specifications so as to be able to
>> specify a minimum width.  I came up with the patch below which lets you
>> write:
>>
>>     (space :align-to FOO :min-width BAR)
>>
>> which seems to work fine, but while trying to update the Elisp doc for
>> it I realized that maybe a better option is to extend the acceptable
>> forms for FOO so it can be of the form:
>>
>>     (space :align-to (max FOO (+ BAR current-x)))
>
> I think I'd actually prefer the first form -- it's easy to reason about,
> and does what most usage cases want (i.e., align if possible, but if
> not, then at least leave some space so that things don't run into each
> other).

The patch I came up with back then doesn't work right.  IIRC it's
because we need to change both the redisplay code and the
`current-column` code and it only changed one of the two.

IIRC, I decided then that the right fix is to rewrite the
`current-column` code to use the redisplay code (instead of trying to
mimic it), but I didn't get around to that (and IIRC it's not
completely straightforward because `current-column` currently behaves
differently *on purpose* in some cases (most importantly w.r.t treating
ellipsis-erased text) so fixing it right will imply changes in behavior
and figuring out how to do it without breaking existing uses).


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: larsi <at> gnus.org, 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
Subject: Re: bug#37858: bug#37880: 27.0.50;
 Changing font size in Info-mode messes up formatting
Date: Sat, 07 May 2022 17:38:10 +0300

> Cc: 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
> Date: Sat, 07 May 2022 10:29:54 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> The patch I came up with back then doesn't work right.  IIRC it's
> because we need to change both the redisplay code and the
> `current-column` code and it only changed one of the two.
> 
> IIRC, I decided then that the right fix is to rewrite the
> `current-column` code to use the redisplay code (instead of trying to
> mimic it), but I didn't get around to that (and IIRC it's not
> completely straightforward because `current-column` currently behaves
> differently *on purpose* in some cases (most importantly w.r.t treating
> ellipsis-erased text) so fixing it right will imply changes in behavior
> and figuring out how to do it without breaking existing uses).

When did you last look at what current-column does in the context of
the issue being discussed here?

I'm not sure it does what you had in mind, but some changes were done
there recently, and the behavior in similar contexts did change.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
Subject: Re: bug#37858: bug#37880: 27.0.50; Changing font size in Info-mode
 messes up formatting
Date: Sat, 07 May 2022 11:58:41 -0400

Eli Zaretskii [2022-05-07 17:38:10] wrote:
> When did you last look at what current-column does in the context of
> the issue being discussed here?

It was soon after I posted this bug report.

> I'm not sure it does what you had in mind, but some changes were done
> there recently, and the behavior in similar contexts did change.

I haven't looked at recent changes in this area, so maybe things would
be a bit easier now, tho looking at `scan_for_column` in `master`,
I still see that we re-implement in `check_display_width` (part of) the
code used in `xdisp.c` to handle the display property, so AFAICT the
duplication is still present.


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: larsi <at> gnus.org, 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
Subject: Re: bug#37858: bug#37880: 27.0.50; Changing font size in Info-mode
 messes up formatting
Date: Sat, 07 May 2022 19:06:55 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: larsi <at> gnus.org,  37880 <at> debbugs.gnu.org,  37858 <at> debbugs.gnu.org
> Date: Sat, 07 May 2022 11:58:41 -0400
> 
> I haven't looked at recent changes in this area, so maybe things would
> be a bit easier now, tho looking at `scan_for_column` in `master`,
> I still see that we re-implement in `check_display_width` (part of) the
> code used in `xdisp.c` to handle the display property, so AFAICT the
> duplication is still present.

There's no duplication.  We simply use the same infrastructure as the
display code does.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: monnier <at> iro.umontreal.ca
Cc: larsi <at> gnus.org, 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
Subject: Re: bug#37858: bug#37880: 27.0.50;
 Changing font size in Info-mode messes up formatting
Date: Sat, 07 May 2022 19:20:33 +0300

> Cc: larsi <at> gnus.org, 37880 <at> debbugs.gnu.org, 37858 <at> debbugs.gnu.org
> Date: Sat, 07 May 2022 19:06:55 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> > Cc: larsi <at> gnus.org,  37880 <at> debbugs.gnu.org,  37858 <at> debbugs.gnu.org
> > Date: Sat, 07 May 2022 11:58:41 -0400
> > 
> > I haven't looked at recent changes in this area, so maybe things would
> > be a bit easier now, tho looking at `scan_for_column` in `master`,
> > I still see that we re-implement in `check_display_width` (part of) the
> > code used in `xdisp.c` to handle the display property, so AFAICT the
> > duplication is still present.
> 
> There's no duplication.  We simply use the same infrastructure as the
> display code does.

And, btw, I don't think I agree with your assertion that we should use
the display routines for current-column and friends.  The
column-oriented functions count actual (not canonical) columns,
whereas display code counts pixels.  It makes little sense to me to
call the display code, which will load fonts, call jit-lock, and
whatnot, just to count columns like current-column expects.

Previous Next

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