bug#43730: 27.1; Running (visual-line-mode 1) twice

bug#43730: 27.1; Running (visual-line-mode 1) twice

[Miha Rihtaršič]

Running (visual-line-mode 1) twice is inconsistent due to variable
visual-line--saved-state being set twice.

A real life example, producible with emacs -Q would be to run

(setq-default truncate-lines t)
(add-hook 'text-mode-hook 'visual-line-mode)
(add-hook 'prog-mode-hook 'visual-line-mode)

and visit a buffer in mhtml-mode, which runs both text-mode-hook and
prog-mode-hook.
visual-line-mode sets the local value of truncate-lines to nil as
expected but turning it off with
M-x visual-line-mode
fails to restore truncate-lines back to t, due to incorrect
visual-line--saved-state.

(My current work-around is to add
(lambda ()
  (unless visual-line-mode (visual-line-mode)))
to the hooks.)

Other minor modes relying on a '-saved-state' variable are possibly
affected as well. A quick grep lead me to:
refill-mode and cua-mode.


In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3)
 of 2020-08-28 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Arch Linux

Recent messages:
Indenting region...done
Visual-Line mode disabled in current buffer
Mark saved where search started
Mark set
Replaced 7 occurrences
Mark set [3 times]
Undo
Mark set
Comint exited abnormally with code 1
Auto-saving...

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-wide-int
 --with-modules --with-cairo --with-harfbuzz 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON
PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_US.utf8
  locale-coding-system: utf-8-unix

Memory information:
((conses 16 852933 497311)
 (symbols 48 45748 2)
 (strings 32 245922 68759)
 (string-bytes 1 7871697)
 (vectors 16 80403)
 (vector-slots 8 1800671 524204)
 (floats 8 724 2716)
 (intervals 56 43104 32951)
 (buffers 1000 27))

bug#43730: 27.1; Running (visual-line-mode 1) twice

[Lars Ingebrigtsen]

Miha Rihtaršič <miha@kamnitnik.top> writes:

> Running (visual-line-mode 1) twice is inconsistent due to variable
> visual-line--saved-state being set twice.
>
> A real life example, producible with emacs -Q would be to run
>
> (setq-default truncate-lines t)
> (add-hook 'text-mode-hook 'visual-line-mode)
> (add-hook 'prog-mode-hook 'visual-line-mode)
>
> and visit a buffer in mhtml-mode, which runs both text-mode-hook and
> prog-mode-hook.
> visual-line-mode sets the local value of truncate-lines to nil as
> expected but turning it off with
> M-x visual-line-mode
> fails to restore truncate-lines back to t, due to incorrect
> visual-line--saved-state.

Thanks for the analysis.  It seems to me that the way to fix this
problem would be to do something general in define-minor-mode.  It has
always confused me that the resulting mode function starts like this:

       (defun ,modefun (&optional arg ,@extra-args)
         ,(easy-mmode--mode-docstring doc pretty-name keymap-sym)
	 ;; Use `toggle' rather than (if ,mode 0 1) so that using
	 ;; repeat-command still does the toggling correctly.
	 (interactive (list (or current-prefix-arg 'toggle)))
	 (let ((,last-message (current-message)))
           (,@setter
            (if (eq arg 'toggle)
                (not ,getter)
              ;; A nil argument also means ON now.
              (> (prefix-numeric-value arg) 0)))
           ,@body

This means that all the modes basically have bodies that start like this:

  (if visual-line-mode
      (progn

And the modes do not know whether the mode was already on, or whether
we're switching it on now, because the only clue it has is the value of
visual-line-mode, and that has just been set based on ARG.

So.  One really aggressive way to try to fix this would be for
define-minor-mode to just not call the body at all if the value of the
mode variable doesn't change.  That is, the second call here would do
absolutely nothing:

(visual-line-mode 1)
(visual-line-mode 1)

I think that would be logical change, but...  it's a pretty radical
change?  Who knows what it would affect?

A much less invasive change would be to bind a variable in the defun
there, like

	 (let ((,previous-state ,modevar))
            
and then modes like visual-line-mode could go

  (when (not (eq visual-line-mode previous-state))
    (if visual-line-mode
        (progn

Any opinions?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43730: 27.1; Running (visual-line-mode 1) twice

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Any opinions?

(And the least invasive change would just be to have visual-line-mode
check visual-line--saved-state before doing anything, with the
equivalent fixes in the other two modes.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43730: 27.1; Running (visual-line-mode 1) twice

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Any opinions?
>
> (And the least invasive change would just be to have visual-line-mode
> check visual-line--saved-state before doing anything, with the
> equivalent fixes in the other two modes.)

I've now done the latter in visual-line-mode and cua-mode.  refill-mode
seems to be doing this OK already.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no