unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#25049: ibuffer bug when saving existing filter, with patches
@ 2016-11-28  6:54 Christopher Genovese
  2016-11-29 15:06 ` Tino Calancha
  0 siblings, 1 reply; 12+ messages in thread
From: Christopher Genovese @ 2016-11-28  6:54 UTC (permalink / raw)
  To: 25049; +Cc: Tino Calancha


[-- Attachment #1.1: Type: text/plain, Size: 2986 bytes --]

Hello,

In at least Emacs 25+, the function 'ibuffer-save-filters'
handles saving existing filters and new filters inconsistently.
Specifically, at the following point in the original function:

  (ibuffer-aif (assoc name ibuffer-saved-filters)
      (setcdr it filters)
    (push (list name filters) ibuffer-saved-filters))

the setcdr (existing filters) and the push (new filters)
save different formats for the variable, with the latter
having an extra layer of parentheses.

As a specific example of failure, using the current default
value of '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)))))

and doing

   (ibuffer-save-filters "foo" '((name . "foo") (derived-mode . text-mode)))
   (ibuffer-save-filters "gnus" '((filename . ".")
                                  (or (derived-mode . prog-mode)
                                      (mode . "compilation-mode"))))

gives the following incorrect value for `ibuffer-saved-filters'

        (("foo"
          ((name . "foo")
           (derived-mode . text-mode)))
         ("gnus"
          (filename . ".")
          (or
           (derived-mode . prog-mode)
           (mode . "compilation-mode")))
         ("programming"
          ((or
            (mode . emacs-lisp-mode)
            (mode . cperl-mode)
            (mode . c-mode)
            (mode . java-mode)
            (mode . idl-mode)
            (mode . lisp-mode)))))

because the "foo" and "gnus" entries have different formats,
the latter not consistent with later code to access it
(e.g., in 'ibuffer-included-in-filter-p-1' and 'ibuffer-decompose-filter').

There are two immediate paths for fixing this:

  1. Change the setcdr to add the extra level of nesting.
  2. Change the format of 'ibuffer-saved-filters' to remove
     the level of testing, change the push (list->cons) and
     the later accesses (cadr->cdr).

The first is very simple, but the extra level of nesting
is unsightly, inconsistent with the structure of filter groups,
and mildly annoying when writing filters by hand.  The
second is fairly simple, requiring only a few more small changes,
but introduces the complication of changing an existing
variable's format. (For what it's worth, I prefer the second.)

I have attached a patch file with patches for three commits
as follows:

  1/3: The very simple fix corresponding to choice 1 above.
  2/3: The simple fix corresponding to choice 2 above.
  3/3: Some extra support added for handling the change
       in format for 'ibuffer-saved-filters'.

Let me know if you need any further information.

Thanks, Chris

[-- Attachment #1.2: Type: text/html, Size: 3920 bytes --]

[-- Attachment #2: ibuffer-saved-filters-bug.patch --]
[-- Type: text/x-diff, Size: 17727 bytes --]

From 01e41027abf27f8fca01f7e4b6fead55c032b1ed Mon Sep 17 00:00:00 2001
From: "Christopher R. Genovese" <genovese@cmu.edu>
Date: Sun, 27 Nov 2016 23:34:50 -0500
Subject: [PATCH 1/3] ibuffer: one possible fix for bug when saving existing
 filter

The function 'ibuffer-save-filters' handles saving existing
filters and new filters inconsistently. Specifically,
at the following point in the original function:

  (ibuffer-aif (assoc name ibuffer-saved-filters)
      (setcdr it filters)
    (push (list name filters) ibuffer-saved-filters))

the setcdr (existing filters) and the push (new filters)
save different formats for the variable, with the latter
having an extra layer of parentheses.

As a specific example of failure, using the current default
value of '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)))))

and doing

   (ibuffer-save-filters "foo" '((name . "foo") (derived-mode . text-mode)))
   (ibuffer-save-filters "gnus" '((filename . ".")
                                  (or (derived-mode . prog-mode)
                                      (mode . "compilation-mode"))))

gives the following incorrect value for `ibuffer-saved-filters'

        (("foo"
          ((name . "foo")
           (derived-mode . text-mode)))
         ("gnus"
          (filename . ".")
          (or
           (derived-mode . prog-mode)
           (mode . "compilation-mode")))
         ("programming"
          ((or
            (mode . emacs-lisp-mode)
            (mode . cperl-mode)
            (mode . c-mode)
            (mode . java-mode)
            (mode . idl-mode)
            (mode . lisp-mode)))))

because the "foo" and "gnus" entries have different formats,
the latter not consistent with later code to access it
(e.g., in 'ibuffer-included-in-filter-p-1' and 'ibuffer-decompose-filter').

There are two immediate paths for fixing this:

  1. Change the setcdr to add the extra level of nesting.
  2. Change the format of 'ibuffer-saved-filters' to remove
     the level of testing, change the push (list->cons) and
     the later accesses (cadr->cdr).

The first is very simple, but the extra level of nesting
is unsightly, inconsistent with the structure of filter groups,
and mildly annoying when writing filters by hand.  The
second is fairly simple, requiring only a few more small changes,
but introduces the complication of changing an existing
variable's format. For what it's worth, I prefer the second.

This commit takes the first choice.

Change Log:

2016-11-27 Christopher R. Genovese  <genovese@cmu.edu>

* lisp/ibuf-ext.el (ibuffer-save-filters): added extra
level of nesting when saving filter to an existing name.
---
 lisp/ibuf-ext.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index 5ef0746..103232c 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -935,7 +935,7 @@ Interactively, prompt for NAME, and use the current filters."
       (read-from-minibuffer "Save current filters as: ")
       ibuffer-filtering-qualifiers)))
   (ibuffer-aif (assoc name ibuffer-saved-filters)
-      (setcdr it filters)
+      (setcdr it (list filters))
     (push (list name filters) ibuffer-saved-filters))
   (ibuffer-maybe-save-stuff))
 
-- 
2.10.0


From 0791820e0ff3665b447ea174b4b915a36dc3eced Mon Sep 17 00:00:00 2001
From: "Christopher R. Genovese" <genovese@cmu.edu>
Date: Mon, 28 Nov 2016 00:33:36 -0500
Subject: [PATCH 2/3] ibuffer: another fix for bug when saving an existing
 filter

As described in the previous commit, the function 'ibuffer-save-filters'
handles saving existing filters and new filters inconsistently,
and there are two paths to fixing the problem.

The previous commit fixed the problem while maintaining the format of
the variable 'ibuffer-saved-filters' with its extra level of nesting.

This commit instead takes the second path: removing the extra level of
nesting from the saved filter format.

Change Log:

2016-11-27 Christopher R. Genovese  <genovese@cmu.edu>

* lisp/ibuf-ext.el (ibuffer-saved-filters): removed extra
nesting level in the alist values and updated docstring.
(ibuffer-save-filters): removed extra level of nesting
in saved filter alist values when saving new filters.
(ibuffer-included-in-filter-p): changed access of saved
filter data (cadr->cdr) to account for reduced nesting.
(ibuffer-decompose-filter): changed access of saved
filter data (cadr->cdr) to account for reduced nesting.
---
 lisp/ibuf-ext.el | 67 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index 103232c..e4a7dfa 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -120,28 +120,32 @@ Buffers whose major mode is in this list, are not searched."
 (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.
-
-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'."
+                                    (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."
   :type '(repeat sexp)
   :group 'ibuffer)
 
@@ -535,13 +539,11 @@ To evaluate a form without viewing the buffer, see `ibuffer-do-eval'."
 			   (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 +851,12 @@ turned into two separate filters [name: foo] and [mode: bar-mode]."
 					  (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))
@@ -935,8 +934,8 @@ Interactively, prompt for NAME, and use the current filters."
       (read-from-minibuffer "Save current filters as: ")
       ibuffer-filtering-qualifiers)))
   (ibuffer-aif (assoc name ibuffer-saved-filters)
-      (setcdr it (list filters))
-    (push (list name filters) ibuffer-saved-filters))
+      (setcdr it filters)
+    (push (cons name filters) ibuffer-saved-filters))
   (ibuffer-maybe-save-stuff))
 
 ;;;###autoload
-- 
2.10.0


From aee1c3cdd47db9c0da2a2f9d1c3a53c40a81cfde Mon Sep 17 00:00:00 2001
From: "Christopher R. Genovese" <genovese@cmu.edu>
Date: Mon, 28 Nov 2016 01:29:04 -0500
Subject: [PATCH 3/3] ibuffer: added support for saved filter format change and
 test

As described in the previous commit, one fix to the
inconsistency in 'ibuffer-save-filters' involves simplifying
the format of 'ibuffer-saved-filters' to reduce the extra
nesting level. This adds some support for this transition,
including a customize setter to transparently update old
format values and a command to check and repair the saved
values if desired.

Also added a test of 'ibuffer-save-filter'.

Change Log:

2016-11-27 Christopher R. Genovese  <genovese@cmu.edu>

* lisp/ibuf-ext.el (ibuffer-saved-filters): added more accurate
customization type and transparent setter to adjust old-format
values.
(ibuffer-update-saved-filters-format): updates old-format
for saved buffer data to new format with reduced nesting level.
(ibuffer-repair-saved-filters): new command to check and
repair saved filters format.
(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.
* test/lisp/ibuffer-tests.el (ibuffer-save-filters): added
a test that filters are saved in the proper format.
---
 lisp/ibuf-ext.el           | 87 +++++++++++++++++++++++++++++++++++++++++++++-
 test/lisp/ibuffer-tests.el | 29 ++++++++++++++++
 2 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index e4a7dfa..1d79729 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -119,6 +119,26 @@ Buffers whose major mode is in this list, are not searched."
 
 (defvar ibuffer-auto-buffers-changed nil)
 
+(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))))
+
 (defcustom ibuffer-saved-filters '(("gnus"
                                     (or (mode . message-mode)
                                         (mode . mail-mode)
@@ -146,9 +166,74 @@ 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."
-  :type '(repeat sexp)
+  :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 (b)
+                     (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
+"))
+
+(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))))))))
+
 (defvar ibuffer-filtering-qualifiers nil
   "A list like (SYMBOL . QUALIFIER) which filters the current buffer list.
 See also `ibuffer-filtering-alist'.")
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.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-12-07 10:56 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-28  6:54 bug#25049: ibuffer bug when saving existing filter, with patches Christopher Genovese
2016-11-29 15:06 ` Tino Calancha
2016-11-29 16:09   ` Christopher Genovese
2016-11-29 23:49     ` Tino Calancha
2016-11-30  5:14       ` Christopher Genovese
2016-11-30  7:02         ` Tino Calancha
2016-11-30 14:07           ` npostavs
2016-11-30 15:03             ` Tino Calancha
2016-11-30 15:11               ` Tino Calancha
2016-11-30 16:31                 ` Christopher Genovese
2016-12-01  2:52                   ` Tino Calancha
2016-12-07 10:56           ` Tino Calancha

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).