bug#39638: 26.3; recentf-auto-cleanup deceptive

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

bug#39638: 26.3; recentf-auto-cleanup deceptive

Allen Li-2
This covers two bugs around recentf-auto-cleanup being
deceptive/unintuitive.

Both bugs are present at 26.3 and on Emacs master as of
556cc727e5076d590f8286406e4f46cff3cee41e
at Sun, 16 Feb 2020 11:37:07 -0800

1. When setting recentf-auto-cleanup to a string, the timer does not
repeat.  It is only set once.  This is in contrast to midnight-mode,
which repeats its timer every day.  The documentation for
recentf-auto-cleanup does not make this clear, and I'm not even sure if
this was the intended behavior.

(defun recentf-auto-cleanup ()
  "Automatic cleanup of the recent list."
  (when (timerp recentf-auto-cleanup-timer)
    (cancel-timer recentf-auto-cleanup-timer))
  (when recentf-mode
    (setq recentf-auto-cleanup-timer
          (cond
           ;; snipped
           ((stringp recentf-auto-cleanup)
            (run-at-time
             recentf-auto-cleanup nil 'recentf-cleanup))))))

2. Due to the behavior of run-at-time, if the time string set was in the
past for today, recentf-cleanup runs immediately when recentf-mode is
turned on (e.g., at Emacs startup).  This makes it pointless to set it
to something like "3:00am" if I want recentf-cleanup to run at a time
when I'm likely not using Emacs and I have also set
recentf-max-saved-items to something large like 2000.  The docstring
does not make this obvious.  This is also how one would usually
customize midnight-mode.

2a. midnight-mode suffers from the same problem of using run-at-time,
but the default behavior of midnight-mode does not make it expensive.
But this means that adding recentf-cleanup to midnight-hook when using a large
recentf-max-saved-items will still be expensive at startup.

I have attached a number of patches:

1. Simply fix some awkward wording that is not directly related to this bug.
2. Document the current behavior.
3. Make recentf-auto-cleanup repeat for time strings.

The third patch can be skipped if deemed too aggressive, but I think
that's the more reasonable behavior to expect.

I have not fixed the problem of recentf-cleanup running immediately if
the time is in the past for today, since I'm not sure the best way to do
it.

In GNU Emacs 26.3 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.10)
 of 2019-08-29 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12007000
System Description: Arch Linux


0001-Fix-recentf-auto-cleanup-customize-wording.patch (901 bytes) Download Attachment
0002-Clarify-behavior-of-setting-recentf-auto-cleanup-to-.patch (1004 bytes) Download Attachment
0003-Make-recentf-auto-cleanup-repeat-when-set-to-string.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#39638: 26.3; recentf-auto-cleanup deceptive

Eli Zaretskii
> From: Allen Li <[hidden email]>
> Date: Sun, 16 Feb 2020 23:33:36 -0800
>
> This covers two bugs around recentf-auto-cleanup being
> deceptive/unintuitive.
>
> Both bugs are present at 26.3 and on Emacs master as of
> 556cc727e5076d590f8286406e4f46cff3cee41e
> at Sun, 16 Feb 2020 11:37:07 -0800
>
> 1. When setting recentf-auto-cleanup to a string, the timer does not
> repeat.  It is only set once.  This is in contrast to midnight-mode,
> which repeats its timer every day.  The documentation for
> recentf-auto-cleanup does not make this clear, and I'm not even sure if
> this was the intended behavior.
>
> (defun recentf-auto-cleanup ()
>   "Automatic cleanup of the recent list."
>   (when (timerp recentf-auto-cleanup-timer)
>     (cancel-timer recentf-auto-cleanup-timer))
>   (when recentf-mode
>     (setq recentf-auto-cleanup-timer
>           (cond
>            ;; snipped
>            ((stringp recentf-auto-cleanup)
>             (run-at-time
>              recentf-auto-cleanup nil 'recentf-cleanup))))))
>
> 2. Due to the behavior of run-at-time, if the time string set was in the
> past for today, recentf-cleanup runs immediately when recentf-mode is
> turned on (e.g., at Emacs startup).  This makes it pointless to set it
> to something like "3:00am" if I want recentf-cleanup to run at a time
> when I'm likely not using Emacs and I have also set
> recentf-max-saved-items to something large like 2000.  The docstring
> does not make this obvious.  This is also how one would usually
> customize midnight-mode.
>
> 2a. midnight-mode suffers from the same problem of using run-at-time,
> but the default behavior of midnight-mode does not make it expensive.
> But this means that adding recentf-cleanup to midnight-hook when using a large
> recentf-max-saved-items will still be expensive at startup.
>
> I have attached a number of patches:
>
> 1. Simply fix some awkward wording that is not directly related to this bug.
> 2. Document the current behavior.
> 3. Make recentf-auto-cleanup repeat for time strings.
>
> The third patch can be skipped if deemed too aggressive, but I think
> that's the more reasonable behavior to expect.
>
> I have not fixed the problem of recentf-cleanup running immediately if
> the time is in the past for today, since I'm not sure the best way to do
> it.

Juanma, any comments?  Did you indeed mean for the cleanup feature to
work as it does, or are those omissions?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#39638: 26.3; recentf-auto-cleanup deceptive

Juanma Barranquero
I think you're talking about commit be9e7056daaf9112afc394fea96fe3fe67b26070.

That code is from David Ponce:

2003-04-27  David Ponce  <[hidden email]>

        * recentf.el

        Major rewrite.  The code is reordered, cleaner and faster.
        Introduce new options to automatically cleanup the recent list,
        and to handle filename transformation (for example to use true
        filenames).

I imagine I did commit it in the old CVS.
Reply | Threaded
Open this post in threaded view
|

bug#39638: 26.3; recentf-auto-cleanup deceptive

Eli Zaretskii
> From: Juanma Barranquero <[hidden email]>
> Date: Mon, 17 Feb 2020 19:45:24 +0100
> Cc: Allen Li <[hidden email]>, [hidden email]
>
> I think you're talking about commit be9e7056daaf9112afc394fea96fe3fe67b26070.
>
> That code is from David Ponce:
>
> 2003-04-27  David Ponce  <[hidden email]>
>
>         * recentf.el
>
>         Major rewrite.  The code is reordered, cleaner and faster.
>         Introduce new options to automatically cleanup the recent list,
>         and to handle filename transformation (for example to use true
>         filenames).

Oops, sorry.  Right you are.

David, would you please answer my question (and in general comment on
the proposed changes)?



Reply | Threaded
Open this post in threaded view
|

bug#39638: 26.3; recentf-auto-cleanup deceptive

Lars Ingebrigtsen
In reply to this post by Allen Li-2
Allen Li <[hidden email]> writes:

> 1. When setting recentf-auto-cleanup to a string, the timer does not
> repeat.  It is only set once.  This is in contrast to midnight-mode,
> which repeats its timer every day.  The documentation for
> recentf-auto-cleanup does not make this clear, and I'm not even sure if
> this was the intended behavior.

I think repeating the action makes a whole lot more sense, and since
the action wasn't spelled out before (and no response from the person
who originally committed the code), I've applied your patch to Emacs 28.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#39638: 26.3; recentf-auto-cleanup deceptive

Stefan Kangas
close 39638 28.1
thanks

Lars Ingebrigtsen <[hidden email]> writes:

> Allen Li <[hidden email]> writes:
>
>> 1. When setting recentf-auto-cleanup to a string, the timer does not
>> repeat.  It is only set once.  This is in contrast to midnight-mode,
>> which repeats its timer every day.  The documentation for
>> recentf-auto-cleanup does not make this clear, and I'm not even sure if
>> this was the intended behavior.
>
> I think repeating the action makes a whole lot more sense, and since
> the action wasn't spelled out before (and no response from the person
> who originally committed the code), I've applied your patch to Emacs 28.

I'm therefore closing this bug report.