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

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

[Allen Li]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-recentf-auto-cleanup-customize-wording.patch --]
[-- Type: text/x-patch, Size: 875 bytes --]

From 6882a948b60c03f19f53597acb92b85f52a41cd0 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sun, 16 Feb 2020 22:59:18 -0800
Subject: [PATCH 1/3] Fix recentf-auto-cleanup customize wording

* lisp/recentf.el (recentf-auto-cleanup): Fix wording
---
 lisp/recentf.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/recentf.el b/lisp/recentf.el
index 27918a9739..94241b00d5 100644
--- a/lisp/recentf.el
+++ b/lisp/recentf.el
@@ -257,7 +257,7 @@ recentf-auto-cleanup
                         :value mode)
                 (const  :tag "Never"
                         :value never)
-                (number :tag "When idle that seconds"
+                (number :tag "When idle after (seconds)"
                         :value 300)
                 (string :tag "At time"
                         :value "11:00pm"))
-- 
2.25.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Clarify-behavior-of-setting-recentf-auto-cleanup-to-.patch --]
[-- Type: text/x-patch, Size: 973 bytes --]

From a47a121c983b9062079ae4aa2e687bcf138dd8a5 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sun, 16 Feb 2020 23:06:02 -0800
Subject: [PATCH 2/3] Clarify behavior of setting recentf-auto-cleanup to
 string

* lisp/recentf.el (recentf-auto-cleanup): Clarify docstring
---
 lisp/recentf.el | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lisp/recentf.el b/lisp/recentf.el
index 94241b00d5..9c4ef21b52 100644
--- a/lisp/recentf.el
+++ b/lisp/recentf.el
@@ -247,6 +247,12 @@ recentf-auto-cleanup
 - A time string
     Cleanup at specified time string, for example at \"11:00pm\".
 
+If a time string is provided, cleanup is only scheduled once each
+time the mode is turned on; it does not repeat daily.
+Furthermore, the time is for the current day.  If it is already
+past the specified time for the day, cleanup happens immediately
+as for `mode'.
+
 Setting this variable directly does not take effect;
 use \\[customize].
 
-- 
2.25.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-recentf-auto-cleanup-repeat-when-set-to-string.patch --]
[-- Type: text/x-patch, Size: 2223 bytes --]

From b1c40cdeac3bf806a140bbc527604163d1818468 Mon Sep 17 00:00:00 2001
From: Allen Li <darkfeline@felesatra.moe>
Date: Sun, 16 Feb 2020 23:15:35 -0800
Subject: [PATCH 3/3] Make recentf-auto-cleanup repeat when set to string

* lisp/recentf.el (recentf-auto-cleanup): Make timer repeat, update docstring
* etc/NEWS: Update news
---
 etc/NEWS        |  7 +++++++
 lisp/recentf.el | 12 +++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1f8e6049a8..b79d4f8fb8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -146,6 +146,13 @@ key             binding
 / v             package-menu-filter-by-version
 / /             package-menu-filter-clear
 
+** recentf
+
+---
+*** 'recentf-auto-cleanup' time string now repeats.
+When 'recentf-auto-cleanup' is set to a time string, it now repeats
+every day, rather than only running once after the mode is turned on.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/recentf.el b/lisp/recentf.el
index 9c4ef21b52..dd8c2135e8 100644
--- a/lisp/recentf.el
+++ b/lisp/recentf.el
@@ -245,13 +245,10 @@ recentf-auto-cleanup
 - A number
     Cleanup each time Emacs has been idle that number of seconds.
 - A time string
-    Cleanup at specified time string, for example at \"11:00pm\".
+    Cleanup at specified time string daily, for example at \"11:00pm\".
 
-If a time string is provided, cleanup is only scheduled once each
-time the mode is turned on; it does not repeat daily.
-Furthermore, the time is for the current day.  If it is already
-past the specified time for the day, cleanup happens immediately
-as for `mode'.
+If a time string is provided and it is already past the specified time
+for the current day, the first cleanup happens immediately as for `mode'.
 
 Setting this variable directly does not take effect;
 use \\[customize].
@@ -377,7 +374,8 @@ recentf-auto-cleanup
              recentf-auto-cleanup t 'recentf-cleanup))
            ((stringp recentf-auto-cleanup)
             (run-at-time
-             recentf-auto-cleanup nil 'recentf-cleanup))))))
+             ;; Repeat every 24 hours.
+             recentf-auto-cleanup (* 24 60 60) 'recentf-cleanup))))))
 \f
 ;;; File functions
 ;;
-- 
2.25.0

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

[Eli Zaretskii]

> From: Allen Li <darkfeline@felesatra.moe>
> 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.

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

[Juanma Barranquero]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

I think you're talking about
commit be9e7056daaf9112afc394fea96fe3fe67b26070.

That code is from David Ponce:

2003-04-27  David Ponce  <david@dponce.com>

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

[-- Attachment #2: Type: text/html, Size: 608 bytes --]

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

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 17 Feb 2020 19:45:24 +0100
> Cc: Allen Li <darkfeline@felesatra.moe>, 39638@debbugs.gnu.org
> 
> I think you're talking about commit be9e7056daaf9112afc394fea96fe3fe67b26070.
> 
> That code is from David Ponce:
> 
> 2003-04-27  David Ponce  <david@dponce.com>
> 
>         * 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)?

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

[Lars Ingebrigtsen]

Allen Li <darkfeline@felesatra.moe> 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

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

[Stefan Kangas]

close 39638 28.1
thanks

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Allen Li <darkfeline@felesatra.moe> 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.