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

[Ari Roponen <ari.roponen@gmail.com>]

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