* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text
@ 2012-05-13 15:54 Ari Roponen
2012-05-13 18:26 ` Eli Zaretskii
0 siblings, 1 reply; 24+ messages in thread
From: Ari Roponen @ 2012-05-13 15:54 UTC (permalink / raw)
To: 11464
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
^ permalink raw reply [flat|nested] 24+ messages in thread* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-13 15:54 bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text Ari Roponen @ 2012-05-13 18:26 ` Eli Zaretskii 2012-05-15 10:07 ` Ari Roponen 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-13 18:26 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464-done > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-13 18:26 ` Eli Zaretskii @ 2012-05-15 10:07 ` Ari Roponen 2012-05-15 16:19 ` Eli Zaretskii 0 siblings, 1 reply; 24+ messages in thread From: Ari Roponen @ 2012-05-15 10:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-15 10:07 ` Ari Roponen @ 2012-05-15 16:19 ` Eli Zaretskii 2012-05-16 5:21 ` Ari Roponen 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-15 16:19 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464 > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-15 16:19 ` Eli Zaretskii @ 2012-05-16 5:21 ` Ari Roponen 2012-05-16 15:15 ` Eli Zaretskii 0 siblings, 1 reply; 24+ messages in thread From: Ari Roponen @ 2012-05-16 5:21 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-16 5:21 ` Ari Roponen @ 2012-05-16 15:15 ` Eli Zaretskii 2012-05-17 4:52 ` Ari Roponen 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-16 15:15 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464 > 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? ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-16 15:15 ` Eli Zaretskii @ 2012-05-17 4:52 ` Ari Roponen 2012-05-17 16:23 ` Eli Zaretskii 0 siblings, 1 reply; 24+ messages in thread From: Ari Roponen @ 2012-05-17 4:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-17 4:52 ` Ari Roponen @ 2012-05-17 16:23 ` Eli Zaretskii 2012-05-17 17:54 ` Ari Roponen 2012-05-17 17:56 ` Michael Welsh Duggan 0 siblings, 2 replies; 24+ messages in thread From: Eli Zaretskii @ 2012-05-17 16:23 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464 > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-17 16:23 ` Eli Zaretskii @ 2012-05-17 17:54 ` Ari Roponen 2012-05-17 17:56 ` Michael Welsh Duggan 1 sibling, 0 replies; 24+ messages in thread From: Ari Roponen @ 2012-05-17 17:54 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-17 16:23 ` Eli Zaretskii 2012-05-17 17:54 ` Ari Roponen @ 2012-05-17 17:56 ` Michael Welsh Duggan 2012-05-17 21:11 ` Eli Zaretskii 1 sibling, 1 reply; 24+ messages in thread From: Michael Welsh Duggan @ 2012-05-17 17:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Ari Roponen, 11464 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) ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-17 17:56 ` Michael Welsh Duggan @ 2012-05-17 21:11 ` Eli Zaretskii 2012-05-17 21:22 ` Michael Welsh Duggan 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-17 21:11 UTC (permalink / raw) To: Michael Welsh Duggan; +Cc: ari.roponen, 11464 > 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 <=. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-17 21:11 ` Eli Zaretskii @ 2012-05-17 21:22 ` Michael Welsh Duggan 2012-05-18 7:42 ` Eli Zaretskii 0 siblings, 1 reply; 24+ messages in thread From: Michael Welsh Duggan @ 2012-05-17 21:22 UTC (permalink / raw) To: Eli Zaretskii; +Cc: ari.roponen, 11464 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) ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-17 21:22 ` Michael Welsh Duggan @ 2012-05-18 7:42 ` Eli Zaretskii 2012-05-18 8:03 ` Ari Roponen 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-18 7:42 UTC (permalink / raw) To: Michael Welsh Duggan; +Cc: ari.roponen, 11464 > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 7:42 ` Eli Zaretskii @ 2012-05-18 8:03 ` Ari Roponen 2012-05-18 8:26 ` Ari Roponen 2012-05-18 8:44 ` Eli Zaretskii 0 siblings, 2 replies; 24+ messages in thread From: Ari Roponen @ 2012-05-18 8:03 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464, Michael Welsh Duggan 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 8:03 ` Ari Roponen @ 2012-05-18 8:26 ` Ari Roponen 2012-05-18 8:44 ` Eli Zaretskii 1 sibling, 0 replies; 24+ messages in thread From: Ari Roponen @ 2012-05-18 8:26 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464, Michael Welsh Duggan 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 8:03 ` Ari Roponen 2012-05-18 8:26 ` Ari Roponen @ 2012-05-18 8:44 ` Eli Zaretskii 2012-05-18 10:47 ` Ari Roponen 1 sibling, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-18 8:44 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464, mwd > 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! ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 8:44 ` Eli Zaretskii @ 2012-05-18 10:47 ` Ari Roponen 2012-05-18 11:32 ` Eli Zaretskii 0 siblings, 1 reply; 24+ messages in thread From: Ari Roponen @ 2012-05-18 10:47 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464, mwd 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 10:47 ` Ari Roponen @ 2012-05-18 11:32 ` Eli Zaretskii 2012-05-18 11:47 ` Ari Roponen 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-18 11:32 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464, mwd > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 11:32 ` Eli Zaretskii @ 2012-05-18 11:47 ` Ari Roponen 2012-05-18 14:02 ` Eli Zaretskii 0 siblings, 1 reply; 24+ messages in thread From: Ari Roponen @ 2012-05-18 11:47 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464, mwd 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 11:47 ` Ari Roponen @ 2012-05-18 14:02 ` Eli Zaretskii 2012-05-18 14:39 ` Ari Roponen 0 siblings, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-18 14:02 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464, mwd > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 14:02 ` Eli Zaretskii @ 2012-05-18 14:39 ` Ari Roponen 2012-05-18 14:59 ` Eli Zaretskii 2012-05-19 12:26 ` Eli Zaretskii 0 siblings, 2 replies; 24+ messages in thread From: Ari Roponen @ 2012-05-18 14:39 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464, mwd 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 14:39 ` Ari Roponen @ 2012-05-18 14:59 ` Eli Zaretskii 2012-05-19 12:26 ` Eli Zaretskii 1 sibling, 0 replies; 24+ messages in thread From: Eli Zaretskii @ 2012-05-18 14:59 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464, mwd > 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-18 14:39 ` Ari Roponen 2012-05-18 14:59 ` Eli Zaretskii @ 2012-05-19 12:26 ` Eli Zaretskii 2012-05-19 12:32 ` Ari Roponen 1 sibling, 1 reply; 24+ messages in thread From: Eli Zaretskii @ 2012-05-19 12:26 UTC (permalink / raw) To: Ari Roponen; +Cc: 11464, mwd > 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text 2012-05-19 12:26 ` Eli Zaretskii @ 2012-05-19 12:32 ` Ari Roponen 0 siblings, 0 replies; 24+ messages in thread From: Ari Roponen @ 2012-05-19 12:32 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 11464, mwd 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2012-05-19 12:32 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-13 15:54 bug#11464: 24.1.50; pos-visible-in-window-p returns a false positive with bidi text Ari Roponen 2012-05-13 18:26 ` Eli Zaretskii 2012-05-15 10:07 ` Ari Roponen 2012-05-15 16:19 ` Eli Zaretskii 2012-05-16 5:21 ` Ari Roponen 2012-05-16 15:15 ` Eli Zaretskii 2012-05-17 4:52 ` Ari Roponen 2012-05-17 16:23 ` Eli Zaretskii 2012-05-17 17:54 ` Ari Roponen 2012-05-17 17:56 ` Michael Welsh Duggan 2012-05-17 21:11 ` Eli Zaretskii 2012-05-17 21:22 ` Michael Welsh Duggan 2012-05-18 7:42 ` Eli Zaretskii 2012-05-18 8:03 ` Ari Roponen 2012-05-18 8:26 ` Ari Roponen 2012-05-18 8:44 ` Eli Zaretskii 2012-05-18 10:47 ` Ari Roponen 2012-05-18 11:32 ` Eli Zaretskii 2012-05-18 11:47 ` Ari Roponen 2012-05-18 14:02 ` Eli Zaretskii 2012-05-18 14:39 ` Ari Roponen 2012-05-18 14:59 ` Eli Zaretskii 2012-05-19 12:26 ` Eli Zaretskii 2012-05-19 12:32 ` Ari Roponen
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).