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

[Eli Zaretskii <eliz@gnu.org>]

> 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