after-find-file-from-revert-buffer

after-find-file-from-revert-buffer

[Juanma Barranquero]

I'm using lexical-binding = t to detect cases of unused arguments and
variables, unwanted shadowing of global variables, etc. (I'm not
planning to convert more packages to lexical-binding, just doing part
of the footwork in case someone wants to do it later.)

In function `after-find-file', one of the parameters is
AFTER-FIND-FILE-FROM-REVERT-BUFFER, whose purpose is to bind that
(undocumented) variable so code run from inside `after-find-file' can
detect whether the function was called from revert-buffer or not.

Its only use, now and since the parameter's and variable's
introduction sixteen years ago, is in the `saveplace-find-file-hook'
function, run from `find-file-hook', to decide whether to move the
cursor or not.

So I want to rename the argument to CALLED-FROM-REVERT-BUFFER-P, and
use a let inside `after-find-file'

  (let ((after-find-file-from-revert-buffer called-from-revert-buffer-p))
  ;; whatever
  ...)

To strictly keep the current semantics, the "whatever" inside that
`let' should be all the current code of `after-find-file'. But that
seems ugly and wasteful, and I think we can go forth with just
let-binding the inside of the final (unless nomodes ...) form:

  (unless nomodes
    (let ((after-find-file-from-revert-buffer called-from-revert-buffer-p))
      (when (and view-read-only view-mode)
        (view-mode-disable))
      (normal-mode t)
      ;; If requested, add a newline at the end of the file.
      (and (memq require-final-newline '(visit visit-save))
           (> (point-max) (point-min))
           (/= (char-after (1- (point-max))) ?\n)
           (not (and (eq selective-display t)
                     (= (char-after (1- (point-max))) ?\r)))
           (save-excursion
             (goto-char (point-max))
             (insert "\n")))
      (when (and buffer-read-only
                 view-read-only
                 (not (eq (get major-mode 'mode-class) 'special)))
        (view-mode-enter))
      (run-hooks 'find-file-hook))))

Additionally, we could document the variable, or find another, less
ugly way to pass that information back to saveplace.el and remove the
variable altogether.

Comments?

TIA,

    Juanma

Re: after-find-file-from-revert-buffer

[Stefan Monnier]

> Additionally, we could document the variable, or find another, less
> ugly way to pass that information back to saveplace.el and remove the
> variable altogether.

Removing the variable sounds like the best option.  So the question is
how to get the same behavior in saveplace without using this variable.
Maybe saveplace could setup a local revert-buffer-function instead.


        Stefan

Re: after-find-file-from-revert-buffer

[Juanma Barranquero]

On Wed, Apr 6, 2011 at 03:34, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Maybe saveplace could setup a local revert-buffer-function instead.

I'll look into that.

If that works, what to do with 4th arg of `after-find-file'? Keep it
(and ignore it), or remove it and use
`set-advertised-calling-convention'?

    Juanma

Re: after-find-file-from-revert-buffer

[Stefan Monnier]

>> Maybe saveplace could setup a local revert-buffer-function instead.
> I'll look into that.

Thanks.

> If that works, what to do with 4th arg of `after-find-file'? Keep it
> (and ignore it), or remove it and use
> `set-advertised-calling-convention'?

Keep it, ignore it, and set-advertised-calling-convention.


        Stefan

Re: after-find-file-from-revert-buffer

[Juanma Barranquero]

> Removing the variable sounds like the best option.  So the question is
> how to get the same behavior in saveplace without using this variable.
> Maybe saveplace could setup a local revert-buffer-function instead.

The issue is that saveplace does not need or want a
revert-buffer-function. What it wants is to piggyback on
find-file-hook to restore the point in other buffers, but detect the
cases where that's caused by revert-buffer, to avoid unexpectedly
moving the point.

If the (before|after)-revert-hooks were always run, we could try to
use these (though it would be ugly); but a `revert-buffer-function' is
free to ignore them.

On the other hand, if the `revert-buffer-function' does not call
`after-find-file', or if it does but does not pass the
AFTER-FIND-FILE-FROM-REVERT-BUFFER parameter, the saveplace hook isn't
detecting the revert-buffer call anyway...

The only answer I can think is having something like
after-find-file-from-revert-buffer, but explicitly for revert-buffer:

(defvar revert-buffer-in-progress-p nil
   "This variable is non-nil whenever a `revert-buffer' operation is
in progress, nil otherwise.")

(defun revert-buffer (...)
   (let ((revert-buffer-in-progress t))
      ...))

which isn't not much better that which we have today, but at least is
better focused: it's cleaner to have a flag to signal use of
revert-buffer, than one to signal use of after-find-file inside
revert-buffer...

    Juanma


P.S. Well, it would be more like

@@ -5047,8 +5048,10 @@
   (interactive (list (not current-prefix-arg)))
   (if revert-buffer-function
-      (funcall revert-buffer-function ignore-auto noconfirm)
+      (let ((revert-buffer-in-progress-p t))
+        (funcall revert-buffer-function ignore-auto noconfirm))
     (with-current-buffer (or (buffer-base-buffer (current-buffer))
 			     (current-buffer))
-      (let* ((auto-save-p (and (not ignore-auto)
+      (let* ((revert-buffer-in-progress-p t)
+             (auto-save-p (and (not ignore-auto)
 			       (recent-auto-save-p)
 			       buffer-auto-save-file-name

Re: after-find-file-from-revert-buffer

[Stefan Monnier]

> The issue is that saveplace does not need or want a
> revert-buffer-function. What it wants is to piggyback on
> find-file-hook to restore the point in other buffers, but detect the
> cases where that's caused by revert-buffer, to avoid unexpectedly
> moving the point.

I was thinking of using a revert-buffer-function of the form

  (λ (&rest args)
    (let ((save-place-in-revert-buffer t))
      (apply revert-buffer-default args)))

> (defun revert-buffer (...)
>    (let ((revert-buffer-in-progress t))
>       ...))

> which isn't not much better that which we have today, but at least is
> better focused: it's cleaner to have a flag to signal use of
> revert-buffer, than one to signal use of after-find-file inside
> revert-buffer...

Yes, that's is better.

> @@ -5047,8 +5048,10 @@
>    (interactive (list (not current-prefix-arg)))
>    (if revert-buffer-function
> -      (funcall revert-buffer-function ignore-auto noconfirm)
> +      (let ((revert-buffer-in-progress-p t))
> +        (funcall revert-buffer-function ignore-auto noconfirm))
>      (with-current-buffer (or (buffer-base-buffer (current-buffer))
>  			     (current-buffer))
> -      (let* ((auto-save-p (and (not ignore-auto)
> +      (let* ((revert-buffer-in-progress-p t)
> +             (auto-save-p (and (not ignore-auto)
>  			       (recent-auto-save-p)
>  			       buffer-auto-save-file-name

I guess you could use a single let that covers both branches of the `if'.


        Stefan

Re: after-find-file-from-revert-buffer

[Juanma Barranquero]

On Wed, Apr 6, 2011 at 18:51, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I was thinking of using a revert-buffer-function of the form
>
>  (λ (&rest args)
>    (let ((save-place-in-revert-buffer t))
>      (apply revert-buffer-default args)))
>
>> (defun revert-buffer (...)
>>    (let ((revert-buffer-in-progress t))
>>       ...))

Global, you mean? I'm not sure it's worth the hassle.

> Yes, that's is better.

OK, I'll install that.

> I guess you could use a single let that covers both branches of the `if'.

IMO, it seems ugly to wrap the whole function just to add a variable.
But it's your call.

    Juanma

Re: after-find-file-from-revert-buffer

[Stefan Monnier]

>> Yes, that's is better.
> OK, I'll install that.

Fine,


        Stefan

Re: after-find-file-from-revert-buffer

[Juanma Barranquero]

> Keep it, ignore it, and set-advertised-calling-convention

The argument that is ignored is not the last one, so
`set-advertised-calling-convention' doesn't seem to be useful in this
case, unless we play games with the parameters.

    Juanma