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

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

Hi Christopher,

thank you very much for your time working on this!
I have checked your newest changes and i got a few additional
comments.  See below.

>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.
Very good.  That would be helpful.  Thank you.

>The buffer name filter you suggest is already available as
>ibuffer-filter-by-name (/ n).
Right! I totally overlooked that.

> 1. Add a `ibuffer-filter-by-visiting-file' (/ v) that selects
>    buffers that are visiting a file.
+(define-ibuffer-filter visiting-file
+    "Limit current view to buffers that are visiting a file.
+This includes buffers visiting a directory in dired."
I wouldn't include Dired buffers here.  In Emacs a buffer
visiting a file means a non-directory file.  For instance,
look doc string of `buffer-file-name'.

I suggest instead:
(define-ibuffer-filter visiting-file
     "Limit current view to buffers that are visiting a file."
   (:description "visiting a file"
    :reader nil)
   (with-current-buffer buf buffer-file-name))

> `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.
I am wondering if it's worth to have the `ibuffer-filter-by-directory'
in the way you are proposing.  I guess `ibuffer-filter-by-filename'
would suffice most of the times.
In the other hand we have `ibuffer-mark-dired-buffers' bound to '*/'
that is handy.  We might want `ibuffer-filter-by-directory' to do
the symmetric thing: that is, to filter buffers in Dired mode, i.e.,
like a shortcut for '/ m' dired-mode RET.

Alternatively we could accept a prefix in this command:
1) Without prefix, just filter buffers in Dired mode.
2) With a prefix, behave as you wish, as follows:

(define-ibuffer-filter directory
     "Limit current view to Dired buffers.

With prefix argument prompt for a regexp and show just
those buffers with their directory matching that regexp.

For a buffer associated with file '/a/b/c.d', this matches
against '/a/b'. For a buffer not associated with a file, this
matches against the value of `default-directory' in that buffer."
   (:description "directory name"
                 :reader (and current-prefix-arg
                              (read-from-minibuffer "Filter by directory name (regex): ")))
   (with-current-buffer buf
     (if qualifier
         (let ((dirname
                (ibuffer-aif (ibuffer-buffer-file-name)
                    (file-name-directory it)
                  default-directory)))
           (and dirname (string-match qualifier dirname)))
       (eq major-mode 'dired-mode))))

This means the command do a different thing if we provide the prefix.
I don't know what approach is more useful.
Does 1) or 2) has sense for you?

>3. Keep the `ibuffer-filter-by-basename', making the name
I saw you bound this command to '/ b'.  Good!
I find easier to remember and type '/ b' than '/ F'.

>(Note: '/ d' is already bound to ibuffer-decompose-filter or I would have
>used it.
Opps!  I didn't notice this.  Thanks.

>I think the new bindings are highly mnemonic and will happily advocate
>for them. But the need for consensus makes total sense.
>I felt that the change I made keeps the mnemonic strong.
Once we are happy with the changes we might ask opinion to other
colleagues in Emacs-dev about what to do with the bindings.

>> Your commit message  don't follow the Emacs standards.
> I've fixed this on the new commit.
>* lisp/ibuf-ext.el: added paragraph to file commentary, along
                      ^^^^^
>(ibuffer-saved-filters): clarified documentation,
                           ^^^^^^^^^ 
Please, start sentences with upper case.


>> You might want to write NEWS entry for the new features.
>Done, and included in this commit/patch.
Thank you.  They like quite verbose for NEWS entries.  We just need
to announce the changes.  It's OK to group all new commands
in the same entry.
Let's ignore the entry for the bug fix.  We will back to that issue
once we open the bug report.
I suggest the following shorter entries:
---
*** New filter commands `ibuffer-filter-by-basename',
`ibuffer-filter-by-file-extension', `ibuffer-filter-by-directory',
`ibuffer-filter-by-starred-name', `ibuffer-filter-by-modified'
and `ibuffer-filter-by-visiting-file'; bound respectively
to '/b', '/.', '//', '/*', '/i' and '/v'.

---
*** Two new commands 'ibuffer-filter-chosen-by-completion'
and `ibuffer-and-filter'; bound to '/ TAB' and '/&'
respectively.

---
*** The key binding for `ibuffer-filter-disable' has being changed
to '/DEL'; the commands `ibuffer-pop-filter', `ibuffer-pop-filter-group'
and `ibuffer-or-filter' have the alternative bindings '/<up>', '/S-<up>'
and '/|'.

>> Could you create a receipt where the bug cause an actual failure?
>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.
You are right, it seems there is a bug.

>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.
That will be very useful.  Thank you.

Regards,
Tino