Re: Ibuffer improvements: filtering, documentation, bug fix, tests

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

Christopher Genovese <genovese@cmu.edu> writes:

> I'd like to submit some mild changes in Ibuffer (ibuffer.el, ibuf-ext.el,
> and ibuffer-tests.el)

> The proposed changes are as follows:
>
>   + Compound filters
>
>     Add support for 'and' and normalize handling of 'not' to allow the
>     original "spliced" format as well as a more lispy "sexp" format.
>
>     Original documentation for the structure of compound filters was
>     almost completely lacking. The updated code documents compound
>     filter structure and clarifies the language used throughout,
>     providing a single authoritative source for documentation on each
>     concept.
>
>     Fixed bug in 'saved' filter handling. There was an inconsistency in
>     how the data was accessed at different points that would cause
>     failure. (I do wonder if anyone ever uses saved filters based on
>     this.) There are two choices in how to fix this; I made one but am
>     open to both.
>
>   + New pre-defined filters and an interactive filtering command
>
>     Several new filters are defined by default to handle some very
>     common filtering tasks (e.g., matching filename components since
>     the 'filename' filter matches on the absolute pathname). A new
>     command is offered to select a filter by completion on the
>     descriptions, which is very easy to use without remembering key
>     bindings.
>
>   + Documentation fixes throughout ibuf-ext.el
>
>   + Many new tests and fixed bug in original test.
>
Hi Chris,

thank you very much for your time preparing this patch!
I have some comments.

I)

ibuffer-filter-by-filename-extension

I would call this:
ibuffer-filter-by-extension
or
ibuffer-filter-by-file-extension

II)
*)
ibuffer-filter-by-filename-root

i don't think this deserves a separated keybinding.  Most of
the time you will be well served with
`ibuffer-filter-by-filename-base'.
Actually, I wouldn't introduce `ibuffer-filter-by-filename-root' at all.

You mention somewhere in the patch:
;; This should probably be called pathname but kept for backward compatibility
The word 'filename' is right; in Emacs it's standard to refer as filename to the
_full_ name of the file.

*)
ibuffer-filter-by-filename-base
I would call this:
ibuffer-filter-by-basename

But what i would do, instead of defining this command and binding it
to '/ F' i would define instead:

(define-ibuffer-filter buffer-name
    "Limit current view to buffers with its name matching QUALIFIER."
  (:description "buffer name"
   :reader (read-from-minibuffer
            "Filter by buffer name (regex): "))
  (string-match qualifier (buffer-name buf)))

And i would bind it to '/ b'.
This has the advantage that it would match any buffers not just those
visiting a file on disk.

*)
I like the new command `ibuffer-filter-chosen-by-completion', and
i think your proposal of binding it to '/ TAB' is a good choice; the
other command previously bound to '/ TAB' its also bound to '/ t', so
this change seems for better.

Similar thoughs applies to binding `ibuffer-filter-by-filename-directory'
to '/ /'; this is consistent with `ibuffer-mark-dired-buffers' ('* /').
Your alternative binding for `ibuffer-filter-disable' ('/ DEL')
is easy to remember.

That said, reassign key bindings is usually a matter of concern.
It might be people get used to '/ TAB' and '/ /' standing for their
current bindings.  It must be a consensus before changing any long
standing key bindings.
Alternatively, we could bind `ibuffer-mark-dired-buffers' to '/ d'.

III)
You use a macro from subr-x.el in ibuf-ext.el, so you need to:
(eval-when-compile (require 'subr-x))

IV) There are several trailing white spaces in your patch.

V) Your commit message  don't follow the Emacs standards.  For instance,
instead of:
* lisp/ibuf-ext.el: added paragraph to file commentary
* lisp/ibuf-ext.el (ibuffer-saved-filters): clarified documentation,
  specified customization type, and simplified data format to be
  consistent with `ibuffer-save-filters'
* lisp/ibuf-ext.el (ibuffer-update-saved-filters-format): new function
  that transforms `ibuffer-saved-filters'-style alist format

I should read:
* lisp/ibuf-ext.el: added paragraph to file commentary
(ibuffer-saved-filters): clarified documentation,
specified customization type, and simplified data format to be
consistent with `ibuffer-save-filters'.
(ibuffer-update-saved-filters-format): new function
that transforms `ibuffer-saved-filters'-style alist format.

that is: End sentences with a period.  Write the modified file
just one.

You might want to write NEWS entry for the new features.

VI)
I would change the wording in `ibuffer-included-in-filters-p' doc string.
Instead of
"Does the buffer BUF successfully pass all of the given FILTERS?"
someting like:
"Return non-nil if BUF pass all FILTERS."

VII)
Once you add `ibuffer-and-filter' there is code duplication with
`ibuffer-or-filter'.  I would extract the common code in a new
auxiliar function `ibuffer--or-and-filter' as follows:

(defun ibuffer--or-and-filter (op arg)
  (if arg
      (progn
        (when (or (null ibuffer-filtering-qualifiers)
                  (not (eq op (caar ibuffer-filtering-qualifiers))))
          (error "Top filter is not an %s" (upcase (symbol-name op))))
        (let ((lim (pop ibuffer-filtering-qualifiers)))
          (setq ibuffer-filtering-qualifiers
                (nconc (cdr lim) ibuffer-filtering-qualifiers))))
    (when (< (length ibuffer-filtering-qualifiers) 2)
      (error "Need two filters to %s" (upcase (symbol-name op))))
    ;; If the second filter is an op, just add to it.
    (let ((first (pop ibuffer-filtering-qualifiers))
          (second (pop ibuffer-filtering-qualifiers)))
      (if (eq op (car second))
          (push (nconc (list op first) (cdr second))
                ibuffer-filtering-qualifiers)
        (push (list op first second)
              ibuffer-filtering-qualifiers))))
  (ibuffer-update nil t))

;;;###autoload
(defun ibuffer-or-filter (&optional reverse)
  "Replace the top two filters in this buffer with their logical OR.
If optional argument REVERSE is non-nil, instead break the top OR
filter into parts."
  (interactive "P")
  (ibuffer--or-and-filter 'or reverse))

;;;###autoload
(defun ibuffer-and-filter (&optional decompose)
  "Replace the top two filters in this buffer with their logical AND.
If optional argument DECOMPOSE is non-nil, instead break the top AND
filter into parts."
  (interactive "P")
  (ibuffer--or-and-filter 'and decompose))

IX)
In `ibuffer-filter-by-starred-name' you are matching a buffer name
starting with "*".  That covers all special buffers but it might
add some garbage.  For instance, sometimes i miss-type a new buffer
"*foo", and then i just make a new one "*foo*" without deleting
"*foo".  I prefer if the filter do not show "*foo".
I use the following more paranoid regexp:
"\\`\\*[^*]+\\*\\(<?[[:digit:]]*>?\\)\\'"
This regexp matches "*foo*" and "*foo*<2>" but it doesn't match neither
"*foo" nor "foo*".

X)
>    Fixed bug in 'saved' filter handling. There was an inconsistency in
>    how the data was accessed at different points that would cause
>    failure. (I do wonder if anyone ever uses saved filters based on
>    this.) There are two choices in how to fix this; I made one but am
>    open to both.
Could you create a receipt where the bug cause an actual failure?

Even if there is no failure i agree it looks nicer because you decrease
1 level the nesting, and make `ibuffer-saved-filters' looks similar than
`ibuffer-saved-filter-groups'.  That is an advantage when the user is
writing filters by hand; but usually an user compose the filters from Ibuffer,
and save them with '/ s', so the actual format is an implementation detail.

As you know, we have an implicit 'AND', that is the reason why the original
implemention lack of an explicit `and'.  I don't object to the new format, though.
I agree is more clear when writing filters by hand.

I much prefer if this part of the patch go to a separated bug report.

Cheers,
Tino