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