bug#25049: ibuffer bug when saving existing filter, with patches

[Tino Calancha <tino.calancha@gmail.com>]

Christopher Genovese <genovese@cmu.edu> writes:

> Thanks, Noa and Tino.
>
> Tino, I noticed that in the patch you just sent in email the '?' on
> old-format-detected was still there.  The rest looked fine
> on first glance, but you might want to edit that.
>
> Alternatively, just in case it helps, I've attached a patch like the others 
> that drops  the help-mode change, changes the b argument to _ to indicate
> unused, and drops the names and dates from before the Change Logs -- while 
> keeping Tino's other suggested changes intact.

Thank you very much.
I have updated the target patch with your latest changes:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From b63c1ab39ad8d1d4edb290707373d769c08b55a9 Mon Sep 17 00:00:00 2001
From: Christopher Genovese <genovese@cmu.edu>
Date: Thu, 1 Dec 2016 11:43:20 +0900
Subject: [PATCH] ibuffer-saved-filters: Remove extra nesting level

* lisp/ibuf-ext.el (ibuffer-saved-filters): Remove extra
nesting level; add transparent setter to adjust old-format values;
update doc string.
(ibuffer-save-filters): Remove extra level of nesting
in ibuffer-saved-filters values when saving new filters (Bug#25049).
(ibuffer-old-saved-filters-warning): New variable with
clickable message with repair options to be displayed
as a warning if 'ibuffer-repair-saved-filters' detects
a format mismatch.
(ibuffer-repair-saved-filters): Add new command to check and
repair saved filters format.
(ibuffer-included-in-filter-p, ibuffer-decompose-filter):
Change access of saved filter data (cadr->cdr) to account
for reduced nesting.
* test/lisp/ibuffer-tests.el (ibuffer-save-filters):
New test; check that filters are saved in the proper format.
---
 lisp/ibuf-ext.el           | 156 +++++++++++++++++++++++++++++++++++----------
 test/lisp/ibuffer-tests.el |  29 +++++++++
 2 files changed, 150 insertions(+), 35 deletions(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index 5ef0746..d1bf576 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -35,7 +35,8 @@
 
 (eval-when-compile
   (require 'ibuf-macs)
-  (require 'cl-lib))
+  (require 'cl-lib)
+  (require 'subr-x))
 
 ;;; Utility functions
 (defun ibuffer-delete-alist (key alist)
@@ -119,32 +120,100 @@ ibuffer-tmp-show-regexps
 
 (defvar ibuffer-auto-buffers-changed nil)
 
-(defcustom ibuffer-saved-filters '(("gnus"
-				    ((or (mode . message-mode)
-					 (mode . mail-mode)
-					 (mode . gnus-group-mode)
-					 (mode . gnus-summary-mode)
-					 (mode . gnus-article-mode))))
-				   ("programming"
-				    ((or (mode . emacs-lisp-mode)
-					 (mode . cperl-mode)
-					 (mode . c-mode)
-					 (mode . java-mode)
-					 (mode . idl-mode)
-					 (mode . lisp-mode)))))
-
-  "An alist of filter qualifiers to switch between.
+(defun ibuffer-update-saved-filters-format (filters)
+  "Transforms alist from old to new `ibuffer-saved-filters' format.
+
+Specifically, converts old-format alist with values of the
+form (STRING (FILTER-SPECS...)) to alist with values of the
+form (STRING FILTER-SPECS...), where each filter spec should be a
+cons cell with a symbol in the car. Any elements in the latter
+form are kept as is.
+
+Returns (OLD-FORMAT-DETECTED . UPDATED-SAVED-FILTERS-LIST)."
+  (when filters
+    (let* ((old-format-detected nil)
+           (fix-filter (lambda (filter-spec)
+                         (if (symbolp (car (cadr filter-spec)))
+                             filter-spec
+                           (setq old-format-detected t) ; side-effect
+                           (cons (car filter-spec) (cadr filter-spec)))))
+           (fixed (mapcar fix-filter filters)))
+      (cons old-format-detected fixed))))
 
-This variable should look like ((\"STRING\" QUALIFIERS)
-                                (\"STRING\" QUALIFIERS) ...), where
-QUALIFIERS is a list of the same form as
-`ibuffer-filtering-qualifiers'.
-See also the variables `ibuffer-filtering-qualifiers',
-`ibuffer-filtering-alist', and the functions
-`ibuffer-switch-to-saved-filters', `ibuffer-save-filters'."
-  :type '(repeat sexp)
+(defcustom ibuffer-saved-filters '(("gnus"
+                                    (or (mode . message-mode)
+                                        (mode . mail-mode)
+                                        (mode . gnus-group-mode)
+                                        (mode . gnus-summary-mode)
+                                        (mode . gnus-article-mode)))
+                                   ("programming"
+                                    (or (mode . emacs-lisp-mode)
+                                        (mode . cperl-mode)
+                                        (mode . c-mode)
+                                        (mode . java-mode)
+                                        (mode . idl-mode)
+                                        (mode . lisp-mode))))
+
+  "An alist mapping saved filter names to filter specifications.
+
+Each element should look like (\"NAME\" . FILTER-LIST), where
+FILTER-LIST has the same structure as the variable
+`ibuffer-filtering-qualifiers', which see. The filters defined
+here are joined with an implicit logical `and' and associated
+with NAME. The combined specification can be used by name in
+other filter specifications via the `saved' qualifier (again, see
+`ibuffer-filtering-qualifiers'). They can also be switched to by
+name (see the functions `ibuffer-switch-to-saved-filters' and
+`ibuffer-save-filters'). The variable `ibuffer-save-with-custom'
+affects how this information is saved for future sessions. This
+variable can be set directly from lisp code."
+  :version "26.1"
+  :type '(alist :key-type (string :tag "Filter name")
+                :value-type (repeat :tag "Filter specification" sexp))
+  :set (lambda (symbol value)
+         ;; Just set-default but update legacy old-style format
+         (set-default symbol (cdr (ibuffer-update-saved-filters-format value))))
   :group 'ibuffer)
 
+(defvar ibuffer-old-saved-filters-warning
+  (concat "Deprecated format detected for variable `ibuffer-saved-filters'.
+
+The format has been repaired and the variable modified accordingly.
+You can save the current value through the customize system by
+either clicking or hitting return "
+          (make-text-button
+           "here" nil
+           'face '(:weight bold :inherit button)
+           'mouse-face '(:weight normal :background "gray50" :inherit button)
+           'follow-link t
+           'help-echo "Click or RET: save new value in customize"
+           'action (lambda (_)
+                     (if (not (fboundp 'customize-save-variable))
+                         (message "Customize not available; value not saved")
+                       (customize-save-variable 'ibuffer-saved-filters
+                                                ibuffer-saved-filters)
+                       (message "Saved updated ibuffer-saved-filters."))))
+          ". See below for
+an explanation and alternative ways to save the repaired value.
+
+Explanation: For the list variable `ibuffer-saved-filters',
+elements of the form (STRING (FILTER-SPECS...)) are deprecated
+and should instead have the form (STRING FILTER-SPECS...), where
+each filter spec is a cons cell with a symbol in the car. See
+`ibuffer-saved-filters' for details. The repaired value fixes
+this format without changing the meaning of the saved filters.
+
+Alternative ways to save the repaired value:
+
+  1. Do M-x customize-variable and entering `ibuffer-saved-filters'
+     when prompted.
+
+  2. Set the updated value manually by copying the
+     following emacs-lisp form to your emacs init file.
+
+%s
+"))
+
 (defvar ibuffer-filtering-qualifiers nil
   "A list like (SYMBOL . QUALIFIER) which filters the current buffer list.
 See also `ibuffer-filtering-alist'.")
@@ -224,6 +293,28 @@ ibuffer-save-with-custom
   :type 'boolean
   :group 'ibuffer)
 
+(defun ibuffer-repair-saved-filters ()
+  "Updates `ibuffer-saved-filters' to its new-style format, if needed.
+
+If this list has any elements of the old-style format, a
+deprecation warning is raised, with a button allowing persistent
+update. Any updated filters retain their meaning in the new
+format. See `ibuffer-update-saved-filters-format' and
+`ibuffer-saved-filters' for details of the old and new formats."
+  (interactive)
+  (when (and (boundp 'ibuffer-saved-filters) ibuffer-saved-filters)
+    (let ((fixed (ibuffer-update-saved-filters-format ibuffer-saved-filters)))
+      (prog1
+          (setq ibuffer-saved-filters (cdr fixed))
+        (when-let (old-format-detected (car fixed))
+          (let ((warning-series t)
+                (updated-form
+                 (with-output-to-string
+                   (pp `(setq ibuffer-saved-filters ',ibuffer-saved-filters)))))
+            (display-warning
+             'ibuffer
+             (format ibuffer-old-saved-filters-warning updated-form))))))))
+
 (defun ibuffer-ext-visible-p (buf all &optional ibuffer-buf)
   (or
    (ibuffer-buf-matches-predicates buf ibuffer-tmp-show-regexps)
@@ -535,13 +626,11 @@ ibuffer-included-in-filter-p-1
 			   (ibuffer-included-in-filter-p buf x))
 		       (cdr filter))))
       (`saved
-       (let ((data
-	      (assoc (cdr filter)
-		     ibuffer-saved-filters)))
+       (let ((data (assoc (cdr filter) ibuffer-saved-filters)))
 	 (unless data
 	   (ibuffer-filter-disable t)
 	   (error "Unknown saved filter %s" (cdr filter)))
-	 (ibuffer-included-in-filters-p buf (cadr data))))
+	 (ibuffer-included-in-filters-p buf (cdr data))))
       (_
        (pcase-let ((`(,_type ,_desc ,func)
                     (assq (car filter) ibuffer-filtering-alist)))
@@ -849,15 +938,12 @@ ibuffer-decompose-filter
 					  (cdr lim)
 					  ibuffer-filtering-qualifiers)))
       (`saved
-       (let ((data
-	      (assoc (cdr lim)
-		     ibuffer-saved-filters)))
+       (let ((data (assoc (cdr lim) ibuffer-saved-filters)))
 	 (unless data
 	   (ibuffer-filter-disable)
 	   (error "Unknown saved filter %s" (cdr lim)))
-	 (setq ibuffer-filtering-qualifiers (append
-					    (cadr data)
-					    ibuffer-filtering-qualifiers))))
+	 (setq ibuffer-filtering-qualifiers
+               (append (cdr data) ibuffer-filtering-qualifiers))))
       (`not
        (push (cdr lim)
 	     ibuffer-filtering-qualifiers))
@@ -936,7 +1022,7 @@ ibuffer-save-filters
       ibuffer-filtering-qualifiers)))
   (ibuffer-aif (assoc name ibuffer-saved-filters)
       (setcdr it filters)
-    (push (list name filters) ibuffer-saved-filters))
+    (push (cons name filters) ibuffer-saved-filters))
   (ibuffer-maybe-save-stuff))
 
 ;;;###autoload
diff --git a/test/lisp/ibuffer-tests.el b/test/lisp/ibuffer-tests.el
index 3a4def3..6d5187a 100644
--- a/test/lisp/ibuffer-tests.el
+++ b/test/lisp/ibuffer-tests.el
@@ -66,5 +66,34 @@
       (mapc (lambda (buf) (when (buffer-live-p buf)
                             (kill-buffer buf))) (list buf1 buf2)))))
 
+(ert-deftest ibuffer-save-filters ()
+  "Tests that `ibuffer-save-filters' saves in the proper format."
+  (skip-unless (featurep 'ibuf-ext))
+  (let ((ibuffer-save-with-custom nil)
+        (ibuffer-saved-filters nil)
+        (test1 '((mode . org-mode)
+                 (or (size-gt . 10000)
+                     (and (not (starred-name))
+                          (directory . "\<org\>")))))
+        (test2 '((or (mode . emacs-lisp-mode) (file-extension . "elc?")
+                     (and (starred-name) (name . "elisp"))
+                     (mode . lisp-interaction-mode))))
+        (test3 '((size-lt . 100) (derived-mode . prog-mode)
+                 (or (filename . "scratch")
+                     (filename . "bonz")
+                     (filename . "temp")))))
+    (ibuffer-save-filters "test1" test1)
+    (should (equal (car ibuffer-saved-filters) (cons "test1" test1)))
+    (ibuffer-save-filters "test2" test2)
+    (should (equal (car ibuffer-saved-filters) (cons "test2" test2)))
+    (should (equal (cadr ibuffer-saved-filters) (cons "test1" test1)))
+    (ibuffer-save-filters "test3" test3)
+    (should (equal (car ibuffer-saved-filters) (cons "test3" test3)))
+    (should (equal (cadr ibuffer-saved-filters) (cons "test2" test2)))
+    (should (equal (car (cddr ibuffer-saved-filters)) (cons "test1" test1)))
+    (should (equal (cdr (assoc "test1" ibuffer-saved-filters)) test1))
+    (should (equal (cdr (assoc "test2" ibuffer-saved-filters)) test2))
+    (should (equal (cdr (assoc "test3" ibuffer-saved-filters)) test3))))
+
 (provide 'ibuffer-tests)
 ;; ibuffer-tests.el ends here
-- 
2.10.2

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
In GNU Emacs 26.0.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.4)
 of 2016-11-30
Repository revision: b63c1ab39ad8d1d4edb290707373d769c08b55a9