bug#16630: desktop-auto-save seems confusing

bug#16630: desktop-auto-save seems confusing

[Glenn Morris]

Package: emacs
Version: 24.3.50

desktop-auto-save has what seems to me to be confusing, limited functionality.

emacs -Q   ; no existing desktop
M-x desktop-save-mode
C-h v desktop-auto-save-timer
   -> nil

M-x customize-variable RET desktop-auto-save-timeout RET
  Set to 20, set for current session

C-h v desktop-auto-save-timer
  -> no longer nil, but still does not auto-save anything,
     because desktop-owner returns nil


Basically, it seems that the only way this feature does anything is if
you enable (desktop-save-mode) in .emacs AND have a pre-existing desktop
file when you start Emacs.

Why doesn't enabling desktop-save-mode at any time just turn it on?
Why does it require the desktop file to exist beforehand?

bug#16630: desktop-auto-save seems confusing

[Juri Linkov]

> Why doesn't enabling desktop-save-mode at any time just turn it on?
> Why does it require the desktop file to exist beforehand?

desktop-auto-save is an noninteractive operation, so first
you need to select a directory where you want to save the desktop
using M-x desktop-save or M-x desktop-save-in-desktop-dir
that asks interactively where to save the desktop.
This allows desktop-auto-save to know the desktop file name.

If there is no other way to let desktop-auto-save know the desktop file name,
then the need to use M-x desktop-save after activating desktop-auto-save
could be mentioned in the docstring.

bug#16630: desktop-auto-save seems confusing

[Glenn Morris]

Juri Linkov wrote:

>> Why doesn't enabling desktop-save-mode at any time just turn it on?
>> Why does it require the desktop file to exist beforehand?
>
> desktop-auto-save is an noninteractive operation, so first
> you need to select a directory where you want to save the desktop
> using M-x desktop-save or M-x desktop-save-in-desktop-dir
> that asks interactively where to save the desktop.
> This allows desktop-auto-save to know the desktop file name.

emacs -Q
M-x desktop-save-mode RET
M-x desktop-save RET /tmp RET
C-h v desktop-auto-save-timer  
   -> still nil, still no auto-save

As I said, nothing enables desktop-auto-save except after-init-hook.

Whereas if I happen to customize desktop-auto-save-timeout, suddenly it
will start auto-saving. This doesn't make much sense.

(Also, I guess the ignore-errors in the :set of desktop-auto-save-timeout
isn't really needed, rather an :initialize is.)

bug#16630: desktop-auto-save seems confusing

[Juri Linkov]

> emacs -Q
> M-x desktop-save-mode RET
> M-x desktop-save RET /tmp RET
> C-h v desktop-auto-save-timer
>    -> still nil, still no auto-save
>
> As I said, nothing enables desktop-auto-save except after-init-hook.
>
> Whereas if I happen to customize desktop-auto-save-timeout, suddenly it
> will start auto-saving. This doesn't make much sense.

Yes, definitely `desktop-auto-save-set-timer' have to be called
in every place that changes one of the preconditions of
`desktop-auto-save' to save the desktop:

  (when (and desktop-save-mode
	     (integerp desktop-auto-save-timeout)
	     (> desktop-auto-save-timeout 0)
	     ;; Avoid desktop saving during lazy loading.
	     (not desktop-lazy-timer)
	     ;; Save only to own desktop file.
	     (eq (emacs-pid) (desktop-owner))
	     desktop-dirname)

Here changes in the value of `desktop-auto-save-timeout'
are handled already by :set in its defcustom, `desktop-lazy-timer'
quickly becomes nil after reading the existing desktop.

So what remains to do is to activate the timer when
the value of `desktop-dirname' changes from nil to non-nil.
This happens in two commands `desktop-read' and `desktop-save'.
In the following patch `desktop-read' and `desktop-save'
will activate the auto-saving timer (its call from `after-init-hook'
is removed because it will be handled by `desktop-read').

Also the condition (eq (emacs-pid) (desktop-owner)) depends on bug#16157.
If it claims the lock only in `desktop-read' and `desktop-save'
then no more calls for `desktop-auto-save-set-timer' are necessary.

=== modified file 'lisp/desktop.el'
--- lisp/desktop.el	2014-01-01 07:43:34 +0000
+++ lisp/desktop.el	2014-02-04 07:48:37 +0000
@@ -1012,7 +1012,10 @@ (defun desktop-save (dirname &optional r
 		(write-region (point-min) (point-max) (desktop-full-file-name) nil 'nomessage))
 	      (setq desktop-file-checksum checksum)
 	      ;; We remember when it was modified (which is presumably just now).
-	      (setq desktop-file-modtime (nth 5 (file-attributes (desktop-full-file-name)))))))))))
+	      (setq desktop-file-modtime (nth 5 (file-attributes (desktop-full-file-name))))))))))
+  ;; When the desktop is saved not by auto-saving, activate the auto-saving timer.
+  (unless auto-save
+    (desktop-auto-save-set-timer)))
 
 ;; ----------------------------------------------------------------------------
 ;;;###autoload
@@ -1103,5 +1106,8 @@ (defun desktop-read (&optional dirname)
 	    ;; Evaluate desktop buffer and remember when it was modified.
 	    (load (desktop-full-file-name) t t t)
 	    (setq desktop-file-modtime (nth 5 (file-attributes (desktop-full-file-name))))
+	    ;; Activate the auto-saving timer after setting
+	    ;; `desktop-dirname' and `desktop-file-modtime'.
+	    (desktop-auto-save-set-timer)
 	    ;; If it wasn't already, mark it as in-use, to bother other
 	    ;; desktop instances.
	    (unless owner
@@ -1468,7 +1474,6 @@ (add-hook
         (setq desktop-save-mode nil)))
     (when desktop-save-mode
       (desktop-read)
-      (desktop-auto-save-set-timer)
       (setq inhibit-startup-screen t))))
 

> (Also, I guess the ignore-errors in the :set of desktop-auto-save-timeout
> isn't really needed, rather an :initialize is.)

Without ignore-errors, after doing

  emacs -Q
  M-x desktop-save-mode RET

it fails with

  Debugger entered--Lisp error: (void-function desktop-auto-save-set-timer)
    desktop-auto-save-set-timer()
    custom-initialize-reset(desktop-auto-save-timeout ...)
    custom-declare-variable(desktop-auto-save-timeout ...)
    byte-code(...)
    autoload-do-load((autoload "desktop" 870431 t nil) desktop-save-mode)
    command-execute(desktop-save-mode record)
    execute-extended-command(nil "desktop-save-mode")
    call-interactively(execute-extended-command nil nil)
    command-execute(execute-extended-command)

bug#16630: desktop-auto-save seems confusing

[Glenn Morris]

Juri Linkov wrote:

> Yes, definitely `desktop-auto-save-set-timer' have to be called
> in every place that changes one of the preconditions of
> `desktop-auto-save' to save the desktop:

Why doesn't desktop-save-mode simply enable the auto-save timer?
The timer function itself seems to take the necessary steps to ensure it
only actually does something when appropriate.

bug#16630: desktop-auto-save seems confusing

[Juri Linkov]

> Why doesn't desktop-save-mode simply enable the auto-save timer?
> The timer function itself seems to take the necessary steps to ensure it
> only actually does something when appropriate.

This would be simpler, and I guess there should not be much
performance overhead of firing the timer that does nothing
in some rare cases (when the desktop filename is not yet known
or another instance acquires the lock).

But `desktop-auto-save-set-timer' still needs to be called
from :set of `desktop-auto-save-timeout' defcustom for the case
when the user customizes the timeout either from 0 to a positive integer,
or back to 0 that needs to cancel the timer.

=== modified file 'lisp/desktop.el'
--- lisp/desktop.el	2014-01-01 07:43:34 +0000
+++ lisp/desktop.el	2014-02-05 07:47:45 +0000
@@ -162,7 +162,10 @@ (define-minor-mode desktop-save-mode
 one session to another.  See variable `desktop-save' and function
 `desktop-read' for details."
   :global t
-  :group 'desktop)
+  :group 'desktop
+  (if desktop-save-mode
+      (desktop-auto-save-set-timer)
+    (desktop-auto-save-cancel-timer)))
 
 (defun desktop-save-mode-off ()
   "Disable `desktop-save-mode'.  Provided for use in hooks."
@@ -1216,6 +1219,11 @@ (defun desktop-auto-save-set-timer ()
 	  (run-with-idle-timer desktop-auto-save-timeout t
 			       'desktop-auto-save))))
 
+(defun desktop-auto-save-cancel-timer ()
+  (when desktop-auto-save-timer
+    (cancel-timer desktop-auto-save-timer)
+    (setq desktop-auto-save-timer nil)))
+
 ;; ----------------------------------------------------------------------------
 ;;;###autoload
 (defun desktop-revert ()
@@ -1465,10 +1471,9 @@ (add-hook
     (let ((key "--no-desktop"))
       (when (member key command-line-args)
         (setq command-line-args (delete key command-line-args))
-        (setq desktop-save-mode nil)))
+        (desktop-save-mode 0)))
     (when desktop-save-mode
       (desktop-read)
-      (desktop-auto-save-set-timer)
       (setq inhibit-startup-screen t))))
 
 ;; So we can restore vc-dir buffers.

bug#16630: desktop-auto-save seems confusing

[Juri Linkov]

> Why doesn't desktop-save-mode simply enable the auto-save timer?
> The timer function itself seems to take the necessary steps to ensure it
> only actually does something when appropriate.

I installed a patch that enables the auto-save timer in desktop-save-mode.

PS: I see nothing wrong in your revision 116250.  I recommend reinstating it.