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

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

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

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 <aaronjensen@gmail.com> 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.

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

[martin rudalics]

 > 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

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

[martin rudalics]

 > 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

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 9 Jan 2021 10:27:12 -0600
> 
> On Sat, Jan 9, 2021 at 9:57 AM Aaron Jensen <aaronjensen@gmail.com> 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?

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

[Aaron Jensen]

On Sat, Jan 9, 2021 at 11:07 AM martin rudalics <rudalics@gmx.at> 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.

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

[Aaron Jensen]

On Sat, Jan 9, 2021 at 11:44 AM Eli Zaretskii <eliz@gnu.org> 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.

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

[Aaron Jensen]

On Sat, Jan 9, 2021 at 11:44 AM Aaron Jensen <aaronjensen@gmail.com> 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.

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 9 Jan 2021 11:44:46 -0600
> Cc: 45748@debbugs.gnu.org
> 
> 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.

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 9 Jan 2021 11:55:06 -0600
> Cc: 45748@debbugs.gnu.org
> 
> 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?

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

[Aaron Jensen]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

On Sat, Jan 9, 2021 at 12:14 PM Eli Zaretskii <eliz@gnu.org> 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.

[-- Attachment #2: 0001-Fix-window-text-pixel-size-with-leading-trailing-spa.patch --]
[-- Type: application/octet-stream, Size: 3434 bytes --]

From 595937263d77bf392a32961119e0ce7f8754525b Mon Sep 17 00:00:00 2001
From: Aaron Jensen <aaronjensen@gmail.com>
Date: Sat, 9 Jan 2021 20:43:32 -0600
Subject: [PATCH] Fix window-text-pixel-size with leading/trailing spaces
 (bug#45748)

First, scan to find the first non-whitespace character and then
backtrack to find the beginning of the line. The previous algorithm
always started on the non-whitespace character during the backtrack,
causing it to stop immediately and not actually find the beginning of
the line. The same applies to the end of line calculation.

* src/xdisp.c: (Fwindow_text_pixel_size): Fix off by one
* test/src/xdisp-tests.el (xdisp-tests--window-text-pixel-size): New test
(xdisp-tests--window-text-pixel-size-leading-space): New test
(xdisp-tests--window-text-pixel-size-trailing-space): New test
---
 src/xdisp.c             |  8 ++++++--
 test/src/xdisp-tests.el | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 6a4304d194..6c8bd346c0 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -10650,9 +10650,10 @@ DEFUN ("window-text-pixel-size", Fwindow_text_pixel_size, Swindow_text_pixel_siz
       bpos = BEGV_BYTE;
       while (bpos < ZV_BYTE)
 	{
-	  c = fetch_char_advance (&start, &bpos);
+	  c = FETCH_BYTE (bpos);
 	  if (!(c == ' ' || c == '\t' || c == '\n' || c == '\r'))
 	    break;
+	  inc_both (&start, &bpos);
 	}
       while (bpos > BEGV_BYTE)
 	{
@@ -10681,7 +10682,10 @@ 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)
 	{
diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index d13ce77a99..ec96d777ff 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -72,4 +72,34 @@ xdisp-tests--minibuffer-scroll
     (should (equal (nth 0 posns) (nth 1 posns)))
     (should (equal (nth 1 posns) (nth 2 posns)))))
 
+(ert-deftest xdisp-tests--window-text-pixel-size () ;; bug#45748
+  (with-temp-buffer
+    (insert "xxx")
+    (let* ((window
+            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
+          (char-width (frame-char-width))
+          (size (window-text-pixel-size nil t t)))
+      (delete-frame (window-frame window))
+      (should (equal (/ (car size) char-width) 3)))))
+
+(ert-deftest xdisp-tests--window-text-pixel-size-leading-space () ;; bug#45748
+  (with-temp-buffer
+    (insert " xx")
+    (let* ((window
+            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
+          (char-width (frame-char-width))
+          (size (window-text-pixel-size nil t t)))
+      (delete-frame (window-frame window))
+      (should (equal (/ (car size) char-width) 3)))))
+
+(ert-deftest xdisp-tests--window-text-pixel-size-trailing-space () ;; bug#45748
+  (with-temp-buffer
+    (insert "xx ")
+    (let* ((window
+            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
+          (char-width (frame-char-width))
+          (size (window-text-pixel-size nil t t)))
+      (delete-frame (window-frame window))
+      (should (equal (/ (car size) char-width) 3)))))
+
 ;;; xdisp-tests.el ends here
-- 
2.28.0

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

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 <rudalics@gmx.at> 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.

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

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 <rudalics@gmx.at> 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

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

[martin rudalics]

 >> 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?

DTRT as in "does the right thing".

 > or something else? And by install do you
 > mean apply the patch (I'm not a committer).

I meant to push it.  Eli, if you are OK with it, please push it.

 > Sorry, just not up on al
 > the lingo :)

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

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 <rudalics@gmx.at> 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

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

[Aaron Jensen]

Eli,

On Sat, Jan 9, 2021 at 8:56 PM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> On Sat, Jan 9, 2021 at 12:14 PM Eli Zaretskii <eliz@gnu.org> 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?

Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Tue, 12 Jan 2021 22:34:42 -0600
> Cc: martin rudalics <rudalics@gmx.at>, 45748@debbugs.gnu.org
> 
> > On Sat, Jan 9, 2021 at 12:14 PM Eli Zaretskii <eliz@gnu.org> 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.

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 9 Jan 2021 20:56:35 -0600
> Cc: martin rudalics <rudalics@gmx.at>, 45748@debbugs.gnu.org
> 
> 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.

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 <eliz@gnu.org> 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

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Fri, 15 Jan 2021 06:34:34 -0600
> Cc: martin rudalics <rudalics@gmx.at>, 45748@debbugs.gnu.org
> 
> >   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.

Why does it require GUI?

> I don't think it's a platform issue. Is there a standard for tests
> that are GUI/interactive only?

If we need to do this interactively, we could move the tests to
manual/redisplay-testsuite.el.  But I'd like first to try to make them
work in batch.

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

[Aaron Jensen]

On Fri, Jan 15, 2021 at 7:15 AM Eli Zaretskii <eliz@gnu.org> wrote:
> Why does it require GUI?

It may not, but I couldn't figure out how to control the window well
enough to predict its size, which is likely ignorance.
That's why I created a child frame so I knew that only what I wanted
was visible and I could predict its size.
I can try a few things later, though I'm open to any suggestions.

Thanks,

Aaron

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

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Fri, 15 Jan 2021 08:04:00 -0600
> Cc: martin rudalics <rudalics@gmx.at>, 45748@debbugs.gnu.org
> 
> On Fri, Jan 15, 2021 at 7:15 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > Why does it require GUI?
> 
> It may not, but I couldn't figure out how to control the window well
> enough to predict its size, which is likely ignorance.
> That's why I created a child frame so I knew that only what I wanted
> was visible and I could predict its size.
> I can try a few things later, though I'm open to any suggestions.

We don't really need to resize a frame, we just need to call the
function and check its result, right?

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

[Aaron Jensen]

On Fri, Jan 15, 2021 at 9:37 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> We don't really need to resize a frame, we just need to call the
> function and check its result, right?

I need to control the contents of a "visible" window and call the
function and check its result. When I attempted to do that, I wasn't
sure if I was getting the size of the test window or something else. I
didn't seem to be able to control it as I wanted to. See how the
current test works.

Aaron

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Aaron Jensen]

(xdisp-tests--window-text-pixel-size)
(xdisp-tests--window-text-pixel-size-leading-space)
(xdisp-tests--window-text-pixel-size-trailing-space): Fix tests
---
 test/src/xdisp-tests.el | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index ec96d777ff..09c2fa83b3 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -73,33 +73,36 @@ xdisp-tests--minibuffer-scroll
     (should (equal (nth 1 posns) (nth 2 posns)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size () ;; bug#45748
-  (with-temp-buffer
+  (with-current-buffer-window "*test*" 'display-buffer-reuse-window nil
+    (erase-buffer)
     (insert "xxx")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size (get-buffer-window) t t)))
+      (message "Size: %S" size)
+      (should (equal (/ (car size) char-width) 3)))))
+
+(ert-deftest xdisp-tests--window-text-pixel-size () ;; bug#45748
+  (with-current-buffer (generate-new-buffer "*test*")
+    (insert "xxx")
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t)))
       (should (equal (/ (car size) char-width) 3)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size-leading-space () ;; bug#45748
-  (with-temp-buffer
+  (with-current-buffer (generate-new-buffer "*test*")
     (insert " xx")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t)))
       (should (equal (/ (car size) char-width) 3)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size-trailing-space () ;; bug#45748
-  (with-temp-buffer
+  (with-current-buffer (generate-new-buffer "*test*")
     (insert "xx ")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t)))
       (should (equal (/ (car size) char-width) 3)))))
 
 ;;; xdisp-tests.el ends here
-- 
2.28.0

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Basil L. Contovounesios]

Aaron Jensen <aaronjensen@gmail.com> writes:

> diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
> index ec96d777ff..09c2fa83b3 100644
> --- a/test/src/xdisp-tests.el
> +++ b/test/src/xdisp-tests.el
> @@ -73,33 +73,36 @@ xdisp-tests--minibuffer-scroll
>      (should (equal (nth 1 posns) (nth 2 posns)))))
>  
>  (ert-deftest xdisp-tests--window-text-pixel-size () ;; bug#45748

FWIW, you can also include the bug number in the test's docstring, if
you prefer.

> -  (with-temp-buffer
> +  (with-current-buffer-window "*test*" 'display-buffer-reuse-window nil

Ideally the tests wouldn't leave this buffer lying around after they
have run; see (info "(ert) Tests and Their Environment").

Also, why does this test not use with-current-buffer like the other
ones?

> +    (erase-buffer)
>      (insert "xxx")
> -    (let* ((window
> -            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
> -          (char-width (frame-char-width))
> -          (size (window-text-pixel-size nil t t)))
> -      (delete-frame (window-frame window))
> +    (let* ((char-width (frame-char-width))

Nit: this and subsequent let* can also be let.

> +           (size (window-text-pixel-size (get-buffer-window) t t)))

Is get-buffer-window necessary?

> +      (message "Size: %S" size)

This debugging leftover should probably be removed.

> +      (should (equal (/ (car size) char-width) 3)))))

Thanks,

-- 
Basil

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Aaron Jensen]

Thanks Basil, updated.

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Aaron Jensen]

(xdisp-tests--window-text-pixel-size)
(xdisp-tests--window-text-pixel-size-leading-space)
(xdisp-tests--window-text-pixel-size-trailing-space): Fix tests
---
 test/src/xdisp-tests.el | 42 ++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index ec96d777ff..637cc796a3 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -73,33 +73,33 @@ xdisp-tests--minibuffer-scroll
     (should (equal (nth 1 posns) (nth 2 posns)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size () ;; bug#45748
-  (with-temp-buffer
+  (with-current-buffer (generate-new-buffer "*test*")
     (insert "xxx")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
-      (should (equal (/ (car size) char-width) 3)))))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t))
+           (width-in-chars (/ (car size) char-width)))
+      (kill-buffer)
+      (should (equal width-in-chars 3)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size-leading-space () ;; bug#45748
-  (with-temp-buffer
+  (with-current-buffer (generate-new-buffer "*test*")
     (insert " xx")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
-      (should (equal (/ (car size) char-width) 3)))))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t))
+           (width-in-chars (/ (car size) char-width)))
+      (kill-buffer)
+      (should (equal width-in-chars 3)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size-trailing-space () ;; bug#45748
-  (with-temp-buffer
+  (with-current-buffer (generate-new-buffer "*test*")
     (insert "xx ")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
-      (should (equal (/ (car size) char-width) 3)))))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t))
+           (width-in-chars (/ (car size) char-width)))
+      (kill-buffer)
+      (should (equal width-in-chars 3)))))
 
 ;;; xdisp-tests.el ends here
-- 
2.28.0

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Aaron Jensen]

Updated again to use with-temp-buffer

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Aaron Jensen]

(xdisp-tests--window-text-pixel-size)
(xdisp-tests--window-text-pixel-size-leading-space)
(xdisp-tests--window-text-pixel-size-trailing-space): Fix tests
---
 test/src/xdisp-tests.el | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index ec96d777ff..4e7d2ad8ab 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -75,31 +75,28 @@ xdisp-tests--minibuffer-scroll
 (ert-deftest xdisp-tests--window-text-pixel-size () ;; bug#45748
   (with-temp-buffer
     (insert "xxx")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
-      (should (equal (/ (car size) char-width) 3)))))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t))
+           (width-in-chars (/ (car size) char-width)))
+      (should (equal width-in-chars 3)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size-leading-space () ;; bug#45748
   (with-temp-buffer
     (insert " xx")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
-      (should (equal (/ (car size) char-width) 3)))))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t))
+           (width-in-chars (/ (car size) char-width)))
+      (should (equal width-in-chars 3)))))
 
 (ert-deftest xdisp-tests--window-text-pixel-size-trailing-space () ;; bug#45748
   (with-temp-buffer
     (insert "xx ")
-    (let* ((window
-            (display-buffer (current-buffer) '(display-buffer-in-child-frame . nil)))
-          (char-width (frame-char-width))
-          (size (window-text-pixel-size nil t t)))
-      (delete-frame (window-frame window))
-      (should (equal (/ (car size) char-width) 3)))))
+    (switch-to-buffer (current-buffer))
+    (let* ((char-width (frame-char-width))
+           (size (window-text-pixel-size nil t t))
+           (width-in-chars (/ (car size) char-width)))
+      (should (equal width-in-chars 3)))))
 
 ;;; xdisp-tests.el ends here
-- 
2.28.0

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Cc: eliz@gnu.org,
> 	contovob@tcd.ie,
> 	Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 16 Jan 2021 12:28:46 -0600
> 
> (xdisp-tests--window-text-pixel-size)
> (xdisp-tests--window-text-pixel-size-leading-space)
> (xdisp-tests--window-text-pixel-size-trailing-space): Fix tests

Thanks, please install this.

bug#45748: [PATCH] * test/src/xdisp-tests.el Fix tests to work in batch mode

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Sat, 16 Jan 2021 12:28:46 -0600
> Cc: contovob@tcd.ie, eliz@gnu.org, Aaron Jensen <aaronjensen@gmail.com>
> 
> (xdisp-tests--window-text-pixel-size)
> (xdisp-tests--window-text-pixel-size-leading-space)
> (xdisp-tests--window-text-pixel-size-trailing-space): Fix tests

Thanks, installed, and closing the bug.