Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Stefan Monnier]

> +;;;###autoload
>  (defun toggle-save-place (&optional parg) ;FIXME: save-place-local-mode!

As can be seen in the comment, I think this name should be changed (and
the implementation should use define-minor-mode), so I suggest we don't
autoload this until we've fixed the naming.


        Stefan

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 25 Feb 2016 08:44:37 -0500
> 
> > +;;;###autoload
> >  (defun toggle-save-place (&optional parg) ;FIXME: save-place-local-mode!
> 
> As can be seen in the comment, I think this name should be changed (and
> the implementation should use define-minor-mode), so I suggest we don't
> autoload this until we've fixed the naming.

It makes no sense not to autoload it, because then there's no way for
users to turn save-place in a single buffer.  Previously, users needed
to load saveplace.el to have the feature, so an autoload was not
needed.  But the changes which switched to save-place-mode invalidated
that paradigm, and created a deficiency, which I fixed.  (They also
broke the command, which I also fixed.)

I have nothing against having a local mode, but I don't have time to
work on this now, so patches are very welcome.

Thanks.

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Stefan Monnier]

> It makes no sense not to autoload it, because then there's no way for
> users to turn save-place in a single buffer.  Previously, users needed
> to load saveplace.el to have the feature, so an autoload was not
> needed.

But that still works "as well as before"
(i.e. as long as they (require 'saveplace)).

> But the changes which switched to save-place-mode invalidated
> that paradigm, and created a deficiency, which I fixed.  (They also
> broke the command, which I also fixed.)

I think I'm beginning to understand, yes, thanks.

> I have nothing against having a local mode, but I don't have time to
> work on this now, so patches are very welcome.

I think I have something better, but I think some of the changes are too
late for 25.1, so I suggest we install the patch below (which against
forces users to use (require 'saveplace) before calling
toggle-save-place, like before: it's not ideal, but it's not
a regression).


        Stefan


2016-02-25  Stefan Monnier  <monnier@iro.umontreal.ca>

	* (toggle-save-place): Don't autoload yet (will do it in 26 when we
	rename it and turn it into a minor mode).  Avoid obsolete var
	`save-place'.  Don't remove hooks.
        lisp/saveplace.el (save-place-mode): Remove global find-file-hook
	since we also add it globally.


diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 0233a52..b7c77a4 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -136,14 +136,13 @@ save-place-mode
       (add-hook 'kill-emacs-hook 'save-place-kill-emacs-hook))
     (add-hook 'kill-buffer-hook 'save-place-to-alist))
    (t
-    (remove-hook 'find-file-hook 'save-place-find-file-hook t)
+    (remove-hook 'find-file-hook 'save-place-find-file-hook)
     (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
     (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
     (remove-hook 'kill-buffer-hook 'save-place-to-alist))))
 
 (make-variable-buffer-local 'save-place-mode) ; Hysterical raisins.
 
-;;;###autoload
 (defun toggle-save-place (&optional parg) ;FIXME: save-place-local-mode!
   "Toggle whether to save your place in this file between sessions.
 If this mode is enabled, point is recorded when you kill the buffer
@@ -167,17 +166,21 @@ toggle-save-place
                          (> (prefix-numeric-value parg) 0)
                        (not save-place)))
     (cond
-     (save-place
+     (save-place-mode
       (add-hook 'find-file-hook 'save-place-find-file-hook t)
       (add-hook 'dired-initial-position-hook 'save-place-dired-hook)
       (unless noninteractive
         (add-hook 'kill-emacs-hook 'save-place-kill-emacs-hook))
       (add-hook 'kill-buffer-hook 'save-place-to-alist))
      (t
-      (remove-hook 'find-file-hook 'save-place-find-file-hook t)
-      (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
-      (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
-      (remove-hook 'kill-buffer-hook 'save-place-to-alist)))
+      ;; FIXME: We should remove the hooks, but only in case this was the last
+      ;; buffer using save-place-local-mode.  Is it worth the trouble, tho?
+      ;; (unless (default-value 'save-place-mode)
+      ;;   (remove-hook 'find-file-hook 'save-place-find-file-hook)
+      ;;   (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
+      ;;   (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
+      ;;   (remove-hook 'kill-buffer-hook 'save-place-to-alist))
+      ))
     (message (if save-place
                  "Place will be saved"
                "No place will be saved in this file"))))

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 25 Feb 2016 12:05:06 -0500
> 
> > It makes no sense not to autoload it, because then there's no way for
> > users to turn save-place in a single buffer.  Previously, users needed
> > to load saveplace.el to have the feature, so an autoload was not
> > needed.
> 
> But that still works "as well as before"
> (i.e. as long as they (require 'saveplace)).

Except that I just told them in NEWS not to...

> > I have nothing against having a local mode, but I don't have time to
> > work on this now, so patches are very welcome.
> 
> I think I have something better, but I think some of the changes are too
> late for 25.1, so I suggest we install the patch below (which against
> forces users to use (require 'saveplace) before calling
> toggle-save-place, like before: it's not ideal, but it's not
> a regression).

Fine with me, thanks.

> -;;;###autoload

I'd like to keep the autoload, otherwise there's an asymmetry: the
global mode is autoloaded, the local one isn't.

>  (defun toggle-save-place (&optional parg) ;FIXME: save-place-local-mode!
>    "Toggle whether to save your place in this file between sessions.
>  If this mode is enabled, point is recorded when you kill the buffer
> @@ -167,17 +166,21 @@ toggle-save-place
>                           (> (prefix-numeric-value parg) 0)
>                         (not save-place)))
>      (cond
> -     (save-place
> +     (save-place-mode

Shouldn't we also replace save-place above this fragment?

> +      ;; FIXME: We should remove the hooks, but only in case this was the last
> +      ;; buffer using save-place-local-mode.  Is it worth the trouble, tho?

No, I don't think it's worth the trouble.  Chances are, the user will
promptly enable the mode in some other buffer.

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Stefan Monnier]

>> -;;;###autoload
> I'd like to keep the autoload, otherwise there's an asymmetry: the
> global mode is autoloaded, the local one isn't.

But there's no "local mode" yet.  That'll be added in 26.
For 25.1 there's only a "local toggle function".

It's mostly a question of name, admittedly.  

An alternative is to use the patch below, which is more intrusive.

diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 0233a52..b2140fb 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -120,6 +120,25 @@ save-place-ignore-files-regexp
 
 (declare-function dired-current-directory "dired" (&optional localp))
 
+(defun save-place--setup-hooks (add)
+  (cond
+   (add
+    (add-hook 'find-file-hook #'save-place-find-file-hook t)
+    (add-hook 'dired-initial-position-hook #'save-place-dired-hook)
+    (unless noninteractive
+      (add-hook 'kill-emacs-hook #'save-place-kill-emacs-hook))
+    (add-hook 'kill-buffer-hook #'save-place-to-alist))
+   (t
+    ;; We should remove the hooks, but only if save-place-mode
+    ;; is nil everywhere.  Is it worth the trouble, tho?
+    ;; (unless (or (default-value 'save-place-mode)
+    ;;             (cl-some <save-place-local-mode-p> (buffer-list)))
+    ;;   (remove-hook 'find-file-hook #'save-place-find-file-hook)
+    ;;   (remove-hook 'dired-initial-position-hook #'save-place-dired-hook)
+    ;;   (remove-hook 'kill-emacs-hook #'save-place-kill-emacs-hook)
+    ;;   (remove-hook 'kill-buffer-hook #'save-place-to-alist))
+    )))
+
 (define-obsolete-variable-alias 'save-place 'save-place-mode "25.1")
 ;;;###autoload
 (define-minor-mode save-place-mode
@@ -128,23 +147,14 @@ save-place-mode
 where it was when you previously visited the same file."
   :global t
   :group 'save-place
-  (cond
-   (save-place-mode
-    (add-hook 'find-file-hook 'save-place-find-file-hook t)
-    (add-hook 'dired-initial-position-hook 'save-place-dired-hook)
-    (unless noninteractive
-      (add-hook 'kill-emacs-hook 'save-place-kill-emacs-hook))
-    (add-hook 'kill-buffer-hook 'save-place-to-alist))
-   (t
-    (remove-hook 'find-file-hook 'save-place-find-file-hook t)
-    (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
-    (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
-    (remove-hook 'kill-buffer-hook 'save-place-to-alist))))
+  (save-place--setup-hooks save-place-mode))
 
-(make-variable-buffer-local 'save-place-mode) ; Hysterical raisins.
+(make-variable-buffer-local 'save-place-mode)
 
+(define-obsolete-function-alias 'toggle-save-place
+  #'save-place-local-mode "25.1")
 ;;;###autoload
-(defun toggle-save-place (&optional parg) ;FIXME: save-place-local-mode!
+(define-minor-mode save-place-local-mode
   "Toggle whether to save your place in this file between sessions.
 If this mode is enabled, point is recorded when you kill the buffer
 or exit Emacs.  Visiting this file again will go to that position,
@@ -157,30 +167,13 @@ toggle-save-place
 file:
 
 \(save-place-mode 1)"
-  (interactive "P")
+  :variable save-place-mode
   (if (not (or buffer-file-name (and (derived-mode-p 'dired-mode)
                                      (boundp 'dired-subdir-alist)
 				     dired-subdir-alist
 				     (dired-current-directory))))
       (message "Buffer `%s' not visiting a file or directory" (buffer-name))
-    (setq save-place (if parg
-                         (> (prefix-numeric-value parg) 0)
-                       (not save-place)))
-    (cond
-     (save-place
-      (add-hook 'find-file-hook 'save-place-find-file-hook t)
-      (add-hook 'dired-initial-position-hook 'save-place-dired-hook)
-      (unless noninteractive
-        (add-hook 'kill-emacs-hook 'save-place-kill-emacs-hook))
-      (add-hook 'kill-buffer-hook 'save-place-to-alist))
-     (t
-      (remove-hook 'find-file-hook 'save-place-find-file-hook t)
-      (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
-      (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
-      (remove-hook 'kill-buffer-hook 'save-place-to-alist)))
-    (message (if save-place
-                 "Place will be saved"
-               "No place will be saved in this file"))))
+    (save-place--setup-hooks save-place-mode)))
 
 (declare-function dired-get-filename "dired" (&optional localp no-error-if-not-filep))
 

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 25 Feb 2016 12:57:49 -0500
> 
> An alternative is to use the patch below, which is more intrusive.

I think it's fine for emacs-25, thanks.

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Stefan Monnier]

Oh, one more thing:

> +    (cond
> +     (save-place

Earlier in the file we have

    (define-obsolete-variable-alias 'save-place 'save-place-mode "25.1")

so we should use save-place-mode here.


        Stefan

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Stefan Monnier]

> Oh, one more thing:

Duh!  I shouldn't be so quick on the trigger, eh?
There's more:

toggle-save-place is supposed to be buffer-local, so:

> +     (t
> +      (remove-hook 'find-file-hook 'save-place-find-file-hook t)
> +      (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
> +      (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
> +      (remove-hook 'kill-buffer-hook 'save-place-to-alist)))

is wrong, since it will turn off save-place globally.


        Stefan

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 25 Feb 2016 08:56:31 -0500
> 
> > +     (t
> > +      (remove-hook 'find-file-hook 'save-place-find-file-hook t)
> > +      (remove-hook 'dired-initial-position-hook 'save-place-dired-hook)
> > +      (remove-hook 'kill-emacs-hook 'save-place-kill-emacs-hook)
> > +      (remove-hook 'kill-buffer-hook 'save-place-to-alist)))
> 
> is wrong, since it will turn off save-place globally.

Yes.

Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 25 Feb 2016 08:51:21 -0500
> 
> Oh, one more thing:
> 
> > +    (cond
> > +     (save-place
> 
> Earlier in the file we have
> 
>     (define-obsolete-variable-alias 'save-place 'save-place-mode "25.1")
> 
> so we should use save-place-mode here.

Yes, feel free to change.