bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
From emacs -Q:

Create a buffer with only the following text and no trailing newline
(note the leading space):

 XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

M-x fit-frame-to-buffer

The frame will be resized to the width of the X's, not including the
leading space, causing the line to wrap.

If the leading space is removed, it works as expected.

Any number of leading spaces are ignored, as are trailing spaces.

This has practical applications with company-posframe, which uses
sometimes one line child windows to show completion options and has
leading and trailing spaces around each completion.

In GNU Emacs 28.0.50 (build 1, x86_64-apple-darwin19.6.0, NS
appkit-1894.60 Version 10.15.7 (Build 19H114))
 of 2021-01-08 built on aaron-sub.local
Repository revision: d1c5e7afb1ad9890d925e8c1a5392b701a754dd5
Repository branch: master
Windowing system distributor 'Apple', version 10.3.1894
System Description:  Mac OS X 10.15.7


Aaron



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
I believe the problem is with:

(window-text-pixel-size nil t)

The FROM of t causes window-text-pixel-size to ignore leading spaces.
A TO of t, causes it to ignore trailing spaces. fit-frame-to-buffer
passes both as t. This is hardcoded unless you use
fit-frame-to-buffer-1

I can't say if window-text-pixel-size is behaving as expected or not.

Aaron



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
On Sat, Jan 9, 2021 at 9:57 AM Aaron Jensen <[hidden email]> wrote:

>
> I believe the problem is with:
>
> (window-text-pixel-size nil t)
>
> The FROM of t causes window-text-pixel-size to ignore leading spaces.
> A TO of t, causes it to ignore trailing spaces. fit-frame-to-buffer
> passes both as t. This is hardcoded unless you use
> fit-frame-to-buffer-1
>
> I can't say if window-text-pixel-size is behaving as expected or not.

The problematic code appears to be here in window-text-pixel-size:

else if (EQ (from, Qt))
  {
    start = BEGV;
    bpos = BEGV_BYTE;
    while (bpos < ZV_BYTE)
      {
c = fetch_char_advance (&start, &bpos);
if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
  break;
      }
    while (bpos > BEGV_BYTE)
      {
dec_both (&start, &bpos);
c = FETCH_BYTE (bpos);
if (!(c == ' ' || c == '\t'))
  break;
      }
  }

The second loop looks like it's attempting to backtrack to the
beginning of the line, but FETCH_BYTE (bpos) after a dec_both returns
the same character that the first loop ended on. In other words, start
and bpos are not in sync, or the way that FETCH_BYTE works is
different. If I subtract 1 from bpos when calling FETCH_BYTE, it works
as expected.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

martin rudalics
In reply to this post by Aaron Jensen
 > I believe the problem is with:
 >
 > (window-text-pixel-size nil t)
 >
 > The FROM of t causes window-text-pixel-size to ignore leading spaces.
 > A TO of t, causes it to ignore trailing spaces. fit-frame-to-buffer
 > passes both as t. This is hardcoded unless you use
 > fit-frame-to-buffer-1
 >
 > I can't say if window-text-pixel-size is behaving as expected or not.

Neither can I but the outcome is annoying.  What would you suggest to
do?  I ask because with

  XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

the leading space on the first line must be shown.  Maybe I don't see an
all too obvious fix here.

martin



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

martin rudalics
In reply to this post by Aaron Jensen
 > The problematic code appears to be here in window-text-pixel-size:
 >
 > else if (EQ (from, Qt))
 >    {
 >      start = BEGV;
 >      bpos = BEGV_BYTE;
 >      while (bpos < ZV_BYTE)
 >        {
 > c = fetch_char_advance (&start, &bpos);
 > if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
 >    break;
 >        }
 >      while (bpos > BEGV_BYTE)
 >        {
 > dec_both (&start, &bpos);
 > c = FETCH_BYTE (bpos);
 > if (!(c == ' ' || c == '\t'))
 >    break;
 >        }
 >    }
 >
 > The second loop looks like it's attempting to backtrack to the
 > beginning of the line, but FETCH_BYTE (bpos) after a dec_both returns
 > the same character that the first loop ended on. In other words, start
 > and bpos are not in sync, or the way that FETCH_BYTE works is
 > different. If I subtract 1 from bpos when calling FETCH_BYTE, it works
 > as expected.

Do you mean the first dec_both skips too much or not enough?  That code
was broken when I wrote it initially, someone fixed the char/byte issue
later and now I'm too silly to understand it.

martin



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Eli Zaretskii
In reply to this post by Aaron Jensen
> From: Aaron Jensen <[hidden email]>
> Date: Sat, 9 Jan 2021 10:27:12 -0600
>
> On Sat, Jan 9, 2021 at 9:57 AM Aaron Jensen <[hidden email]> wrote:
> >
> > I believe the problem is with:
> >
> > (window-text-pixel-size nil t)

You mean with

  (window-text-pixel-size t t)

right?

> > The FROM of t causes window-text-pixel-size to ignore leading spaces.
> > A TO of t, causes it to ignore trailing spaces. fit-frame-to-buffer
> > passes both as t. This is hardcoded unless you use
> > fit-frame-to-buffer-1

??? fit-frame-to-buffer always calls fit-frame-to-buffer-1, sow hat do
you mean by "unless"?

> The problematic code appears to be here in window-text-pixel-size:
>
> else if (EQ (from, Qt))
>   {
>     start = BEGV;
>     bpos = BEGV_BYTE;
>     while (bpos < ZV_BYTE)
>       {
> c = fetch_char_advance (&start, &bpos);
> if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
>   break;
>       }
>     while (bpos > BEGV_BYTE)
>       {
> dec_both (&start, &bpos);
> c = FETCH_BYTE (bpos);
> if (!(c == ' ' || c == '\t'))
>   break;
>       }
>   }
>
> The second loop looks like it's attempting to backtrack to the
> beginning of the line, but FETCH_BYTE (bpos) after a dec_both returns
> the same character that the first loop ended on.

No, it doesn't, it returns the byte at bpos after decrementing bpos.
So it's the character before that.

> In other words, start and bpos are not in sync

??? FETCH_BYTE doesn't change bpos, so if it was in sync with start
before FETCH_BYTE, it is still in sync after it.  So I don't think I
understand what you mean here.

Can you elaborate on your findings, please?



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
In reply to this post by martin rudalics
On Sat, Jan 9, 2021 at 11:07 AM martin rudalics <[hidden email]> wrote:
> Do you mean the first dec_both skips too much or not enough?  That code
> was broken when I wrote it initially, someone fixed the char/byte issue
> later and now I'm too silly to understand it.

It doesn't skip enough. I believe this fixes both the leading and
trailing space problems:

diff --git a/src/xdisp.c b/src/xdisp.c
index 6a4304d194..20e7ca3a1e 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10652,7 +10652,10 @@ DEFUN ("window-text-pixel-size",
Fwindow_text_pixel_size, Swindow_text_pixel_siz
  {
    c = fetch_char_advance (&start, &bpos);
    if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
-     break;
+            {
+              dec_both (&start, &bpos);
+       break;
+            }
  }
       while (bpos > BEGV_BYTE)
  {
@@ -10680,12 +10683,17 @@ DEFUN ("window-text-pixel-size",
Fwindow_text_pixel_size, Swindow_text_pixel_siz
  {
    dec_both (&end, &bpos);
    c = FETCH_BYTE (bpos);
    if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
-     break;
+            {
+              inc_both (&end, &bpos);
+       break;
+            }
  }
       while (bpos < ZV_BYTE)
  {
    c = fetch_char_advance (&end, &bpos);
    if (!(c == ' ' || c == '\t'))
      break;
  }

The problem is that the first loop leaves the pointer pointing to the
next character and it's only back tracked once.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
In reply to this post by Eli Zaretskii
On Sat, Jan 9, 2021 at 11:44 AM Eli Zaretskii <[hidden email]> wrote:
>
> You mean with
>
>   (window-text-pixel-size t t)
>
> right?

Yes, sorry bad copy/paste.

> ??? fit-frame-to-buffer always calls fit-frame-to-buffer-1, sow hat do
> you mean by "unless"?

I mean unless you call fit-frame-to-buffer-1 passing nil for from and
to, which I believe is the most likely desired behavior for something
like posframe, but I'm not the maintainer of that so I couldn't say
for certain.


> > The second loop looks like it's attempting to backtrack to the
> > beginning of the line, but FETCH_BYTE (bpos) after a dec_both returns
> > the same character that the first loop ended on.
>
> No, it doesn't, it returns the byte at bpos after decrementing bpos.
> So it's the character before that.

Maybe we're just getting hung up on my wording. After
fetch_char_advance, bpos points to the byte of the character after
what was returned from fetch_char_advance. If you then dec_both and
FETCH_BYTE, you will get the same character returned from the last
time fetch_char_advance was called, which was likely not the intent.

> > In other words, start and bpos are not in sync
>
> ??? FETCH_BYTE doesn't change bpos, so if it was in sync with start
> before FETCH_BYTE, it is still in sync after it.  So I don't think I
> understand what you mean here.
>
> Can you elaborate on your findings, please?

Yeah, I was mistaken on that point. They stay in sync. They both
needed to backtrack an extra time. See my patch in the email I sent
right before this one.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
In reply to this post by Aaron Jensen
On Sat, Jan 9, 2021 at 11:44 AM Aaron Jensen <[hidden email]> wrote:
>
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 6a4304d194..20e7ca3a1e 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -10652,7 +10652,10 @@ DEFUN ("window-text-pixel-size",
> Fwindow_text_pixel_size, Swindow_text_pixel_siz
>   {

It might actually be easier to read and understand if it was written
without fetch_char_advance and just used inc_both, dec_both and
FETCH_BYTE. I don't know if fetch_char_advance is doing something that
that combination wouldn't do, however, so I can't say if that'd be
safe or not.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Eli Zaretskii
In reply to this post by Aaron Jensen
> From: Aaron Jensen <[hidden email]>
> Date: Sat, 9 Jan 2021 11:44:46 -0600
> Cc: [hidden email]
>
> diff --git a/src/xdisp.c b/src/xdisp.c
> index 6a4304d194..20e7ca3a1e 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -10652,7 +10652,10 @@ DEFUN ("window-text-pixel-size",
> Fwindow_text_pixel_size, Swindow_text_pixel_siz
>   {
>     c = fetch_char_advance (&start, &bpos);
>     if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
> -     break;
> +            {
> +              dec_both (&start, &bpos);
> +       break;
> +            }
>   }

This increments position, then decrements it, which is sub-optimal.

>   {
>     dec_both (&end, &bpos);
>     c = FETCH_BYTE (bpos);
>     if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
> -     break;
> +            {
> +              inc_both (&end, &bpos);
> +       break;
> +            }
>   }

Same here.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Eli Zaretskii
In reply to this post by Aaron Jensen
> From: Aaron Jensen <[hidden email]>
> Date: Sat, 9 Jan 2021 11:55:06 -0600
> Cc: [hidden email]
>
> It might actually be easier to read and understand if it was written
> without fetch_char_advance and just used inc_both, dec_both and
> FETCH_BYTE.

Yes, probably.

But before we do any changes here, we need a test suite.  Would you
mind adding the necessary tests to test/src/xdisp-tests.el?



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
On Sat, Jan 9, 2021 at 12:14 PM Eli Zaretskii <[hidden email]> wrote:
> Yes, probably.
>
> But before we do any changes here, we need a test suite.  Would you
> mind adding the necessary tests to test/src/xdisp-tests.el?

Okay, I added a few basic tests for this case. I don't know if that's
the best way to test it, but it's what I could figure out.

The implementation bypasses the extra backtrack for the FROM, but the
algorithm for the TO is the same as I first submitted. In that case,
it's not really an extra backtrack since I have to dec to read the
byte. If I had access to dec_bytepos declared in syntax.c, I think I
could use that to avoid the extra backtrack (I'd fetch the dec_bytepos
(bpos)) and test that and then only move the pointers if not breaking
from the loop.

I left the fetch_char_advance in the TO algorithm since it didn't seem
necessary to replace it with separate fetch/inc_both.

This could be done w/o the backtracking entirely by keeping two
pointers, but that's more complicated, probably not much more
efficient (if at all) and this will likely never be called in a tight
loop.

Let me know how this looks and if you want me to make any tweaks.

0001-Fix-window-text-pixel-size-with-leading-trailing-spa.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

martin rudalics
 > Let me know how this looks and if you want me to make any tweaks.

   dec_both (&end, &bpos);
   c = FETCH_BYTE (bpos);
   if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))

Sorry but on that last line I have

          if (!(c == ' ' || c == '\t'))

or am I confused?

martin



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
On Sun, Jan 10, 2021 at 10:05 AM martin rudalics <[hidden email]> wrote:

>
>  > Let me know how this looks and if you want me to make any tweaks.
>
>           dec_both (&end, &bpos);
>           c = FETCH_BYTE (bpos);
>           if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
>
> Sorry but on that last line I have
>
>           if (!(c == ' ' || c == '\t'))
>
> or am I confused?

Not sure I follow, does the patch not apply?



Here is what I have after my patch for the handling of the TO == t:

      end = ZV;
      bpos = ZV_BYTE;
      while (bpos > BEGV_BYTE)
{
  dec_both (&end, &bpos);
  c = FETCH_BYTE (bpos);
  if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
            {
      inc_both (&end, &bpos);
      break;
            }
}
      while (bpos < ZV_BYTE)
{
  c = fetch_char_advance (&end, &bpos);
  if (!(c == ' ' || c == '\t'))
    break;
}

and before:

      end = ZV;
      bpos = ZV_BYTE;
      while (bpos > BEGV_BYTE)
{
  dec_both (&end, &bpos);
  c = FETCH_BYTE (bpos);
  if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
    break;
}
      while (bpos < ZV_BYTE)
{
  c = fetch_char_advance (&end, &bpos);
  if (!(c == ' ' || c == '\t'))
    break;
}

The difference is the inc_both before the break in the *first* loop,
not the second.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

martin rudalics
 > Not sure I follow, does the patch not apply?

It didn't for some reason but applies now and seems to DTRT.  So I think
you should install it.

Thanks, martin



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
On Sun, Jan 10, 2021 at 11:49 AM martin rudalics <[hidden email]> wrote:
>
>  > Not sure I follow, does the patch not apply?
>
> It didn't for some reason but applies now and seems to DTRT.  So I think
> you should install it.

Sorry, dtrt as in dtrt-mode? or something else? And by install do you
mean apply the patch (I'm not a committer). Sorry, just not up on al
the lingo :)

Thanks,

Aaron



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
On Sun, Jan 10, 2021 at 11:57 AM martin rudalics <[hidden email]> wrote:
>
> DTRT as in "does the right thing".

Ah, TIL (that's Today I Learned ;) )

> martin, who didn't know that a dtrt-mode existed

Hah

Best,

Aaron



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Eli Zaretskii
In reply to this post by Aaron Jensen
> From: Aaron Jensen <[hidden email]>
> Date: Tue, 12 Jan 2021 22:34:42 -0600
> Cc: martin rudalics <[hidden email]>, [hidden email]
>
> > On Sat, Jan 9, 2021 at 12:14 PM Eli Zaretskii <[hidden email]> wrote:
> > > Yes, probably.
> > >
> > > But before we do any changes here, we need a test suite.  Would you
> > > mind adding the necessary tests to test/src/xdisp-tests.el?
> >
> > Okay, I added a few basic tests for this case. I don't know if that's
> > the best way to test it, but it's what I could figure out.
> > ...
>
> Does this one look good to you?

Sorry, I didn't yet have time to take a good look at the changes.  I
will do that in a couple of days.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Eli Zaretskii
In reply to this post by Aaron Jensen
> From: Aaron Jensen <[hidden email]>
> Date: Sat, 9 Jan 2021 20:56:35 -0600
> Cc: martin rudalics <[hidden email]>, [hidden email]
>
> Let me know how this looks and if you want me to make any tweaks.

Thanks, I installed this on the master branch.

However, the new tests fail for me in batch execution on MS-Windows
with this error:

  Test xdisp-tests--window-text-pixel-size condition:
      (error "Not using an ASCII terminal now; cannot make a new ASCII frame")

It works if I run the test interactively.

The backtrace is

  make-terminal-frame(((parent-frame . #<frame F1 062dca38>)))
  tty-create-frame-with-faces(((parent-frame . #<frame F1 062dca38>)))
  #f(compiled-function (params) #<bytecode 0x6bc24af6be4b5d2>)(((paren
  apply(#f(compiled-function (params) #<bytecode 0x6bc24af6be4b5d2>) (
  frame-creation-function(((parent-frame . #<frame F1 062dca38>)))
  make-frame(((parent-frame . #<frame F1 062dca38>)))
  display-buffer-in-child-frame(#<killed buffer> nil)
  display-buffer(#<killed buffer> (display-buffer-in-child-frame))
  (let* ((window (display-buffer (current-buffer) '(display-buffer-in-
  (progn (insert "xx ") (let* ((window (display-buffer (current-buffer
  (unwind-protect (progn (insert "xx ") (let* ((window (display-buffer
  (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn
  (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current
  (closure (t) nil (let ((temp-buffer (generate-new-buffer " *temp*" t
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name xdisp-tests--window-text-pixel-size-t
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (tag :unstable)) #f(compiled-function (event-type
  ert-run-tests-batch((not (tag :unstable)))
  ert-run-tests-batch-and-exit((not (tag :unstable)))
  eval((ert-run-tests-batch-and-exit '(not (tag :unstable))) t)

I guess you are assuming some functionality that isn't supported on
all platforms?  Can you please rewrite the tests so that they work on
all platforms?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#45748: 28.0.50; fit-frame-to-buffer ignores leading spaces

Aaron Jensen
On Fri, Jan 15, 2021 at 6:11 AM Eli Zaretskii <[hidden email]> wrote:
>
> However, the new tests fail for me in batch execution on MS-Windows
> with this error:
>
>   Test xdisp-tests--window-text-pixel-size condition:
>       (error "Not using an ASCII terminal now; cannot make a new ASCII frame")
>
> It works if I run the test interactively.

The tests do not work in batch mode. I don't know of a way to test
window-text-pixel-size in a terminal since it requires GUI.

> I guess you are assuming some functionality that isn't supported on
> all platforms?  Can you please rewrite the tests so that they work on
> all platforms?

I don't think it's a platform issue. Is there a standard for tests
that are GUI/interactive only? Perhaps you could wrap the tests
accordingly?

Thanks,

Aaron



12