[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#30205: 27.0.50; Minor mode commands enable the minor mode even if th
From: |
Basil L. Contovounesios |
Subject: |
bug#30205: 27.0.50; Minor mode commands enable the minor mode even if the body fails |
Date: |
Thu, 18 Jul 2019 22:40:11 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) |
Lars Ingebrigtsen <larsi@gnus.org> writes:
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
>> In *scratch*:
>>
>> (define-minor-mode foo-mode nil nil nil nil (error "what"))
>>
>> C-h v foo-mode shows that foo-mode is nil, as it should be.
>> But after M-x foo-mode, it is t, even though the mode command failed.
>> This can be confusing for mode commands that can conditionally fail,
>> e.g. depending on some external property.
>
> Hm... OK, this is the function run when saying M-x foo-mode:
>
> (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
> ;; The on/off hooks are here for backward compatibility only.
> (run-hooks ',hook (if ,getter ',hook-on ',hook-off))
>
> So `setter' is the thing that sets the mode variable, and then body is
> run.
>
> I agree with you that it would be better that the mode variable remains
> unchanged if `body' fails. But I `body' is likely to need to have that
> set to work. We could roll back the value to the previous value on
> errors?
>
> Hm. On the other hand, if `body' has done most of the stuff it needs to
> do and fails "late" in the process, then the mode will be in effect even
> if it failed, and in that case it would be wrong to roll back.
>
> So I don't know. Does anybody have an opinion?
Unless we can provide a way to roll back the entire mode function
"transaction", I would regard errors in mode functions as UB and the
real bug here. In other words, I don't see any benefit to using the
mode variable as an indicator of success, when mode functions shouldn't
fail to begin with.
--
Basil