bug#56393: Actually fix the long lines display bug

[Eli Zaretskii]

> Date: Tue, 05 Jul 2022 10:17:37 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> Gosh!  After including too little, I included too much.  The attached one 
> is correct (I hope).

Thanks!  It's a good idea, but I wonder how far we can safely take it;
see below.

> diff --git a/doc/emacs/trouble.texi b/doc/emacs/trouble.texi
> index f06b93759d..887e5c6170 100644
> --- a/doc/emacs/trouble.texi
> +++ b/doc/emacs/trouble.texi
> @@ -158,7 +158,6 @@ Lossage
>  * Crashing::              What Emacs does when it crashes.
>  * After a Crash::         Recovering editing in an Emacs session that 
> crashed.
>  * Emergency Escape::      What to do if Emacs stops responding.
> -* Long Lines::            Mitigating slowness due to extremely long lines.
>  * DEL Does Not Delete::   What to do if @key{DEL} doesn't delete.
>  @end menu
>  
> @@ -433,64 +432,6 @@ Emergency Escape
>  emergency escape---but there are cases where it won't work, when a
>  system call hangs or when Emacs is stuck in a tight loop in C code.
>  
> -@node Long Lines
> -@subsection Long Lines
> -@cindex long lines

This removal is premature, IMO.  We should first establish that this
feature can indeed cope well enough with the important use cases, and
I'm not sure we are there yet.  It is possible that this feature will
be one of several available features that attempt to solve the same
problem in different ways, each way with its own advantages and
disadvantages.

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -324,9 +324,6 @@ startup.  Previously, these functions ignored
>  
>  * Changes in Emacs 29.1
>  
> ----
> -** 'longlines-mode' is no longer obsolete.
> -

Likewise here.

> +** Emacs is now capable of editing files with arbitarily long lines.

I'd suggest saying something like

  Emacs now copes better with editing files with extremely long lines.

> +(defcustom auto-narrow-display-length 30000
> +  "Number of characters to which display is restricted in 
> `auto-narrow-mode'."

This should tell how the narrowed region is positioned relative to
point, I think, because without it the doc string tells too little.

> +(defcustom auto-narrow-widen-automatically
> +  '( move-beginning-of-line move-end-of-line backward-sentence 
> forward-sentence
> +     backward-sexp forward-sexp beginning-of-defun end-of-defun
> +     beginning-of-buffer end-of-buffer goto-char goto-line
> +     mark-sexp mark-defun mark-paragraph mark-whole-buffer mark-page
> +     exchange-point-and-mark pop-global-mark set-mark-command 
> jump-to-register
> +     bookmark-jump)
> +  "Commands for which display is automatically widened in 
> `auto-narrow-mode'."

What are the criteria for this default?  (I'm surprised the list is so
short, for the reasons I explain below.)

> +(setq auto-narrow-long-line-threshold 30000
> +      auto-narrow-pre-command-function #'auto-narrow-pre-command-function
> +      auto-narrow-post-command-function #'auto-narrow-post-command-function)

We might eventually decide to have this turned off by default.

> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -4997,6 +4997,20 @@ because (1) it preserves some marker positions (in 
> unchanged portions
>          Otherwise start with an empty undo_list.  */
>       bset_undo_list (current_buffer, EQ (old_undo, Qt) ? Qt : Qnil);
>  
> +      if (!noninteractive && !NILP (Vauto_narrow_long_line_threshold))
> +     {
> +       ptrdiff_t cur, next, found, max = 0;
> +       for (cur = PT; cur < PT + inserted; cur = next)
> +         {
> +           next = find_newline1 (cur, CHAR_TO_BYTE (cur), 0, -1, 1,
> +                                 &found, NULL, true);
> +           if (next - cur > max) max = next - cur;
> +           if (!found) break;
> +         }
> +       if (max > XFIXNUM (Vauto_narrow_long_line_threshold))
> +         safe_call (1, Qauto_narrow_mode);
> +     }
> +

This will invoke auto-narrow-mode for any file that is for some reason
inserted by Emacs into some buffer.  Many Emacs commands insert files
into temporary buffers or buffers that are never meant for display,
and AFAIU the above will narrow such a buffer if a file inserted
happens to have long lines, is that correct?  If so, a Lisp program
that inserts such a file, and then processes it, might fail to do its
job because it will bump into the auto-narrowed restriction.  I think
this is a symptom of a larger problem with this approach; more about
it below.

> +  DEFVAR_LISP ("auto-narrow-long-line-threshold",
> +            Vauto_narrow_long_line_threshold,
> +            doc: /* Line length above which `auto-narrow-mode' is entered.
> +When non-nil, and when a file with one or more lines longer than
> +`auto-narrow-long-line-threshold' is opened or inserted in a buffer,
> +`auto-narrow-mode' is automatically enabled.
> +When nil, `auto-narrow-mode' is not entered automatically.  */);
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  "When nil, `auto-narrow-mode' is disabled."

> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -1290,6 +1290,9 @@ command_loop_1 (void)
>  
>    if (NILP (Vmemory_full))
>      {
> +      if (!NILP (Vauto_narrow_post_command_function) && !NILP (Vrun_hooks))
> +     safe_run_hooks (Qauto_narrow_post_command_function);
> +
>        /* Make sure this hook runs after commands that get errors and
>        throw to top level.  */
> [...]
> @@ -1522,6 +1527,8 @@ command_loop_1 (void)
>            }
>        kset_last_prefix_arg (current_kboard, Vcurrent_prefix_arg);
>  
> +      safe_run_hooks (Qauto_narrow_post_command_function);
> +
>        safe_run_hooks (Qpost_command_hook);

AFAIU, this means post-command-hook functions will run with the
current buffer narrowed, right?  That could cause them to fail, if
they for some reason want to access the buffer portions outside the
restriction.

> @@ -1472,6 +1475,8 @@ command_loop_1 (void)
>        Vreal_this_command = cmd;
>        safe_run_hooks (Qpre_command_hook);
>  
> +      safe_run_hooks (Qauto_narrow_pre_command_function);
> +

And here's a similar problem with pre-command-hook functions.

A similar problem will happen with any Lisp that is run from the
redisplay code: it will only see the narrowed region.

I don't quite see clearly what these general issues could mean, but
they are at least worrisome, I think, because they could potentially
mean significant breakage in many places.  The simple measure of
having a list of commands that automatically widen the buffer is
insufficient, I'm afraid, because the real problem is not on the
command level, it is on the level of Lisp programs which say
save-excursion and then go far away for whatever purposes.  Also for
commands written by users: for example, if someone writes a command
that is a trivial wrapper around goto-line, that command will no
longer automatically widen as goto-line does, right?

Can we come up with a better solution for these potential problems?
Alternatively, maybe you will explain that I'm bothered by a
non-existent problem?

Other observations:

Since the narrowing is basically in effect only during redisplay, it
doesn't help with commands that use display code outside of redisplay
proper.  For example, C-n, C-v, C-l, and other commands are still
extremely sluggish in files with long lines.  E.g., try the file
long-line.xml mentioned here:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=45898#80

Or try dragging the mode line of a window showing that file.

Also, the automatic restriction is very visible and causes surprising
effects: (point-min) and (point-max) evaluate to the narrowed region
(so it's basically very hard to know the real size of the buffer), and
scrolling through the buffer causes the scroll-bar thumb move in a
non-monotonic manner: I scroll towards the end of the buffer, but the
thumb sometimes jumps back up, as if I moved towards the beginning.

And sometimes I see that C-n moves point horizontally, i.e. point
stays on the same screen line, instead of the expected vertical
movement.  I guess this is because the restriction violates some
assumptions that Emacs makes regarding horizontal coordinates of some
reference position.

I think at this point it is best to have this feature on a feature
branch, so that people could try it, and so that improving it won't
need to post the entire patch each time anew.

Thanks.