bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
The attached proposed patch adds a defcustom and makes a change to two
existing functions in order to allow a user the option to only have
autorevert mode operate on visible buffers. The change to function
'after-find-files' may also fix a bug (or change behavior) in that
sometimes when being prompted to manually revert a buffer that had been
in auto-revert-mode, the auto-revert-mode status would be lost, ie. set
to nil.

Use of this feature:

1) Reduces CPU workload when using autorevert

2) Reduces messages when using `auto-revert-verbose'

Possible disadvantage:

1) When making a buffer visible, it might take as long as
   'auto-revert-interval' seconds for any accumulated changes to
   auto-revert. The default for that value is five seconds.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

autorevert.patch (1K) Download Attachment
files.patch (538 bytes) Download Attachment
NEWS.patch (550 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Eli Zaretskii
> Date: Tue, 15 Sep 2020 00:07:28 -0400
> From: Boruch Baum <[hidden email]>
>
> +(defcustom auto-revert-only-if-visible nil
> +  "If non-nil, suppress Auto-Revert Mode when a buffer isn't visible."

"Visible" is ambiguous in this context.  I suggest "not displayed in
any window" instead.  Or, better, just "auto-revert only if buffer is
displayed" (which also solved a problem with double negation).

> +  :version "28")
              ^^^^
"28.1"

> +                         (if (display-graphic-p) 'visible (window-normalize-frame nil)))))))

Is the different treatment of GUI and TTY frames necessary here?

Btw, what will be the effect of this option?  Suppose some buffer was
not displayed and missed its auto-revert opportunity.  Then I switch
to it in some window -- will it appear with stale contents, or will it
auto-revert before being displayed in the window?



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
On 2020-09-15 17:30, Eli Zaretskii wrote:
> > Date: Tue, 15 Sep 2020 00:07:28 -0400
> > From: Boruch Baum <[hidden email]>
> >
> > +(defcustom auto-revert-only-if-visible nil
> > +  "If non-nil, suppress Auto-Revert Mode when a buffer isn't visible."
>
> "Visible" is ambiguous in this context.  I suggest "not displayed in
> any window" instead.  Or, better, just "auto-revert only if buffer is
> displayed" (which also solved a problem with double negation).

In the case of TTY emacs, only one frame seems possible to ever be
visible, but each and every frame seems to be considered, let's say
'displayed'. In that situation, this patch only auto-reverts buffers
displayed on windows of the actually 'displayed' TTY frame.

> > +  :version "28")
>               ^^^^
> "28.1"

Thanks. I had just taken a guess.

> > +                         (if (display-graphic-p) 'visible (window-normalize-frame nil)))))))
>
> Is the different treatment of GUI and TTY frames necessary here?

For users of TTY emacs (ahem, me), most assuredly. In TTY emacs, AFAICT
only one frame can ever be 'displayed' (actually visible), but function
`get-buffer-window-list' with argument ALL-FRAMES set to 'visible
returns all windows on all frames. From that function's docstring:

  --8<--cut here-(start)------------------------------------------- >8
  - ‘visible’ means consider all windows on all visible frames on
    the current terminal.
  --8<--cut here-(end)--------------------------------------------- >8

That behavior of function `get-buffer-window-list' may in turn be
because of the behavior of function `frame-visible-p', whose docstring
begins with:

  --8<--cut here-(start)------------------------------------------- >8
  Return t if FRAME is "visible" (actually in use for display).
  --8<--cut here-(end)--------------------------------------------- >8

But ends with the punch-line:

  --8<--cut here-(start)------------------------------------------- >8
  If FRAME is a text terminal frame, this always returns t.
  Such frames are always considered visible, whether or not they are
  currently being displayed on the terminal.
  --8<--cut here-(end)--------------------------------------------- >8

So since for the purposes of this patch, what matters is the actual user
experience, the distinction *was* needed.

  Off-topic: would anything constructive result if I submit a separate
  'complaint' about that behavior / judgment / decision that 'Such
  frames are always considered visible'?

> Btw, what will be the effect of this option?  Suppose some buffer was
> not displayed and missed its auto-revert opportunity.  Then I switch
> to it in some window -- will it appear with stale contents, or will it
> auto-revert before being displayed in the window?

I kind of mentioned this in my note: From my testing, at the instant an
un-reverted buffer is displayed, it is in its 'stale' state but is also
instantly considered for auto-revert. The *theoretical* maximum delay to
the auto-revert then being performed is based upon defcustom
`auto-revert-interval', whose default is five seconds. In practice, the
possible downsides are that: 1) it might be visually surprising to the
user to see the auto-revert occur; 2) if the user was very hurried to
type at wherever POINT was in the buffer, then the auto-revert would be
further delayed if defcustom `auto-revert-stop-on-user-input' was non-nil
(the default), and would only happen once the hurried typist pauses. In
my anecdotal use / testing / debugging, this has only every happened
when I consciously made the effort to race the auto-revert, but the
experience of a caffeinated or adrenaline-d user might be different.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Eli Zaretskii
> Date: Tue, 15 Sep 2020 11:39:58 -0400
> From: Boruch Baum <[hidden email]>
> Cc: [hidden email]
>
>   Off-topic: would anything constructive result if I submit a separate
>   'complaint' about that behavior / judgment / decision that 'Such
>   frames are always considered visible'?

That's why I said "visible" is ambiguous.  "Frame visibility" is a
concept in Emacs, so changing it now, and for TTY displays on top of
that, is next to unimaginable, IMO.

> > Btw, what will be the effect of this option?  Suppose some buffer was
> > not displayed and missed its auto-revert opportunity.  Then I switch
> > to it in some window -- will it appear with stale contents, or will it
> > auto-revert before being displayed in the window?
>
> I kind of mentioned this in my note: From my testing, at the instant an
> un-reverted buffer is displayed, it is in its 'stale' state but is also
> instantly considered for auto-revert.

Maybe we should arrange it to actually auto-revert before being
displayed.  I envision bug reports if we don't.



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
On 2020-09-15 18:49, Eli Zaretskii wrote:

> > Date: Tue, 15 Sep 2020 11:39:58 -0400
> > From: Boruch Baum <[hidden email]>
> > Cc: [hidden email]
> >
> > > Btw, what will be the effect of this option?  Suppose some buffer was
> > > not displayed and missed its auto-revert opportunity.  Then I switch
> > > to it in some window -- will it appear with stale contents, or will it
> > > auto-revert before being displayed in the window?
> >
> > I kind of mentioned this in my note: From my testing, at the instant an
> > un-reverted buffer is displayed, it is in its 'stale' state but is also
> > instantly considered for auto-revert.
>
> Maybe we should arrange it to actually auto-revert before being
> displayed.  I envision bug reports if we don't.

Do you have a strategy or implementation in mind? Would adding a
function to `window-configuration-change-hook' do the trick (ie. catch
all relevant events)?

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Eli Zaretskii
> Date: Tue, 15 Sep 2020 12:12:39 -0400
> From: Boruch Baum <[hidden email]>
> Cc: [hidden email]
>
> > Maybe we should arrange it to actually auto-revert before being
> > displayed.  I envision bug reports if we don't.
>
> Do you have a strategy or implementation in mind? Would adding a
> function to `window-configuration-change-hook' do the trick (ie. catch
> all relevant events)?

Yes, I think so.



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Michael Albinus
In reply to this post by Boruch Baum
Boruch Baum <[hidden email]> writes:

Hi Boruch,

> The attached proposed patch adds a defcustom and makes a change to two
> existing functions in order to allow a user the option to only have
> autorevert mode operate on visible buffers. The change to function
> 'after-find-files' may also fix a bug (or change behavior) in that
> sometimes when being prompted to manually revert a buffer that had been
> in auto-revert-mode, the auto-revert-mode status would be lost, ie. set
> to nil.

I haven't tested your patch, but I'm asking myself whether it cooperates
with autorevert triggered by file notifications. That is, if a file
changes on disk, a notification event is sent, and autovert happens. If
autorevert does not happen because the buffer is not visible, this event
will be lost, and no autorevert will happen later on when the buffer
becomes visible.

Have you tested this scenario?

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
On 2020-09-15 20:47, Michael Albinus wrote:
> I haven't tested your patch, but I'm asking myself whether it cooperates
> with autorevert triggered by file notifications.
> ...
> Have you tested this scenario?

YES! In fact, that was a major motivation for the submission, and
the primary method of testing it.

Please do test the patch, and report back.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
In reply to this post by Eli Zaretskii
On 2020-09-15 20:24, Eli Zaretskii wrote:

> > Date: Tue, 15 Sep 2020 12:12:39 -0400
> > From: Boruch Baum <[hidden email]>
> > Cc: [hidden email]
> >
> > > Maybe we should arrange it to actually auto-revert before being
> > > displayed.  I envision bug reports if we don't.
> >
> > Do you have a strategy or implementation in mind? Would adding a
> > function to `window-configuration-change-hook' do the trick (ie. catch
> > all relevant events)?
>
> Yes, I think so.

It looks to me like that's not the case: my testing seems to show that
it does catch cases of changing a buffer displayed in a window but does
not catch changes of frames due to functions like `other-frame' or
`select-frame'.

So, the good news is that I've written the code that makes the
improvement for the caught cases, and I can submit that.

As for the cases of changing frames, a less-desirable option would be to
preempt bug-reports by documenting the limitation. Auto-revert already
has other curious limitations (eg. for dired buffers it doesn't operate
_at__all_ on many types of file changes), and this limitation only
introduces a delay, so by comparison its a pretty mild limitation.

A better option would be able to catch frame-change events. I haven't
found a straightforward way to trap that. Does such a method exist?

An inelegant solution that would cover most of the remaining events
would be to advise :after ~4 frame functions, and to add an element to
variable `move-frame-functions'.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Michael Albinus
In reply to this post by Boruch Baum
Boruch Baum <[hidden email]> writes:

Hi Boruch,

>> I haven't tested your patch, but I'm asking myself whether it cooperates
>> with autorevert triggered by file notifications.
>> ...
>> Have you tested this scenario?
>
> YES! In fact, that was a major motivation for the submission, and
> the primary method of testing it.
>
> Please do test the patch, and report back.

I see (still haven't tested, just reviewing). I don't understand yet,
why it is necessary to move "(setq auto-revert-notify-modified-p nil)"
into the "(when (and revert ..." form.

Furthermore, I see a problem to call auto-revert functionality in
files.el. autorevert.el is not dumped into the emacs binary. Wouldn't it
be better to add a function to find-file-hook? I understand the problem
with adding a newline at file end, but maybe this could be solved differently?

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Eli Zaretskii
In reply to this post by Boruch Baum
> Date: Wed, 16 Sep 2020 16:11:04 -0400
> From: Boruch Baum <[hidden email]>
> Cc: [hidden email]
>
> It looks to me like that's not the case: my testing seems to show that
> it does catch cases of changing a buffer displayed in a window but does
> not catch changes of frames due to functions like `other-frame' or
> `select-frame'.
>
> So, the good news is that I've written the code that makes the
> improvement for the caught cases, and I can submit that.
>
> As for the cases of changing frames, a less-desirable option would be to
> preempt bug-reports by documenting the limitation. Auto-revert already
> has other curious limitations (eg. for dired buffers it doesn't operate
> _at__all_ on many types of file changes), and this limitation only
> introduces a delay, so by comparison its a pretty mild limitation.

If we have no better way, we could document this as a limitation,
yes.  But let's make one more attempt to solve this.

> A better option would be able to catch frame-change events. I haven't
> found a straightforward way to trap that. Does such a method exist?

Can you describe the problematic case in more detail?  With that in
hand, perhaps Martin (CC'ed) could suggest a method.

> An inelegant solution that would cover most of the remaining events
> would be to advise :after ~4 frame functions, and to add an element to
> variable `move-frame-functions'.

Yes, I'd prefer to avoid such solutions.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
In reply to this post by Michael Albinus
On 2020-09-17 13:07, Michael Albinus wrote:

> Boruch Baum <[hidden email]> writes:
>
> Hi Boruch,
>
> >> I haven't tested your patch, but I'm asking myself whether it cooperates
> >> with autorevert triggered by file notifications.
> >> ...
> >> Have you tested this scenario?
> >
> > YES! In fact, that was a major motivation for the submission, and
> > the primary method of testing it.
> >
> > Please do test the patch, and report back.
>
> I see (still haven't tested, just reviewing).

Actually testing the code would be helpful. I'm beginning to feel the
'crunch' in pre-holiday preparations for Jewish New Years, so you're on
your own until at least sometime Monday.

> I don't understand yet, why it is necessary to move "(setq
> auto-revert-notify-modified-p nil)" into the "(when (and revert ..."
> form.

The reset was being applied too broadly and preventing auto-reverts from
occurring when they needed to be. I don't have the specific key-sequence
handy, but it should be clear if you try the code with/without it in
common auto-revert-if-not-visible scenarios.

> Furthermore, I see a problem to call auto-revert functionality in
> files.el. autorevert.el is not dumped into the emacs binary.

I don't understand what you mean. What's the problem?

> Wouldn't it be better to add a function to find-file-hook?

Tried it, and it was insufficient.

> I understand the problem with adding a newline at file end

I don't. AFAIK, it has nothing to do with my patch.

> but maybe this could be solved differently?

Don't know what you mean. What are you suggesting?

Like I mentioned earlier, I need a few days for the Holiday. If you can
test the patch, that would be great.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

Boruch Baum
In reply to this post by Eli Zaretskii
On 2020-09-17 16:13, Eli Zaretskii wrote:

> > Date: Wed, 16 Sep 2020 16:11:04 -0400
> > From: Boruch Baum <[hidden email]>
> > Cc: [hidden email]
> >
> > It looks to me like that's not the case: my testing seems to show that
> > it does catch cases of changing a buffer displayed in a window but does
> > not catch changes of frames due to functions like `other-frame' or
> > `select-frame'.
> >
> > So, the good news is that I've written the code that makes the
> > improvement for the caught cases, and I can submit that.
> >
> > As for the cases of changing frames, a less-desirable option would be to
> > preempt bug-reports by documenting the limitation. Auto-revert already
> > has other curious limitations (eg. for dired buffers it doesn't operate
> > _at__all_ on many types of file changes), and this limitation only
> > introduces a delay, so by comparison its a pretty mild limitation.
>
> If we have no better way, we could document this as a limitation,
> yes.  But let's make one more attempt to solve this.
>
> > A better option would be able to catch frame-change events. I haven't
> > found a straightforward way to trap that. Does such a method exist?
>
> Can you describe the problematic case in more detail?  With that in
> hand, perhaps Martin (CC'ed) could suggest a method.

I'm needing now to focus on Rosh HaShanna preparations, and can let
emacs distract me for the next few days, but from memory, on tty emacs:

1) touch ~/foo

2) find it in emacs

3) bury the frame so it's not human-visible

   3.1) OT: I find it useful to globally bind C-x 5 b to function
        select-frame-by-name, because I do this routinely as part of my
        workflow. Another option might be C-x 5 2.

4) M-! echo "bar" >> ~/foo

5) switch to the buried frame

6) Depending upon your timing, you'll need to wait up to
   auto-revert-interval seconds for the buffer to be updated.

> > An inelegant solution that would cover most of the remaining events
> > would be to advise :after ~4 frame functions, and to add an element to
> > variable `move-frame-functions'.
>
> Yes, I'd prefer to avoid such solutions.

It maybe could all be solved if there were a hook for frame change of
state (eg. getting/losing focus, being raised/buried). It's surprising
to me that such events were never thought significant enough in emacs to
merit a hook.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0



Reply | Threaded
Open this post in threaded view
|

bug#43412: [FEATURE] autorevert-only-if-visible [PATCH]

martin rudalics
In reply to this post by Eli Zaretskii
 >> It looks to me like that's not the case: my testing seems to show that
 >> it does catch cases of changing a buffer displayed in a window but does
 >> not catch changes of frames due to functions like `other-frame' or
 >> `select-frame'.
 >>
 >> So, the good news is that I've written the code that makes the
 >> improvement for the caught cases, and I can submit that.
 >>
 >> As for the cases of changing frames, a less-desirable option would be to
 >> preempt bug-reports by documenting the limitation. Auto-revert already
 >> has other curious limitations (eg. for dired buffers it doesn't operate
 >> _at__all_ on many types of file changes), and this limitation only
 >> introduces a delay, so by comparison its a pretty mild limitation.
 >
 > If we have no better way, we could document this as a limitation,
 > yes.  But let's make one more attempt to solve this.
 >
 >> A better option would be able to catch frame-change events. I haven't
 >> found a straightforward way to trap that. Does such a method exist?
 >
 > Can you describe the problematic case in more detail?  With that in
 > hand, perhaps Martin (CC'ed) could suggest a method.

 From what I've read so far, Baruch wants to react to two events only: A
window displays another buffer and a window gets selected, where the
latter subsumes anything like frame selection or switching, or a frame
getting focus.  The former should be handled by adding a function to
'window-buffer-change-functions'.  The latter should be handled by
adding a function to 'window-selection-change-functions'.

When these added functions (probably it's one and the same function) are
run, one can use 'window-old-buffer', 'frame-old-selected-window',
'old-selected-window' and 'old-selected-frame' to individually check
what has changed since the last time.  For example, to find out whether
a window on the frame reported by 'window-buffer-change-functions' has
changed its buffer, 'walk-window-tree' for that frame with a function
that checks whether 'window-buffer' for any such window differs from
'window-old-buffer'.  If it does differ, you probably want to check
whether that buffer should be reverted.

For 'window-selection-change-functions' you probably want to just check
whether the buffer of the selected window of the reported frame should
be reverted.  I would avoid using 'window-configuration-change-hook'
because that hook also triggers when a window changed its size.  All
hooks are described in detail in section "28.28 Hooks for Window
Scrolling and Changes" of the Elisp manual.  If you have any further
questions, please ask.

martin