make pred customable in auto-save-visited-mode

make pred customable in auto-save-visited-mode

[纪秀峰]

When I enable  auto-save-visited-mode and start an ediff merge session 
It always prompt me save "*ediff-merge*” to a file.
 


(define-minor-mode auto-save-visited-mode
  "Toggle automatic saving to file-visiting buffers on or off.
With a prefix argument ARG, enable regular saving of all buffers
visiting a file if ARG is positive, and disable it otherwise.
Unlike `auto-save-mode', this mode will auto-save buffer contents
to the visited files directly and will also run all save-related
hooks.  See Info node `Saving' for details of the save process.

If called from Lisp, enable the mode if ARG is omitted or nil,
and toggle it if ARG is `toggle'."
  :group 'auto-save
  :global t
  (when auto-save--timer (cancel-timer auto-save--timer))
  (setq auto-save--timer
        (when auto-save-visited-mode
          (run-with-idle-timer
           auto-save-visited-interval :repeat
           #'save-some-buffers :no-prompt
           (lambda ()  ;;<————————————how about make this customable
             (not (and buffer-auto-save-file-name
                       auto-save-visited-file-name)))))))

Re: make pred customable in auto-save-visited-mode

[Eli Zaretskii]

> From: 纪秀峰 <jixiuf@qq.com>
> Date: Sat, 31 Mar 2018 00:59:19 +0800
> Feedback-ID: esmtp:qq.com:bgforeign:bgforeign1
> 
> When I enable  auto-save-visited-mode and start an ediff merge session 
> It always prompt me save "*ediff-merge*” to a file.

Why is that a problem?  *ediff-merge* is a buffer where you prepare
the merge, and that merge will eventually be saved to some file,
right?  So it doesn't sound wrong to offer to save it.

>   (setq auto-save--timer
>         (when auto-save-visited-mode
>           (run-with-idle-timer
>            auto-save-visited-interval :repeat
>            #'save-some-buffers :no-prompt
>            (lambda ()  ;;<————————————how about make this customable
>              (not (and buffer-auto-save-file-name
>                        auto-save-visited-file-name)))))))

Letting users customize a non-trivial function is not the best way of
affecting this behavior.  I think you can have what you want by adding
a function to ediff-startup-hook, and in that function set
buffer-auto-save-file-name of the *ediff-merge* buffer to nil.

Re: make pred customable in auto-save-visited-mode

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 31. März 2018 um 10:44 Uhr:

> > From: 纪秀峰 <jixiuf@qq.com>
> > Date: Sat, 31 Mar 2018 00:59:19 +0800
> > Feedback-ID: esmtp:qq.com:bgforeign:bgforeign1
> >
> > When I enable  auto-save-visited-mode and start an ediff merge session
> > It always prompt me save "*ediff-merge*” to a file.
>
> Why is that a problem?  *ediff-merge* is a buffer where you prepare
> the merge, and that merge will eventually be saved to some file,
> right?  So it doesn't sound wrong to offer to save it.
>

However, auto-saving should probably not prompt for filenames. That's at
least the intention of the :noprompt argument here.


>
> >   (setq auto-save--timer
> >         (when auto-save-visited-mode
> >           (run-with-idle-timer
> >            auto-save-visited-interval :repeat
> >            #'save-some-buffers :no-prompt
> >            (lambda ()  ;;<————————————how about make this customable
> >              (not (and buffer-auto-save-file-name
> >                        auto-save-visited-file-name)))))))
>
> Letting users customize a non-trivial function is not the best way of
> affecting this behavior.  I think you can have what you want by adding
> a function to ediff-startup-hook, and in that function set
> buffer-auto-save-file-name of the *ediff-merge* buffer to nil.
>
>
I agree that customization isn't the best solution here. However, I'm
suprised that `save-some-buffers' still prompts even if :noprompt is passed.
The issue might be in `save-some-buffers': Its docstring says "If PRED is
t, then certain non-file buffers will also be considered.", but later it
only checks whether PRED is non-nil:

                    (or
                     (buffer-file-name buffer)
                     (with-current-buffer buffer
                       (or (eq buffer-offer-save 'always)
                           (and pred buffer-offer-save (> (buffer-size)
0)))))

Maybe instead of `pred' here we should say (eq pred t). Alternative, the
predicate lambda should check whether `buffer-file-name' is non-nil.

[-- Attachment #2: Type: text/html, Size: 2963 bytes --]

Re: make pred customable in auto-save-visited-mode

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 1822 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am Sa., 31. März 2018 um
12:23 Uhr:

> Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 31. März 2018 um 10:44 Uhr:
>
>>
>>
>> >   (setq auto-save--timer
>> >         (when auto-save-visited-mode
>> >           (run-with-idle-timer
>> >            auto-save-visited-interval :repeat
>> >            #'save-some-buffers :no-prompt
>> >            (lambda ()  ;;<————————————how about make this customable
>> >              (not (and buffer-auto-save-file-name
>> >                        auto-save-visited-file-name)))))))
>>
>> Letting users customize a non-trivial function is not the best way of
>> affecting this behavior.  I think you can have what you want by adding
>> a function to ediff-startup-hook, and in that function set
>> buffer-auto-save-file-name of the *ediff-merge* buffer to nil.
>>
>>
> I agree that customization isn't the best solution here. However, I'm
> suprised that `save-some-buffers' still prompts even if :noprompt is passed.
> The issue might be in `save-some-buffers': Its docstring says "If PRED is
> t, then certain non-file buffers will also be considered.", but later it
> only checks whether PRED is non-nil:
>
>                     (or
>                      (buffer-file-name buffer)
>                      (with-current-buffer buffer
>                        (or (eq buffer-offer-save 'always)
>                            (and pred buffer-offer-save (> (buffer-size)
> 0)))))
>
> Maybe instead of `pred' here we should say (eq pred t). Alternative, the
> predicate lambda should check whether `buffer-file-name' is non-nil.
>

Probably the second option is better because the first option would prevent
PRED from running for non-file-visiting buffers. Patch attached.

[-- Attachment #1.2: Type: text/html, Size: 2835 bytes --]

[-- Attachment #2: 0001-files.el-auto-save-visited-mode-Don-t-prompt-for-filen.txt --]
[-- Type: text/plain, Size: 1010 bytes --]

From 65efc1bfcfa8a9b14f325df0987bd233ec25edb3 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Sat, 31 Mar 2018 12:26:30 +0200
Subject: [PATCH] * files.el (auto-save-visited-mode): Don't prompt for
 filenames.

---
 lisp/files.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 6a13a9d71a..83cba24336 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -436,8 +436,9 @@ auto-save-visited-mode
            auto-save-visited-interval :repeat
            #'save-some-buffers :no-prompt
            (lambda ()
-             (not (and buffer-auto-save-file-name
-                       auto-save-visited-file-name)))))))
+             (and buffer-file-name
+                  (not (and buffer-auto-save-file-name
+                            auto-save-visited-file-name))))))))
 
 ;; The 'set' part is so we don't get a warning for using this variable
 ;; above, while still catching code that _sets_ the variable to get
-- 
2.16.1

Re: make pred customable in auto-save-visited-mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 31 Mar 2018 10:28:58 +0000
> Cc: 纪秀峰 <jixiuf@qq.com>, emacs-devel@gnu.org
> 
>  I agree that customization isn't the best solution here. However, I'm suprised that `save-some-buffers'
>  still prompts even if :noprompt is passed.
>  The issue might be in `save-some-buffers': Its docstring says "If PRED is t, then certain non-file buffers
>  will also be considered.", but later it only checks whether PRED is non-nil:
> 
>                      (or
>                       (buffer-file-name buffer)
>                       (with-current-buffer buffer
>                         (or (eq buffer-offer-save 'always)
>                             (and pred buffer-offer-save (> (buffer-size) 0)))))
> 
>  Maybe instead of `pred' here we should say (eq pred t). Alternative, the predicate lambda should check
>  whether `buffer-file-name' is non-nil.
> 
> Probably the second option is better because the first option would prevent PRED from running for
> non-file-visiting buffers. Patch attached.

AFAIU, this will unconditionally disable auto-saving in *ediff-merge*
and all the similar buffers, when auto-save-visited-mode is turned
on.  Is that what we want?  And if that's going to be the default, how
are users supposed to override it if they want?

Re: make pred customable in auto-save-visited-mode

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 31. März 2018 um 12:45 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sat, 31 Mar 2018 10:28:58 +0000
> > Cc: 纪秀峰 <jixiuf@qq.com>, emacs-devel@gnu.org
> >
> >  I agree that customization isn't the best solution here. However, I'm
> suprised that `save-some-buffers'
> >  still prompts even if :noprompt is passed.
> >  The issue might be in `save-some-buffers': Its docstring says "If PRED
> is t, then certain non-file buffers
> >  will also be considered.", but later it only checks whether PRED is
> non-nil:
> >
> >                      (or
> >                       (buffer-file-name buffer)
> >                       (with-current-buffer buffer
> >                         (or (eq buffer-offer-save 'always)
> >                             (and pred buffer-offer-save (> (buffer-size)
> 0)))))
> >
> >  Maybe instead of `pred' here we should say (eq pred t). Alternative,
> the predicate lambda should check
> >  whether `buffer-file-name' is non-nil.
> >
> > Probably the second option is better because the first option would
> prevent PRED from running for
> > non-file-visiting buffers. Patch attached.
>
> AFAIU, this will unconditionally disable auto-saving in *ediff-merge*
> and all the similar buffers, when auto-save-visited-mode is turned
> on.  Is that what we want?  And if that's going to be the default, how
> are users supposed to override it if they want?
>

I think not prompting is the right approach. It's not very user-friendly to
interrupt the user every couple of seconds, and no other application I know
that has autosave prompts the user for filenames. I think we should change
the behavior by applying the patch; if some users do want to be
interrupted, we can still add a customization option later.

[-- Attachment #2: Type: text/html, Size: 2491 bytes --]

Re: make pred customable in auto-save-visited-mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 01 Apr 2018 17:19:38 +0000
> Cc: jixiuf@qq.com, emacs-devel@gnu.org
> 
>  > Probably the second option is better because the first option would prevent PRED from running for
>  > non-file-visiting buffers. Patch attached.
> 
>  AFAIU, this will unconditionally disable auto-saving in *ediff-merge*
>  and all the similar buffers, when auto-save-visited-mode is turned
>  on.  Is that what we want?  And if that's going to be the default, how
>  are users supposed to override it if they want?
> 
> I think not prompting is the right approach. It's not very user-friendly to interrupt the user every couple of
> seconds, and no other application I know that has autosave prompts the user for filenames. I think we should
> change the behavior by applying the patch; if some users do want to be interrupted, we can still add a
> customization option later. 

AFAIU, you are not disabling the prompt, you are disabling the
auto-saving itself for such buffers.  I'm asking if that is what we
want -- disable auto-saving for buffers like *ediff-merge* when
auto-save-visited-mode is turned on.

Re: make pred customable in auto-save-visited-mode

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am So., 1. Apr. 2018 um 19:35 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 01 Apr 2018 17:19:38 +0000
> > Cc: jixiuf@qq.com, emacs-devel@gnu.org
> >
> >  > Probably the second option is better because the first option would
> prevent PRED from running for
> >  > non-file-visiting buffers. Patch attached.
> >
> >  AFAIU, this will unconditionally disable auto-saving in *ediff-merge*
> >  and all the similar buffers, when auto-save-visited-mode is turned
> >  on.  Is that what we want?  And if that's going to be the default, how
> >  are users supposed to override it if they want?
> >
> > I think not prompting is the right approach. It's not very user-friendly
> to interrupt the user every couple of
> > seconds, and no other application I know that has autosave prompts the
> user for filenames. I think we should
> > change the behavior by applying the patch; if some users do want to be
> interrupted, we can still add a
> > customization option later.
>
> AFAIU, you are not disabling the prompt, you are disabling the
> auto-saving itself for such buffers.  I'm asking if that is what we
> want -- disable auto-saving for buffers like *ediff-merge* when
> auto-save-visited-mode is turned on.
>

I think that's the right thing to do, yes.

[-- Attachment #2: Type: text/html, Size: 1886 bytes --]

Re: make pred customable in auto-save-visited-mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 01 Apr 2018 19:51:51 +0000
> Cc: jixiuf@qq.com, emacs-devel@gnu.org
> 
>  AFAIU, you are not disabling the prompt, you are disabling the
>  auto-saving itself for such buffers.  I'm asking if that is what we
>  want -- disable auto-saving for buffers like *ediff-merge* when
>  auto-save-visited-mode is turned on.
> 
> I think that's the right thing to do, yes. 

Very well, then please push to the release branch, and thanks.

Re: make pred customable in auto-save-visited-mode

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Mo., 2. Apr. 2018 um 06:44 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 01 Apr 2018 19:51:51 +0000
> > Cc: jixiuf@qq.com, emacs-devel@gnu.org
> >
> >  AFAIU, you are not disabling the prompt, you are disabling the
> >  auto-saving itself for such buffers.  I'm asking if that is what we
> >  want -- disable auto-saving for buffers like *ediff-merge* when
> >  auto-save-visited-mode is turned on.
> >
> > I think that's the right thing to do, yes.
>
> Very well, then please push to the release branch, and thanks.
>

Thanks, done.

[-- Attachment #2: Type: text/html, Size: 1116 bytes --]

Re: make pred customable in auto-save-visited-mode

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 31 Mar 2018 10:23:13 +0000
> Cc: 纪秀峰 <jixiuf@qq.com>, emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 31. März 2018 um 10:44 Uhr:
> 
>  > From: 纪秀峰 <jixiuf@qq.com>
>  > Date: Sat, 31 Mar 2018 00:59:19 +0800
>  > Feedback-ID: esmtp:qq.com:bgforeign:bgforeign1
>  >
>  > When I enable  auto-save-visited-mode and start an ediff merge session
>  > It always prompt me save "*ediff-merge*” to a file.
> 
>  Why is that a problem?  *ediff-merge* is a buffer where you prepare
>  the merge, and that merge will eventually be saved to some file,
>  right?  So it doesn't sound wrong to offer to save it.
> 
> However, auto-saving should probably not prompt for filenames. That's at least the intention of the :noprompt
> argument here.

I think you misinterpret the question with which Emacs prompts, see
below.

> The issue might be in `save-some-buffers': Its docstring says "If PRED is t, then certain non-file buffers will
> also be considered.", but later it only checks whether PRED is non-nil:

The question with which Emacs prompts is not the one controlled by
PRED, it's a question about the file name in which to save, I think.
It comes from basic-save-buffer, and that happens because
*ediff-merge* doesn't have a buffer-file-name.

So if we make the change you suggest, then Emacs will not offer to
save *ediff-merge* at all when this minor mode is turned on.  Is that
what we want?

Also, I don't think there's a problem with t and non-t non-nil values:
the doc string doesn't document such values that are not functions.
So there's no problem with checking that PRED is non-nil.