Customizing recentf

Customizing recentf

[Michael Mauger]

I updated my w32 build two days ago to replace a late-June build (via a make 
bootstrap).  The first thing I noticed was that my recentf list of recently 
editted files was missing.  

I use Customize to enable and configure recentf.  I have the following in my 
custom-set-variables:

 '(recentf-mode t)
 '(recentf-save-file "~/.emacs-recentf")

Debugging the issue, it appears that recentf was being enabled before the 
recentf-save-file variable was set.  So it was reading the (non-existent) 
~/.recentf (thus an empty list of recently used files), but when I left Emacs 
it was saving to the file I had customized.

If I changed the recentf-mode line to:

 '(recentf-mode t nil (recentf))

it works as desired.  However, if I customize anything, the line reverts back 
to the original.  Obviously, getting rid of the save-file customization would 
work too.

I dug a little deeper, and saw that activation of minor modes under customize 
should be delayed to address just this issue, but the process of trying to 
figure out what went wrong began to make my brain go "owwwie".  

Anyone else seeing this?  I can provide additional info if it would be helpful.

This is GNU Emacs 22.0.50.1 (i386-mingw-nt5.1.2600)
 of 2006-08-23 on ASSHOLE1

  (Yes, that is the name of my machine ;-) )

-- Michael Mauger

Re: Customizing recentf

[Richard Stallman]

Does the patch below make it work?

We made a change so that minor mode variables don't normally 
force loading of their files for customization purposes,
because minor mode variables in general don't need to do so.
This one appears to need it for specific reasons.
We can fix each such case in this manner.  Maybe that is
a suitable approach.  Does anyone see a problem with it?

*** recentf.el	03 Apr 2006 17:30:03 -0400	1.53
--- recentf.el	28 Aug 2006 04:35:41 -0400	
***************
*** 1324,1329 ****
--- 1324,1330 ----
    :global t
    :group 'recentf
    :keymap recentf-mode-map
+   :require recentf
    (unless (and recentf-mode (recentf-enabled-p))
      (if recentf-mode
          (progn

Re: Customizing recentf

[Stefan Monnier]

> Does the patch below make it work?
> We made a change so that minor mode variables don't normally 
> force loading of their files for customization purposes,
> because minor mode variables in general don't need to do so.
> This one appears to need it for specific reasons.
> We can fix each such case in this manner.  Maybe that is
> a suitable approach.  Does anyone see a problem with it?

I think we should first figure out why this one needs it.  It doesn't
seem particularly special.  There may simply be a bug elsewhere,


        Stefan

Re: Customizing recentf

[Richard Stallman]

    I think we should first figure out why this one needs it.  It doesn't
    seem particularly special.  There may simply be a bug elsewhere,

Well, mmaug already sent an explanation of why it fails.
Do you see a bug in the chain of events he described?

Re: Customizing recentf

[Stefan Monnier]

>     I think we should first figure out why this one needs it.  It doesn't
>     seem particularly special.  There may simply be a bug elsewhere,

> Well, mmaug already sent an explanation of why it fails.
> Do you see a bug in the chain of events he described?

Not really, but the chain is incomplete.  See the part where he says "owwie":

>> I dug a little deeper, and saw that activation of minor modes under
>> customize should be delayed to address just this issue, but the process
>> of trying to  figure out what went wrong began to make my brain go
>> "owwwie".  


-- Stefan

Re: Customizing recentf

[Michael Mauger]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> 
> >     I think we should first figure out why this one needs it.  It doesn't
> >     seem particularly special.  There may simply be a bug elsewhere,
> 
> > Well, mmaug already sent an explanation of why it fails.
> > Do you see a bug in the chain of events he described?
> 
> Not really, but the chain is incomplete.  See the part where he says "owwie":
> 
> >> I dug a little deeper, and saw that activation of minor modes under
> >> customize should be delayed to address just this issue, but the process
> >> of trying to  figure out what went wrong began to make my brain go
> >> "owwwie".  
> 

Okay, I think I've figured out what is happening, not sure how best to fix it...

This is probably not a problem with just recentf.  I'm seeing a problem only 
because this is a minor mode which hasn't been loaded yet and there are mode 
related settings that are alphabetically sorted after the minor mode variable 
name.  On top of that, this particular mode uses the later variable when it 
starts.  My guess is that there are other minor modes out there with similar 
problems but which may be far more subtle and finding them is a challenge.

Some background:

* the define-minor-mode of recentf-mode is Autoloaded
* minor mode variables or variables having a require list (the 4th element in 
the custom-set-varaiables entry) are moved to the end of the list to be set.

RMS solution was to force the creation of the 4th element in the custom-set-
variables entry by adding the :require recentf property to the define-minor-
mode command.  When custom is next updated, "nil (recentf)" is added to the 
entry automatically.  This fixes recentf but doesn't address the underlying 
problem.

The issue is that when custom-set-variable checks to see if a symbol is a minor-
mode, it looks at the custom-set property which isn't set until the module is 
loaded.  Once the module has been loaded, custom properly detects that recentf-
mode is a minor mode variable.

If I look at the plist of recentf-mode prior to the custom-set-variable, it 
looks like:

  (variable-documentation 1424541 
   custom-autoload t 
   custom-loads ("recentf"))

The custom-autoload and custom-loads properties are used once the c-s-v entries 
have been sorted, what I'm wondering is, if processing these before the sort 
makes sense, so that the optional minor mode modules get loaded before checking 
their properties?  

Here's an attempt (basically I moved the code to autoload out of the main loop 
and put it before the sort).  This did fix the problem for me and would seem to 
fix the problem for other, more subtle, situations.  I didn't move the code to 
handle explicit requires in the c-s-v entries since I figured that should be 
reserved for very special situations.  This patch should allow us to remove 
most of the :require settings in autoloaded minor modes.

*** emacs/lisp/custom.el.orig   Wed Aug 23 20:21:04 2006
--- emacs/lisp/custom.el        Tue Aug 29 18:56:36 2006
***************
*** 874,879 ****
--- 874,891 ----
  EXP itself is saved unevaluated as SYMBOL property `saved-value' and
  in SYMBOL's list property `theme-value' \(using `custom-push-theme')."
    (custom-check-theme theme)
+ 
+   ;; Load any modules referred to by symbols in the set-variable list
+   ;; This allows us to detect minor modes even if they hadn't been
+   ;; loaded yet.
+   (dolist (entry args)
+     (let* ((symbol (indirect-variable (nth 0 entry))))
+       (unless (or (get symbol 'standard-value)
+                 (memq (get symbol 'custom-autoload) '(nil noset)))
+       ;; This symbol needs to be autoloaded, even just for a `set'.
+       (custom-load-symbol symbol))))
+ 
+   ;; Move minor modes and variables with explicit requires to the end
    (setq args
        (sort args
              (lambda (a1 a2)
***************
*** 904,913 ****
            (when requests
              (put symbol 'custom-requests requests)
              (mapc 'require requests))
-             (unless (or (get symbol 'standard-value)
-                         (memq (get symbol 'custom-autoload) '(nil noset)))
-               ;; This symbol needs to be autoloaded, even just for a `set'.
-               (custom-load-symbol symbol))
            (setq set (or (get symbol 'custom-set) 'custom-set-default))
            (put symbol 'saved-value (list value))
            (put symbol 'saved-variable-comment comment)
--- 916,921 ----

Re: Customizing recentf

[Michael Mauger]

Michael Mauger writes:

> Here's an attempt ...

FYI, in my own configuration, it caused the following minor modes to be 
recognized and moved to the end of the set-variables list as opposed to without 
the patch:

*  show-paren-mode
*  global-hl-line-mode
*  global-auto-revert-mode
*  display-time-mode
*  auto-insert-mode

Now, none of these minor modes may exhibit the same issue as recentf re: the 
sort order of mode related variables, but you never know...

Re: Customizing recentf

[Stefan Monnier]

> The custom-autoload and custom-loads properties are used once the c-s-v
> entries  have been sorted, what I'm wondering is, if processing these
> before the sort  makes sense, so that the optional minor mode modules get
> loaded before checking  their properties?  

That would make perfect sense, yes.

> Here's an attempt (basically I moved the code to autoload out of the main
> loop  and put it before the sort).  This did fix the problem for me and
> would seem to  fix the problem for other, more subtle, situations.
> I didn't move the code to  handle explicit requires in the c-s-v entries
> since I figured that should be  reserved for very special situations.
> This patch should allow us to remove  most of the :require settings in
> autoloaded minor modes.

Looks good, thank you.


        Stefan

Re: Customizing recentf

[Richard Stallman]

I think you have found a good fix.  Thank you.
Would someone please install it in a few days
if no problem has been found?

Re: Customizing recentf

[Stefan Monnier]

> Here's an attempt (basically I moved the code to autoload out of the main
> loop  and put it before the sort).  This did fix the problem for me and
> would seem to  fix the problem for other, more subtle, situations.
> I didn't move the code to  handle explicit requires in the c-s-v entries
> since I figured that should be  reserved for very special situations.
> This patch should allow us to remove  most of the :require settings in
> autoloaded minor modes.

Installed,


        Stefan