all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
       [not found] ` <E1aYgUg-0006y7-3M@vcs.savannah.gnu.org>
@ 2016-02-25 13:44   ` Stefan Monnier
  2016-02-25 16:09     ` Eli Zaretskii
  2016-02-25 13:51   ` Stefan Monnier
  1 sibling, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2016-02-25 13:44 UTC (permalink / raw)
  To: emacs-devel; +Cc: Eli Zaretskii

> +;;;###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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
       [not found] ` <E1aYgUg-0006y7-3M@vcs.savannah.gnu.org>
  2016-02-25 13:44   ` [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place' Stefan Monnier
@ 2016-02-25 13:51   ` Stefan Monnier
  2016-02-25 13:56     ` Stefan Monnier
  2016-02-25 16:10     ` Eli Zaretskii
  1 sibling, 2 replies; 10+ messages in thread
From: Stefan Monnier @ 2016-02-25 13:51 UTC (permalink / raw)
  To: emacs-devel; +Cc: Eli Zaretskii

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 13:51   ` Stefan Monnier
@ 2016-02-25 13:56     ` Stefan Monnier
  2016-02-25 16:10       ` Eli Zaretskii
  2016-02-25 16:10     ` Eli Zaretskii
  1 sibling, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2016-02-25 13:56 UTC (permalink / raw)
  To: emacs-devel; +Cc: Eli Zaretskii

> 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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 13:44   ` [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place' Stefan Monnier
@ 2016-02-25 16:09     ` Eli Zaretskii
  2016-02-25 17:05       ` Stefan Monnier
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2016-02-25 16:09 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 13:51   ` Stefan Monnier
  2016-02-25 13:56     ` Stefan Monnier
@ 2016-02-25 16:10     ` Eli Zaretskii
  1 sibling, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2016-02-25 16:10 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 13:56     ` Stefan Monnier
@ 2016-02-25 16:10       ` Eli Zaretskii
  0 siblings, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2016-02-25 16:10 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 16:09     ` Eli Zaretskii
@ 2016-02-25 17:05       ` Stefan Monnier
  2016-02-25 17:34         ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2016-02-25 17:05 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

> 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"))))



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 17:05       ` Stefan Monnier
@ 2016-02-25 17:34         ` Eli Zaretskii
  2016-02-25 17:57           ` Stefan Monnier
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2016-02-25 17:34 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 17:34         ` Eli Zaretskii
@ 2016-02-25 17:57           ` Stefan Monnier
  2016-02-25 18:13             ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2016-02-25 17:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

>> -;;;###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))
 



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place'
  2016-02-25 17:57           ` Stefan Monnier
@ 2016-02-25 18:13             ` Eli Zaretskii
  0 siblings, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2016-02-25 18:13 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

> 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.



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-02-25 18:13 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20160224205710.26753.62720@vcs.savannah.gnu.org>
     [not found] ` <E1aYgUg-0006y7-3M@vcs.savannah.gnu.org>
2016-02-25 13:44   ` [Emacs-diffs] emacs-25 040e0d6: Fix 'toggle-save-place' Stefan Monnier
2016-02-25 16:09     ` Eli Zaretskii
2016-02-25 17:05       ` Stefan Monnier
2016-02-25 17:34         ` Eli Zaretskii
2016-02-25 17:57           ` Stefan Monnier
2016-02-25 18:13             ` Eli Zaretskii
2016-02-25 13:51   ` Stefan Monnier
2016-02-25 13:56     ` Stefan Monnier
2016-02-25 16:10       ` Eli Zaretskii
2016-02-25 16:10     ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.