defadvice bug or something else?

defadvice bug or something else?

[Tim Cross]

Hi All,

I've been tracking down a bug in an application which makes extensive
use of defadvice and found something which seems inconsistent. I'm not
sure if this is a bug or some misunderstanding on my part.

The problem is, I'm getting different values for window-end calls when
they occur as part of an after advice compared to when they are called
ouside of any defadvice. the value returned when called within defadvice
is incorrect and does not represent the position at the end of the
window.

I used the following defadvice and defun to verify this behaviour

(require 'cl-lib)
(require 'advice)

(cl-loop
 for f in
 '(scroll-up scroll-down
             scroll-up-command scroll-down-command)
 do
 (eval
  `(defadvice ,f (after emacspeak pre act comp)
     (message "scroll advice: start = %d end = %d diff = %d"
              (window-start)
              (window-end)
              (- (window-end) (window-start))))))

(defun tx-window ()
  (interactive)
  (message "tx-window: start = %d end = %d diff = %d"
           (window-start)
           (window-end)
           (- (window-end) (window-start))))

Using a test file of data which is multiple screenfuls in size, I open
the file, move point to the start of the buffer, scroll down with either
C-v or pgdown, run M-x tx-window. I did this twice and then did the
opposite, scrolling back two windows to be back at the beginning of the
buffer, executing tx-window after each scroll. The output I get is

scroll advice: start = 1259 end = 1536 diff = 277 
tx-window: start = 1259 end = 2863 diff = 1604
scroll advice: start = 2662 end = 2863 diff = 201 
tx-window: start = 2662 end = 4069 diff = 1407
scroll advice: start = 1259 end = 4069 diff = 2810 
tx-window: start = 1259 end = 2863 diff = 1604
scroll advice: start = 1 end = 2863 diff = 2862 
tx-window: start = 1 end = 1536 diff = 1535

I would expect the output from 'scroll advice' and 'tx-window' to be the
same. Note the end value (from window-end call). The value returned from
within 'scroll advice' is incorrect. It is either way too small after
C-v/pgdown or way too large after M-v/pgup. The value from tx-window is
correct.

This is with Emacs 27.2 built from git repo yesterday on Ubuntu 20.10,
but I think the same issue exists in current master. I started with
emacs -Q and loaded the above test code.

Does this look like a bug or is there something I've overlooked or
misunderstood?

thanks,

Tim

-- 
Tim Cross

Re: defadvice bug or something else?

[Stefan Monnier]

>   `(defadvice ,f (after emacspeak pre act comp)
>      (message "scroll advice: start = %d end = %d diff = %d"
>               (window-start)
>               (window-end)
>               (- (window-end) (window-start))))))

I think this may return outdated values for `window-end` because it is
run before the window end position has been recomputed by redisplay.
If that's not what you want, then you need to pass non-nil value for the
`update` argument.


        Stefan

Re: defadvice bug or something else?

[T.V Raman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030, Size: 818 bytes --]



Thanks Tim for posting the repro case, and thanks Stefan for the pointer
to the optional 'update arg to window -end -- this fixes the bug in
emacspeak that Tim reported recently.


Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>   `(defadvice ,f (after emacspeak pre act comp)
>>      (message "scroll advice: start = %d end = %d diff = %d"
>>               (window-start)
>>               (window-end)
>>               (- (window-end) (window-start))))))
>
> I think this may return outdated values for `window-end` because it is
> run before the window end position has been recomputed by redisplay.
> If that's not what you want, then you need to pass non-nil value for the
> `update` argument.
>
>
>         Stefan
>
>

-- 

Thanks,

--Raman
7©4 Id: kg:/m/0285kf1  •0Ü8

Re: defadvice bug or something else?

[Tim Cross]

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

On Thu, 29 Apr 2021 at 12:07, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >   `(defadvice ,f (after emacspeak pre act comp)
> >      (message "scroll advice: start = %d end = %d diff = %d"
> >               (window-start)
> >               (window-end)
> >               (- (window-end) (window-start))))))
>
> I think this may return outdated values for `window-end` because it is
> run before the window end position has been recomputed by redisplay.
> If that's not what you want, then you need to pass non-nil value for the
> `update` argument.
>
>
Thanks Stefan, you nailed it. That was exactly the problem. I should have
spotted it when I checked the docs for window-end.

regards,

Tim

--
Tim Cross

[-- Attachment #2: Type: text/html, Size: 1298 bytes --]

Re: defadvice bug or something else?

[dick.r.chiang]

You make it so hard to help you, what with the syntactically erroneous code,
the hand-wavy minimum reproducible example, the verbiage, and the transposition
of up versus down (C-v is scroll-up, not scroll-down).

emacs -Q --eval                                                                   \
"(dolist (f (quote (scroll-up-command scroll-down-command)))                      \
  (add-function                                                                   \
   :after (symbol-function f)                                                     \
   (lambda (&rest _args)                                                          \
     (redisplay)                                                                  \
     (princ (format \"wstart = %d,  wend = %d\n\" (window-start) (window-end))    \
            (function external-debugging-output)))))"                             \
--eval "(save-excursion (apply (function insert) (mapcar (apply-partially         \
           (function format) \"%d\n\") (number-sequence 1 1000))))"               \
--eval "(progn (scroll-up-command) (scroll-up-command) (scroll-down-command)      \
           (scroll-down-command))"                                                \
--eval "(kill-emacs)"

emacs -Q --eval                                                                   \
"(defun tx-window ()                                                              \
   (redisplay)                                                                    \
   (princ (format \"wstart = %d,  wend = %d\n\" (window-start) (window-end))      \
     (function external-debugging-output)))"                                      \
--eval "(save-excursion (apply (function insert) (mapcar (apply-partially         \
           (function format) \"%d\n\") (number-sequence 1 1000))))"               \
--eval "(progn (scroll-up-command) (tx-window) (scroll-up-command) (tx-window)    \
            (scroll-down-command) (tx-window) (scroll-down-command) (tx-window))" \
--eval "(kill-emacs)"

Re: defadvice bug or something else?

[Tim Cross]

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

On Thu, 29 Apr 2021 at 14:11, <dick.r.chiang@gmail.com> wrote:

> You make it so hard to help you, what with the syntactically erroneous
> code,
> the hand-wavy minimum reproducible example, the verbiage, and the
> transposition
> of up versus down (C-v is scroll-up, not scroll-down).
>

I don't believe the code I posted was syntactically erroneous at all. It
might be the older style for defining advice, but it is still syntactically
correct.

As to hand wavy minimal reproducible example, I find it far easier to read
than what you have presented, plus your code seems heavy handed with
multiple calls to force redisplay when all that was actually required was
to add the 'update switch to the call to window-end.

Yes, I did refer to scroll down when talking about both C-v
(scroll-up-command) and pgDown (yes, formally it is called 'next' in Emacs,
but on the keyboard the key is labelled pgDown). However, this is really
just another example of how terminology has changed. At any rate, I'm sure
my 'verbiage' made the intention clear. Besides, the direction was
irrelevant - the issue existed when scrolling in either direction and most
people these days talk about scrolling down when moving to the end and
scrolling up when moving to the beginning.

Should you find any of my future posts as frustrating, you can of course
just ignore them. I really don't. mind

 emacs -Q --eval
       \

> "(dolist (f (quote (scroll-up-command scroll-down-command)))
>         \
>   (add-function
>        \
>    :after (symbol-function f)
>        \
>    (lambda (&rest _args)
>         \
>      (redisplay)
>         \
>      (princ (format \"wstart = %d,  wend = %d\n\" (window-start)
> (window-end))    \
>             (function external-debugging-output)))))"
>        \
> --eval "(save-excursion (apply (function insert) (mapcar (apply-partially
>        \
>            (function format) \"%d\n\") (number-sequence 1 1000))))"
>        \
> --eval "(progn (scroll-up-command) (scroll-up-command)
> (scroll-down-command)      \
>            (scroll-down-command))"
>         \
> --eval "(kill-emacs)"
>
> emacs -Q --eval
>        \
> "(defun tx-window ()
>         \
>    (redisplay)
>         \
>    (princ (format \"wstart = %d,  wend = %d\n\" (window-start)
> (window-end))      \
>      (function external-debugging-output)))"
>         \
> --eval "(save-excursion (apply (function insert) (mapcar (apply-partially
>        \
>            (function format) \"%d\n\") (number-sequence 1 1000))))"
>        \
> --eval "(progn (scroll-up-command) (tx-window) (scroll-up-command)
> (tx-window)    \
>             (scroll-down-command) (tx-window) (scroll-down-command)
> (tx-window))" \
> --eval "(kill-emacs)"
>


-- 
regards,

Tim

--
Tim Cross

[-- Attachment #2: Type: text/html, Size: 4648 bytes --]