bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

The following code shows a problem with pos-visible-in-window-p
and bidirectional text. When there are right-to-left characters
at the beginning of the buffer, the function returns nil, as the
last line is not visible. However, when "a" is inserted into the
beginning of the buffer, the function returns t, which is
incorrect.

The bug happens in trunk and emacs-24, but not in emacs-23, so it
could be considered as a regression ;-)


(let (before after)
  (pop-to-buffer (get-buffer-create "test"))
  (erase-buffer)

  (dotimes (i (* 2 (window-height)))
    (insert "\u05d0\n"))		; HEBREW LETTER ALEF
  (insert "Last line\n")
  (goto-char (point-min))

  (setq before (pos-visible-in-window-p (point-max)))

  (insert "a")
  (setq after (pos-visible-in-window-p (point-max)))
  (message "Visible: before: %S, after: %S" before after))


In GNU Emacs 24.1.50.8 (x86_64-unknown-linux-gnu, GTK+ Version 3.4.1)
 of 2012-05-13 on arirop
Bzr revision: 108213 monnier@iro.umontreal.ca-20120513030506-t7ggxqlr92y8yjw4
Windowing system distributor `Fedora Project', version 11.0.11200000
Configured using:
 `configure '--with-x-toolkit=gtk3''

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Date: Sun, 13 May 2012 18:54:59 +0300
> 
> The following code shows a problem with pos-visible-in-window-p
> and bidirectional text. When there are right-to-left characters
> at the beginning of the buffer, the function returns nil, as the
> last line is not visible. However, when "a" is inserted into the
> beginning of the buffer, the function returns t, which is
> incorrect.

Sigh... where are you guys on weekends, when I _really_ have time to
work on Emacs?...

> The bug happens in trunk and emacs-24, but not in emacs-23, so it
> could be considered as a regression ;-)

Nothing that's related to R2L text in a left-to-right paragraph can
ever be a regression wrt Emacs 23.

But since the fix is quite simple, here you go: fixed in revision
107994 on the emacs-24 branch.

Thanks.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

>
> But since the fix is quite simple, here you go: fixed in revision
> 107994 on the emacs-24 branch.
>

Thank you. I can still reproduce the bug with that revision, but the
following tweak seems to help.  I'm not sure if it is correct, but at
least it fixes the testcase, and everything else seems to work okay.

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-05-13 18:22:35 +0000
+++ src/xdisp.c	2012-05-15 09:51:45 +0000
@@ -1313,7 +1313,7 @@
 	visible_p = bottom_y > window_top_y;
       else if (top_y < it.last_visible_y)
 	visible_p = 1;
-      if (bottom_y >= it.last_visible_y
+      if (bottom_y <= it.last_visible_y
 	  && it.bidi_p && it.bidi_it.scan_dir == -1
 	  && IT_CHARPOS (it) < charpos)
 	{


-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: 11464@debbugs.gnu.org
> Date: Tue, 15 May 2012 13:07:08 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >
> > But since the fix is quite simple, here you go: fixed in revision
> > 107994 on the emacs-24 branch.
> >
> 
> Thank you. I can still reproduce the bug with that revision, but the
> following tweak seems to help.  I'm not sure if it is correct, but at
> least it fixes the testcase, and everything else seems to work okay.
> 
> === modified file 'src/xdisp.c'
> --- src/xdisp.c	2012-05-13 18:22:35 +0000
> +++ src/xdisp.c	2012-05-15 09:51:45 +0000
> @@ -1313,7 +1313,7 @@
>  	visible_p = bottom_y > window_top_y;
>        else if (top_y < it.last_visible_y)
>  	visible_p = 1;
> -      if (bottom_y >= it.last_visible_y
> +      if (bottom_y <= it.last_visible_y
>  	  && it.bidi_p && it.bidi_it.scan_dir == -1
>  	  && IT_CHARPOS (it) < charpos)
>  	{

Interesting.  What are the values of bottom_y and it.last_visible_y
that you see?  I only see strict equality in that condition, so both
variants work for me.

Anyway, I installed the change you suggested, thanks.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

> Interesting.  What are the values of bottom_y and it.last_visible_y
> that you see?  I only see strict equality in that condition, so both
> variants work for me.

Tested with emacs-24 revision 107999:

  bottom_y = 300
  it.last_visible_y = 304

I also tested with two other cases and got different values.

With "emacs -nw" I got these values:

  bottom_y = 10
  it.last_visible_y = 10

With "emacs -fn fixed" I got these values:

  bottom_y = 270
  it.last_visible_y = 256


The bug doesn't show up in any of these cases.

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: 11464@debbugs.gnu.org
> Date: Wed, 16 May 2012 08:21:54 +0300
> 
> Tested with emacs-24 revision 107999:
> 
>   bottom_y = 300
>   it.last_visible_y = 304
> 
> I also tested with two other cases and got different values.
> 
> With "emacs -nw" I got these values:
> 
>   bottom_y = 10
>   it.last_visible_y = 10
> 
> With "emacs -fn fixed" I got these values:
> 
>   bottom_y = 270
>   it.last_visible_y = 256

Thanks.  For my education: In that last case with "-fn fixed", what is
the value of top_y and window_top_y to go with bottom_y = 270?

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ari Roponen <ari.roponen@gmail.com>
>>
>> With "emacs -fn fixed" I got these values:
>> 
>>   bottom_y = 270
>>   it.last_visible_y = 256
>
> Thanks.  For my education: In that last case with "-fn fixed", what is
> the value of top_y and window_top_y to go with bottom_y = 270?

bottom_y = 270
it.last_visible_y = 256
top_y = 255
window_top_y = 0

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: 11464@debbugs.gnu.org
> Date: Thu, 17 May 2012 07:52:11 +0300
> 
> bottom_y = 270
> it.last_visible_y = 256
> top_y = 255
> window_top_y = 0

Now I'm totally bewildered.  I don't understand how this case is at
all relevant to the bug.  Please bear with me while I explain what
puzzles me, and please point out what I missed.

Here's the relevant code fragment:

      int top_x = it.current_x;
      int top_y = it.current_y;
      /* Calling line_bottom_y may change it.method, it.position, etc.  */
      enum it_method it_method = it.method;
      int bottom_y = (last_height = 0, line_bottom_y (&it));
      int window_top_y = WINDOW_HEADER_LINE_HEIGHT (w);

      if (top_y < window_top_y)
	visible_p = bottom_y > window_top_y;
      else if (top_y < it.last_visible_y)
	visible_p = 1;
      if (bottom_y <= it.last_visible_y
	  && it.bidi_p && it.bidi_it.scan_dir == -1
	  && IT_CHARPOS (it) < charpos)
	{

The original problem was that the "else if" clause would incorrectly
set visible_p to 1.  The "if" clause I added after that, viz.:

      if (bottom_y <= it.last_visible_y
	  && it.bidi_p && it.bidi_it.scan_dir == -1
	  && IT_CHARPOS (it) < charpos)

attempts to correct that, by eventually resetting visible_p to zero.

Now, if bottom_y = 270, it.last_visible_y = 256, and top_y = 255, then
the condition in the above "else if" clause is false, and visible_p
could not possibly be set to 1.  The preceding "if" clause is also
false, since window_top_y = 0.  Therefore, visible_p should have
stayed zero for this situation, and I don't understand why you needed
to change

      if (bottom_y >= it.last_visible_y

into

      if (bottom_y <= it.last_visible_y

Can you tell where I'm wrong?

Thanks in advance.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ari Roponen <ari.roponen@gmail.com>
>> Cc: 11464@debbugs.gnu.org
>> Date: Thu, 17 May 2012 07:52:11 +0300
>> 
>> bottom_y = 270
>> it.last_visible_y = 256
>> top_y = 255
>> window_top_y = 0
>
> Now I'm totally bewildered.  I don't understand how this case is at
> all relevant to the bug.  Please bear with me while I explain what
> puzzles me, and please point out what I missed.

Sorry for being unclear. I gave values for three cases, but the bug only
happened in two of them. Your patch fixed the second case, my tweak
fixed the first case, and the third one (i.e. the one with the above
values) worked all the time.

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Michael Welsh Duggan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Ari Roponen <ari.roponen@gmail.com>
>> Cc: 11464@debbugs.gnu.org
>> Date: Thu, 17 May 2012 07:52:11 +0300
>> 
>> bottom_y = 270
>> it.last_visible_y = 256
>> top_y = 255
>> window_top_y = 0
>
> Now I'm totally bewildered.  I don't understand how this case is at
> all relevant to the bug.  Please bear with me while I explain what
> puzzles me, and please point out what I missed.
>
> Here's the relevant code fragment:
>
>       int top_x = it.current_x;
>       int top_y = it.current_y;
>       /* Calling line_bottom_y may change it.method, it.position, etc.  */
>       enum it_method it_method = it.method;
>       int bottom_y = (last_height = 0, line_bottom_y (&it));
>       int window_top_y = WINDOW_HEADER_LINE_HEIGHT (w);
>
>       if (top_y < window_top_y)
>         visible_p = bottom_y > window_top_y;
>       else if (top_y < it.last_visible_y)
>         visible_p = 1;
>       if (bottom_y <= it.last_visible_y
>           && it.bidi_p && it.bidi_it.scan_dir == -1
>           && IT_CHARPOS (it) < charpos)
>         {
>
> The original problem was that the "else if" clause would incorrectly
> set visible_p to 1.  The "if" clause I added after that, viz.:
>
>       if (bottom_y <= it.last_visible_y
>           && it.bidi_p && it.bidi_it.scan_dir == -1
>           && IT_CHARPOS (it) < charpos)
>
> attempts to correct that, by eventually resetting visible_p to zero.
>
> Now, if bottom_y = 270, it.last_visible_y = 256, and top_y = 255, then
> the condition in the above "else if" clause is false, and visible_p
> could not possibly be set to 1.  

Maybe I am misunderstanding, but I think possibly you've been staring at
the code too long.  

  if (top_y < window_top_y) ==> if (255 < 0) ==> if (FALSE)

  else if (top_y < it.last_visible_y) ==> if (255 < 256) ==> if (TRUE)

hence, visible_p _would_ be set to 1 by the else if clause.

-- 
Michael Welsh Duggan
(mwd@cert.org)

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Michael Welsh Duggan <mwd@cert.org>
> Cc: Ari Roponen <ari.roponen@gmail.com>, 11464@debbugs.gnu.org
> Date: Thu, 17 May 2012 13:56:45 -0400
> 
>   else if (top_y < it.last_visible_y) ==> if (255 < 256) ==> if (TRUE)
> 
> hence, visible_p _would_ be set to 1 by the else if clause.

And will never be reset to zero, because 27 <= 256 => FALSE.  And yet
Ari said that he needed to change the condition to <=.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Michael Welsh Duggan]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Welsh Duggan <mwd@cert.org>
>> Cc: Ari Roponen <ari.roponen@gmail.com>, 11464@debbugs.gnu.org
>> Date: Thu, 17 May 2012 13:56:45 -0400
>> 
>>   else if (top_y < it.last_visible_y) ==> if (255 < 256) ==> if (TRUE)
>> 
>> hence, visible_p _would_ be set to 1 by the else if clause.
>
> And will never be reset to zero, because 27 <= 256 => FALSE.  And yet
> Ari said that he needed to change the condition to <=.

But where do you get 27?  The number was 270, I believe.

>> bottom_y = 270
>> it.last_visible_y = 256
>> top_y = 255
>> window_top_y = 0

-- 
Michael Welsh Duggan
(mwd@cert.org)

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Michael Welsh Duggan <mwd@cert.org>
> Cc: ari.roponen@gmail.com, 11464@debbugs.gnu.org
> Date: Thu, 17 May 2012 17:22:36 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Michael Welsh Duggan <mwd@cert.org>
> >> Cc: Ari Roponen <ari.roponen@gmail.com>, 11464@debbugs.gnu.org
> >> Date: Thu, 17 May 2012 13:56:45 -0400
> >> 
> >>   else if (top_y < it.last_visible_y) ==> if (255 < 256) ==> if (TRUE)
> >> 
> >> hence, visible_p _would_ be set to 1 by the else if clause.
> >
> > And will never be reset to zero, because 27 <= 256 => FALSE.  And yet
> > Ari said that he needed to change the condition to <=.
> 
> But where do you get 27?  The number was 270, I believe.

It's 270, yes.  But the rest is correct: if visible_p is set  in this
case, it will never be reset with the current code.  My original code
used

      if (bottom_y >= it.last_visible_y

which would have caught this case.

Ari, can you please describe again what happens in this particular
case on your machine, step by step, when you step with a debugger
through the relevant fragment?

I'm sorry I keep asking questions, but I must understand what was
incorrect in my original reasoning, to make sure there's no subtle
bugs lurking here.

Also, for this case:

>  bottom_y = 300
>  it.last_visible_y = 304

can you show the corresponding values of top_y and window_top_y?

Thanks.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

>
> It's 270, yes.  But the rest is correct: if visible_p is set  in this
> case, it will never be reset with the current code.  My original code
> used
>
>       if (bottom_y >= it.last_visible_y
>
> which would have caught this case.
>
> Ari, can you please describe again what happens in this particular
> case on your machine, step by step, when you step with a debugger
> through the relevant fragment?

I sent this reply tonight:
http://lists.gnu.org/archive/html/bug-gnu-emacs/2012-05/msg00476.html

The summary is that the case you are wondering about didn't have the bug
at all.

I tried to reproduce the bug in all three cases with emacs-24 revision
107933. The first case "emacs -Q" had the bug; "emacs -nw" had the bug;
"emacs -fn fixed" didn't have the bug.

Then I updated to revision 107934 (your patch). That fixed the "emacs
-nw" case. My tweak fixed the "emacs -Q" case.

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Ari Roponen <ari.roponen@gmail.com> writes:

> I sent this reply tonight:
> http://lists.gnu.org/archive/html/bug-gnu-emacs/2012-05/msg00476.html
>
> The summary is that the case you are wondering about didn't have the bug
> at all.
>
> I tried to reproduce the bug in all three cases with emacs-24 revision
> 107933. The first case "emacs -Q" had the bug; "emacs -nw" had the bug;
> "emacs -fn fixed" didn't have the bug.
>
> Then I updated to revision 107934 (your patch). That fixed the "emacs
> -nw" case. My tweak fixed the "emacs -Q" case.

Of course, I meant revisions 107993 and 107994.  And the reply was sent
yesterday ;-)

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: Michael Welsh Duggan <mwd@cert.org>,  11464@debbugs.gnu.org
> Date: Fri, 18 May 2012 11:03:52 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >
> > It's 270, yes.  But the rest is correct: if visible_p is set  in this
> > case, it will never be reset with the current code.  My original code
> > used
> >
> >       if (bottom_y >= it.last_visible_y
> >
> > which would have caught this case.
> >
> > Ari, can you please describe again what happens in this particular
> > case on your machine, step by step, when you step with a debugger
> > through the relevant fragment?
> 
> I sent this reply tonight:
> http://lists.gnu.org/archive/html/bug-gnu-emacs/2012-05/msg00476.html
> 
> The summary is that the case you are wondering about didn't have the bug
> at all.

Yes, you did say that.  But as Michael points out, this fragment

      else if (top_y < it.last_visible_y)
	visible_p = 1;

should have set visible_p to 1 in your last case, the one where

> bottom_y = 270
> it.last_visible_y = 256
> top_y = 255
> window_top_y = 0

And if that happens, the correction code below, viz.:

      if (bottom_y <= it.last_visible_y
	  && it.bidi_p && it.bidi_it.scan_dir == -1
	  && IT_CHARPOS (it) < charpos)

would evaluate to false, and visible_p would have stayed at its 1
value, which is wrong.

So could you please clarify this case?

Also, what was the value of top_y in your first case, i.e.:

> bottom_y = 300
> it.last_visible_y = 304

Again, sorry for bothering you with the details.  I just must
understand what is going on here and where did I err in my original
change.

Thanks!

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

> And if that happens, the correction code below, viz.:
>
>       if (bottom_y <= it.last_visible_y
> 	  && it.bidi_p && it.bidi_it.scan_dir == -1
> 	  && IT_CHARPOS (it) < charpos)
>
> would evaluate to false, and visible_p would have stayed at its 1
> value, which is wrong.
>
> So could you please clarify this case?

Yes, pos_visible_p indeed returns incorrect value (1), but it is called
from Fpos_visible_in_window_p, which returns the correct value.  For
longer description, see below.

>
> Also, what was the value of top_y in your first case, i.e.:
>
>> bottom_y = 300
>> it.last_visible_y = 304

That case has the following values now (in emacs-24 rev. 108005):

bottom_y = 302
it.last_visible_y = 304
top_y = 285

I'm not sure why bottom_y has changed its value.  I guess that is
because I installed some new fonts.

>
> Again, sorry for bothering you with the details.  I just must
> understand what is going on here and where did I err in my original
> change.

No worries.  Here are some logs from my debugging sessions.  I don't
pretend to know the code, but this shows why the bug didn't happen in
the last case ("emacs -fn fixed").

I tried emacs-24 revision 108005, compiled with "-O0 -ggdb".

File bug.el contains this:

(progn
  (pop-to-buffer (get-buffer-create "test"))
  (erase-buffer)
  (dotimes (i (* 2 (window-height)))
    (insert "\u05d0\n"))		; HEBREW LETTER ALEF
  (insert "Last line\n")
  (goto-char (point-min))
  (insert "a")
  (message "Should be nil: %S" (pos-visible-in-window-p (point-max))))


The function pos_visible_p returns the incorrect value (1):

[src]$ gdb --quiet --args ./emacs -Q -fn fixed -l bug.el
Reading symbols from /usr/local/repos/bzr/emacs/emacs-24/src/emacs...done.
warning: File "/usr/local/repos/bzr/emacs/emacs-24/src/.gdbinit" auto-loading has been declined by your `auto-load safe-path' set to "/usr/share/gdb/auto-load:/usr/lib/debug:/usr/bin/mono-gdb.py".
(gdb) source .gdbinit
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from terminal]
DISPLAY = :0
TERM = xterm
Breakpoint 1 at 0x5623fd: file emacs.c, line 394.
Temporary breakpoint 2 at 0x588153: file sysdep.c, line 859.
(gdb) b xdisp.c:1311
Breakpoint 3 at 0x4311d1: file xdisp.c, line 1311.
(gdb) b xdisp.c:1566
Breakpoint 4 at 0x4324d2: file xdisp.c, line 1566.
(gdb) r
Starting program: /usr/local/repos/bzr/emacs/emacs-24/src/emacs -Q -fn fixed -l bug.el
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffe4578700 (LWP 28308)]
[New Thread 0x7fffe3d77700 (LWP 28309)]

Breakpoint 3, pos_visible_p (w=0x13dbd90, charpos=80, x=0x7fffffffb78c, y=
    0x7fffffffb788, rtop=0x7fffffffb79c, rbot=0x7fffffffb798, rowh=
    0x7fffffffb794, vpos=0x7fffffffb790) at xdisp.c:1312
1312	      if (top_y < window_top_y)
Missing separate debuginfos, use: debuginfo-install [...]
(gdb) p visible_p
$1 = 0
(gdb) n
1314	      else if (top_y < it.last_visible_y)
(gdb) 
1315		visible_p = 1;
(gdb) 
1316	      if (bottom_y <= it.last_visible_y
(gdb) 
1341	      if (visible_p)
(gdb) p visible_p
$2 = 1
(gdb) c
Continuing.

Breakpoint 4, pos_visible_p (w=0x13dbd90, charpos=80, x=0x7fffffffb78c, y=
    0x7fffffffb788, rtop=0x7fffffffb79c, rbot=0x7fffffffb798, rowh=
    0x7fffffffb794, vpos=0x7fffffffb790) at xdisp.c:1566
1566	  return visible_p;
(gdb) p visible_p
$3 = 1
(gdb)


The reason the test case works is that the function
Fpos_visible_in_window_p contains this:

  /* If position is above window start or outside buffer boundaries,
     or if window start is out of range, position is not visible.  */
  if ((EQ (pos, Qt)
       || (posint >= CHARPOS (top) && posint <= BUF_ZV (buf)))
      && CHARPOS (top) >= BUF_BEGV (buf)
      && CHARPOS (top) <= BUF_ZV (buf)
      && pos_visible_p (w, posint, &x, &y, &rtop, &rbot, &rowh, &vpos)
      && (fully_p = !rtop && !rbot, (!NILP (partially) || fully_p)))
    in_window = Qt;

Here pos_visible_p returns 1, but the next condition evaluates to false, since
(!NILP (partially) || fully_p) is false.

[src]$ gdb --quiet --args ./emacs -Q -fn fixed -l bug.el
Reading symbols from /usr/local/repos/bzr/emacs/emacs-24/src/emacs...done.
warning: File "/usr/local/repos/bzr/emacs/emacs-24/src/.gdbinit" auto-loading has been declined by your `auto-load safe-path' set to "/usr/share/gdb/auto-load:/usr/lib/debug:/usr/bin/mono-gdb.py".
(gdb) source .gdbinit
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from terminal]
DISPLAY = :0
TERM = xterm
Breakpoint 1 at 0x5623fd: file emacs.c, line 394.
Temporary breakpoint 2 at 0x588153: file sysdep.c, line 859.
(gdb) b Fpos_visible_in_window_p
Breakpoint 3 at 0x48a5db: file window.c, line 1431.
(gdb) r
Starting program: /usr/local/repos/bzr/emacs/emacs-24/src/emacs -Q -fn fixed -l bug.el
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffe4578700 (LWP 28471)]
[New Thread 0x7fffe3d77700 (LWP 28472)]

Breakpoint 3, Fpos_visible_in_window_p (pos=320, window=12763682, partially=
    12763682) at window.c:1431
1431	  Lisp_Object in_window = Qnil;
Missing separate debuginfos, use: debuginfo-install [..]
(gdb) n
1432	  int rtop, rbot, rowh, vpos, fully_p = 1;
(gdb) 
1435	  w = decode_window (window);
(gdb) 
1436	  buf = XBUFFER (w->buffer);
(gdb) 
1437	  SET_TEXT_POS_FROM_MARKER (top, w->start);
(gdb) 
1439	  if (EQ (pos, Qt))
(gdb) 
1441	  else if (!NILP (pos))
(gdb) 
1443	      CHECK_NUMBER_COERCE_MARKER (pos);
(gdb) 
1444	      posint = XINT (pos);
(gdb) 
1453	  if ((EQ (pos, Qt)
(gdb) 
1454	       || (posint >= CHARPOS (top) && posint <= BUF_ZV (buf)))
(gdb) 
1455	      && CHARPOS (top) >= BUF_BEGV (buf)
(gdb) 
1456	      && CHARPOS (top) <= BUF_ZV (buf)
(gdb) 
1457	      && pos_visible_p (w, posint, &x, &y, &rtop, &rbot, &rowh, &vpos)
(gdb) 
1458	      && (fully_p = !rtop && !rbot, (!NILP (partially) || fully_p)))
(gdb) 
1461	  if (!NILP (in_window) && !NILP (partially))
(gdb) xprintsym in_window
"nil"(gdb) p fully_p
$1 = 0
(gdb) xprintsym partially
"nil"(gdb) n
1471	  return in_window;
(gdb) 

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: mwd@cert.org,  11464@debbugs.gnu.org
> Date: Fri, 18 May 2012 13:47:26 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > And if that happens, the correction code below, viz.:
> >
> >       if (bottom_y <= it.last_visible_y
> > 	  && it.bidi_p && it.bidi_it.scan_dir == -1
> > 	  && IT_CHARPOS (it) < charpos)
> >
> > would evaluate to false, and visible_p would have stayed at its 1
> > value, which is wrong.
> >
> > So could you please clarify this case?
> 
> Yes, pos_visible_p indeed returns incorrect value (1), but it is called
> from Fpos_visible_in_window_p, which returns the correct value.  For
> longer description, see below.

OK, but pos_visible_p is called from other places, and needs to
produce a correct result by itself.

> > Also, what was the value of top_y in your first case, i.e.:
> >
> >> bottom_y = 300
> >> it.last_visible_y = 304
> 
> That case has the following values now (in emacs-24 rev. 108005):
> 
> bottom_y = 302
> it.last_visible_y = 304
> top_y = 285

Very strange, for both 300 and 302.  These values seem to imply that
the screen line where we wind up spans pixel coordinates [285..302],
which means move_it_to didn't get to its goal coordinate 303.

What are the values of it.max_ascent and it.max_descent in this case,
at the point where the move_it_to call on line 1287 returns?

> I'm not sure why bottom_y has changed its value.  I guess that is
> because I installed some new fonts.

That's okay, they both puzzle me alike.

Thanks.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

> Very strange, for both 300 and 302.  These values seem to imply that
> the screen line where we wind up spans pixel coordinates [285..302],
> which means move_it_to didn't get to its goal coordinate 303.
>
> What are the values of it.max_ascent and it.max_descent in this case,
> at the point where the move_it_to call on line 1287 returns?
>

it.max_ascent = 14
it.max_descent = 3

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: mwd@cert.org,  11464@debbugs.gnu.org
> Date: Fri, 18 May 2012 14:47:23 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Very strange, for both 300 and 302.  These values seem to imply that
> > the screen line where we wind up spans pixel coordinates [285..302],
> > which means move_it_to didn't get to its goal coordinate 303.
> >
> > What are the values of it.max_ascent and it.max_descent in this case,
> > at the point where the move_it_to call on line 1287 returns?
> >
> 
> it.max_ascent = 14
> it.max_descent = 3

Thanks.  This is consistent with top_y and bottom_y, but leaves open
the question why move_it_to didn't advance one more line.  I will try
to reproduce this on my system before I bother you with more
questions.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  This is consistent with top_y and bottom_y, but leaves open
> the question why move_it_to didn't advance one more line.  I will try
> to reproduce this on my system before I bother you with more
> questions.

In case it helps, I found out this:

Using emacs-24 rev. 107994 (your original patch):

./emacs -Q -fn "DejaVu Sans Mono-10" -l bug.el
=> "Should be nil: nil"

./emacs -Q -fn "DejaVu Sans Mono-12" -l bug.el
=> "Should be nil: t"

In both cases, the latin letters use the given font, and the Hebrew
letters are displayed with corresponding "Arimo" font:

    xft:-unknown-DejaVu Sans Mono-normal-normal-normal-*-16-*-*-*-m-0-iso10646-1 (#x44)
    xft:-unknown-Arimo-normal-normal-normal-*-16-*-*-*-*-0-iso10646-1 (#x9BD)

-- 
Ari Roponen

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: mwd@cert.org,  11464@debbugs.gnu.org
> Date: Fri, 18 May 2012 17:39:39 +0300
> 
> In case it helps, I found out this:
> 
> Using emacs-24 rev. 107994 (your original patch):
> 
> ./emacs -Q -fn "DejaVu Sans Mono-10" -l bug.el
> => "Should be nil: nil"
> 
> ./emacs -Q -fn "DejaVu Sans Mono-12" -l bug.el
> => "Should be nil: t"
> 
> In both cases, the latin letters use the given font, and the Hebrew
> letters are displayed with corresponding "Arimo" font:
> 
>     xft:-unknown-DejaVu Sans Mono-normal-normal-normal-*-16-*-*-*-m-0-iso10646-1 (#x44)
>     xft:-unknown-Arimo-normal-normal-normal-*-16-*-*-*-*-0-iso10646-1 (#x9BD)

Yes, this helps.  Thanks.

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Eli Zaretskii]

> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,FREEMAIL_FROM,
> 	RCVD_IN_DNSWL_LOW,T_DKIM_INVALID autolearn=ham version=3.3.2
> From: Ari Roponen <ari.roponen@gmail.com>
> Cc: mwd@cert.org,  11464@debbugs.gnu.org
> Date: Fri, 18 May 2012 17:39:39 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks.  This is consistent with top_y and bottom_y, but leaves open
> > the question why move_it_to didn't advance one more line.  I will try
> > to reproduce this on my system before I bother you with more
> > questions.
> 
> In case it helps, I found out this:
> 
> Using emacs-24 rev. 107994 (your original patch):
> 
> ./emacs -Q -fn "DejaVu Sans Mono-10" -l bug.el
> => "Should be nil: nil"
> 
> ./emacs -Q -fn "DejaVu Sans Mono-12" -l bug.el
> => "Should be nil: t"
> 
> In both cases, the latin letters use the given font, and the Hebrew
> letters are displayed with corresponding "Arimo" font:
> 
>     xft:-unknown-DejaVu Sans Mono-normal-normal-normal-*-16-*-*-*-m-0-iso10646-1 (#x44)
>     xft:-unknown-Arimo-normal-normal-normal-*-16-*-*-*-*-0-iso10646-1 (#x9BD)

I couldn't find an Arimo font that supports Hebrew.  However, I found
several fonts already installed on my system with which I could
reproduce all of your 3 cases.

It turned out there was a subtle misfeature in move_it_to, which
pos_visible_p calls, that would produce a situation where bottom_y
computed in pos_visible_p could be less that it.last_visible_y.  That
misfeature caused the computation of bottom_y produce inaccurate
result.  By fixing that misfeature (see below), I was able to revert
the comparison against bottom_y to what it originally was, and get
correct results from pos_visible_p in all the cases I tried.

In general, if move_it_to stops at the bottom of the window, the last
line's bottom_y cannot be less than last_visible_y, because that means
there's one more line (partially) visible in the window.  So the
comparison I originally installed is the correct one.

I installed the patch below as revision 108008 on the emacs-24
branch.  Please test.

Ari and Michael, many thanks for your help in working on this tricky
problem.


=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-05-15 16:17:42 +0000
+++ src/ChangeLog	2012-05-19 12:14:11 +0000
@@ -1,3 +1,13 @@
+2012-05-19  Eli Zaretskii  <eliz@gnu.org>
+
+	* xdisp.c (move_it_to): Under MOVE_TO_Y, when restoring iterator
+	state after an additional call to move_it_in_display_line_to, keep
+	the values of it->max_ascent and it->max_descent found for the
+	entire line.
+	(pos_visible_p): Revert the comparison against bottom_y to what it
+	was in revid eliz@gnu.org-20120513182235-4p6386j761ld0nwb.
+	(Bug#11464)
+
 2012-05-15  Eli Zaretskii  <eliz@gnu.org>
 
 	* xdisp.c (pos_visible_p): Fix last change.  (Bug#11464)

=== modified file 'src/xdisp.c'
--- src/xdisp.c	2012-05-15 16:17:42 +0000
+++ src/xdisp.c	2012-05-19 12:14:11 +0000
@@ -1313,7 +1313,7 @@ pos_visible_p (struct window *w, EMACS_I
 	visible_p = bottom_y > window_top_y;
       else if (top_y < it.last_visible_y)
 	visible_p = 1;
-      if (bottom_y <= it.last_visible_y
+      if (bottom_y >= it.last_visible_y
 	  && it.bidi_p && it.bidi_it.scan_dir == -1
 	  && IT_CHARPOS (it) < charpos)
 	{
@@ -8689,8 +8689,18 @@ move_it_to (struct it *it, EMACS_INT to_
 		{
 		  /* If TO_Y is in this line and TO_X was reached
 		     above, we scanned too far.  We have to restore
-		     IT's settings to the ones before skipping.  */
+		     IT's settings to the ones before skipping.  But
+		     keep the more accurate values of max_ascent and
+		     max_descent we've found while skipping the rest
+		     of the line, for the sake of callers, such as
+		     pos_visible_p, that need to know the line
+		     height.  */
+		  int max_ascent = it->max_ascent;
+		  int max_descent = it->max_descent;
+
 		  RESTORE_IT (it, &it_backup, backup_data);
+		  it->max_ascent = max_ascent;
+		  it->max_descent = max_descent;
 		  reached = 6;
 		}
 	      else

bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text

[Ari Roponen]

Eli Zaretskii <eliz@gnu.org> writes:

> I installed the patch below as revision 108008 on the emacs-24
> branch.  Please test.
>

I just tested this with all the cases I had, and they worked correctly.

Thank you very much!

-- 
Ari Roponen