Hi Tino,

Thanks so much for your detailed and very helpful comments.

I've made almost all your suggested changes, and for the few
exceptions, I changed the code in the direction I think you
intended.

Below, I give specific responses to each of your points.
Please take a look. There are a few questions/points for
your consideration therein.

I haven't had a chance yet to split the commit to isolate the
saved filters fix, but I will do that tomorrow and submit
appropriate patches and bug reports. (Below, I do describe the
bug more precisely and give an example.)

I still thought it would be useful to respond to your
suggestions now. I've attached a patch file of the most recent
commit on my branch against the current master for reference.

Thanks again for all your feedback and help!

   Regards, Chris


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

I agree that those are simpler. I had gone with the more complicated
name to make clear that also filters on whether the buffer is visiting a
file. But that is also clear with the simpler name.

I've used file-extension to make clear that this applies to file buffers
only. By related reasoning, I've also changed filename-directory and
filename-base to directory and basename; see the note below in
response to your basename comment for how I've handled these in the
revision.


> ibuffer-filter-by-filename-root

I have eliminated this as you suggested.


> You mention somewhere in the patch:
> ;; This should probably be called pathname but kept for backward
compatibility
> The word 'filename' is right...

Good point. I've eliminated this comment.


> 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:

I see your point, but I still think there are good reasons to keep this
one. This filter is useful to prevent inadvertent matching both against
random parts of a file's path and against utility (non-file) buffers
with systematically related names. I think this is a fairly common use
case. (For me, this is one of the filters I use most often
interactively.) Moreover, the buffer name and the buffer file name need
not be the same (e.g., with uniquify/ multiple files of the same name,
or the edge case of explicit renaming).

The buffer name filter you suggest is already available as
ibuffer-filter-by-name (/ n).

Here's what I've done on this:

 1. Add a `ibuffer-filter-by-visiting-file' (/ v) that selects
    buffers that are visiting a file. This is useful in its own
    right (see next item). It also makes '/ n' + '/ v' [that is,
    (and (name . "...") (visiting-file))] almost the same as
    my '/ F', or put another way, makes '/ F' a more precise
    shortcut.

 2. Changed `ibuffer-filter-by-filename-directory' to
    `ibuffer-filter-by-directory' and changed the functionality so that
    in a file buffer it matches against the file's path but in a
    non-file buffer matches against default-directory. This is of
    practical interest and '/ /' + '/ v'
    [that is, (and (directory . "...") (visiting-file))] gives the original
    functionality quite simply.

 3. Keep the `ibuffer-filter-by-basename', making the name
    change you suggested and keeping it on '/ F'.  It does
    no harm here and, I think, adds some value.

Let me know what you think.


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

Understood.

I think the new bindings are highly mnemonic and will happily advocate
for them. But the need for consensus makes total sense.

(Note: '/ d' is already bound to ibuffer-decompose-filter or I would have
used it. I felt that the change I made keeps the mnemonic strong with
less overall impact -- what's a better match for decompose?, for instance.)


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

Done. Good catch!


> There are several trailing white spaces in your patch.

Fixed.


> Your commit message  don't follow the Emacs standards.

Thanks for spelling that out. I've fixed this on the new
commit rather than amend the old message and modify
the history. If you think more is required here, let
me know.


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

Done, and included in this commit/patch.


> I would change the wording in `ibuffer-included-in-filters-p' doc string.

Done.


> Once you add `ibuffer-and-filter' there is code duplication...I would
> extract the common code in a new auxiliar function....

Done. I removed some additional code duplication by using
ibuffer-decompose-filter as well and more with the push, while
eliminating unnecessary nesting in the result.


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

That makes sense. This means thinking of starred buffers as special
entities, in which case you don't want to match '*foo''s. If you want
those, you can filter by name explicitly. I've made the suggested
change.


> Could you create a receipt where the bug cause an actual failure?
> ... 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.

OK, I'll do that. Just for the discussion here, the issue is
at the following point in the *original* `ibuffer-save-filters':

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

This treats existing filters (setcdr) and new filters (push)
inconsistently. Using the 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)))))

As you can see, the existing entry "gnus" breaks the expected format.
So to be more precise than I was earlier: In addition to the unnecessary
nesting level, this breaks anytime you save to an existing filter. My
change replaces the `list' with a `cons' and replaces various `cadr''s
with `cdr''s, making the two cases consistent and eliminating the extra
nesting.

Tomorrow, I will pull out the saved filter changes and submit a
formal bug report with patches for the two approaches, making
the other ibuffer changes independent. For the moment, to
facilitate discussion, I've included the commit with my previous
approach included and attached the patch. Sorry for the extra
delay on doing the splitting, but I'm on it.



On Sat, Nov 19, 2016 at 6:17 AM, Tino Calancha <tino.calancha@gmail.com>
wrote:

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