Re: Redisplay hook error bactraces [Was: Fontification error backtrace]

[Eli Zaretskii <eliz@gnu.org>]

> Date: Tue, 12 Jul 2022 19:48:49 +0000
> Cc: larsi@gnus.org, Stefan Monnier <monnier@iro.umontreal.ca>,
>   emacs-devel@gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> To use it, set the variable backtrace-on-redisplay-error to t.

There's no such variable in the patch.

> Optionally, configure redisplay-last-error, which determines which
> backtrace(s) to keep, should a command cause more than one.

Why do we need two variables for that? why not a single variable that
is not a boolean?

> > > OK, I've never used it.  On reading the documentation, I expected it to
> > > wait until after the next command.
> 
> > That's because the ELisp reference manual describes it from the POV of
> > setting up the warning as part of some command.  And even then, it
> > says:
> 
> >      The value of this variable is a list of warnings to be displayed
> >      after the current command has finished.
> 
> > "Current command", not "next command".
> 
> > If you add a warning to the list inside redisplay, it will pop up
> > after redisplay returns.
> 
> I've tried to use it, but the list of warnings does indeed wait until
> after the next command before being displayed.

I think it's a terminology issue: how do you define "next command" in
this context?

I'm telling you: we have code in the display engine that uses delayed
warnings, and the warnings pop up right away, not after one more
command.

> > > Now I remember why I created the backtrace in signal_or_quit - it needs
> > > to be done before the stack gets unwound, which happens later in
> > > signal_or_quit.  On return to save_eval_handler is just too late for
> > > this.
> 
> > That's orthogonal, I think?  You can collect the data inside
> > signal_or_quit, and the signal handler then only needs to handle the
> > error gracefully after it adds the warning.
> 
> The "handling" of the error is to allow the redisplay to continue, just
> as though no backtrace were generated.  Since there's no possibility of
> user interaction or a recursive redisplay, that should be OK.

I don't understand this response.  It seems to be unrelated to what I
wrote.

> > > Then the mechanism I've implemented, namely to set redisplay_deep_handler
> > > to the handler which would handle an error, could be made to serve for
> > > other sorts of Lisp code.
> 
> > It could, but I see no reason for having a new handler.
> 
> Sorry, I didn't express that well.  That variable redisplay_deep_handler
> merely records the handler that a condition-case might use.  In
> signal_or_quit, if the handler about to be used matches
> redisplay_deep_handler, then a backtrace gets generated.

??? You use a handler as a flag?  Then why not just use a simple flag
variable?

> +  /* If an error is signalled during a Lisp hook in redisplay, write a
> +     backtrace into the buffer *Backtrace*.  */
> +  if (!debugger_called && !NILP (error_symbol)
> +      && redisplay_lisping
> +      && backtrace_on_redisplay_lisp_error
> +      && (!backtrace_yet || !NILP (Vredisplay_last_error))
> +      && (NILP (clause) || h == redisplay_deep_handler)
> +      && NILP (Vinhibit_debugger)
> +      && !NILP (Ffboundp (Qdebug_early)))

Why do we need so many conditions and flags?  Why do we need a new
variable redisplay_lisping, when we already have redisplaying_p?  Why
do you need to compare both redisplay_deep_handler and
backtrace_on_redisplay_lisp_error?  And what is the test of
backtrace_yet about?

I still hope this could be done more elegantly and with fewer changes
to infrastructure.

Thanks.