From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Tino Calancha Newsgroups: gmane.emacs.devel Subject: Re: Ibuffer improvements: filtering, documentation, bug fix, tests Date: Sat, 19 Nov 2016 20:17:14 +0900 Message-ID: <877f7zhg79.fsf@gmail.com> References: NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1479554288 31371 195.159.176.226 (19 Nov 2016 11:18:08 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 19 Nov 2016 11:18:08 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) Cc: emacs-devel , tino.calancha@gmail.com To: Christopher Genovese Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Nov 19 12:18:03 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c83ek-00077i-LU for ged-emacs-devel@m.gmane.org; Sat, 19 Nov 2016 12:18:02 +0100 Original-Received: from localhost ([::1]:40914 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c83eo-0001wz-6H for ged-emacs-devel@m.gmane.org; Sat, 19 Nov 2016 06:18:06 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:36690) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c83e8-0001wl-9A for emacs-devel@gnu.org; Sat, 19 Nov 2016 06:17:25 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c83e5-0001zl-57 for emacs-devel@gnu.org; Sat, 19 Nov 2016 06:17:24 -0500 Original-Received: from mail-pg0-x235.google.com ([2607:f8b0:400e:c05::235]:35058) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c83e4-0001yg-RN for emacs-devel@gnu.org; Sat, 19 Nov 2016 06:17:21 -0500 Original-Received: by mail-pg0-x235.google.com with SMTP id p66so112349385pga.2 for ; Sat, 19 Nov 2016 03:17:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:subject:in-reply-to:references:user-agent:cc:date :message-id:mime-version; bh=ZwMhHwwbV/ZGwKm68dDsgKGBAwh+qzFFG4ZMWe+12b8=; b=dqif9x3+ao4yfK3nfEyg1asaxdRSTvFe46fnRtiCy76F6UJAzZdtI2cXHWk7RTsDeO nqnAEr22uARbd2hxmmR/P/skxZ8eJkUIjzSZECApSirS3Sh2nqmmDjbfNnb1Yj+W0JBI 39KWUUHWTvZczH32FKQuwPjK59yLG0g1aZlMSyEiVDNMgZpCYf0VwzF593hRPpKXiB13 L7nX35EythvXpzORykUVSw6Pfo6EG+E/LIzbdxY1h9agCx2xr/HFdCuuMMHFpIJUVE2y 8pf/C5jI6L6sqaFtIbUSsEN0GVhXi56kowl4aGuyBbmilghkHSGVyk3vgEi7uq4Ih+7p jv5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:in-reply-to:references :user-agent:cc:date:message-id:mime-version; bh=ZwMhHwwbV/ZGwKm68dDsgKGBAwh+qzFFG4ZMWe+12b8=; b=A4ySPFTBUDc60ZqIXTo7c+oNSaZsAds7BqrGkfLwt6ZIVHiorowz8s/Sgufy0BhnIP NX9y5Zk/qBumPvldnOJgxW/Hrr0KhXyKn/NupwAbLWtuiaV3SeYxifPccN8JnW0b8l/w BShVrTkbq7L1ZS35FRI1DXXv/P9yrnKIoHIp/3AdllcwJI3ZZ8lMByWa8T616d/f0zMJ Yf0erH+VDWT2AP20k9CL6GfYHWV0nTv7cbZ461rnPr+QRp5JbusQRhAAT7+qmKGK3Wm5 FgjC13pI6S++shk69omwmu97fxTEiVcYYJcUTdJe7ZlT5Gl+OMF++8nqlLRIG0QnMhIW aG3w== X-Gm-Message-State: AKaTC03p3TYRKFKY/xabXL0kQk61CZDb6f9cnHJrJ86CiCUXyZr79ISFFf1XvAILfR5eug== X-Received: by 10.98.213.7 with SMTP id d7mr5507813pfg.3.1479554239380; Sat, 19 Nov 2016 03:17:19 -0800 (PST) Original-Received: from calancha-pc (57.92.100.220.dy.bbexcite.jp. [220.100.92.57]) by smtp.gmail.com with ESMTPSA id o126sm6348008pga.34.2016.11.19.03.17.17 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 19 Nov 2016 03:17:18 -0800 (PST) In-Reply-To: (Christopher Genovese's message of "Thu, 17 Nov 2016 10:53:58 -0500") X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2607:f8b0:400e:c05::235 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:209495 Archived-At: Christopher Genovese 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: "\\`\\*[^*]+\\*\\(?\\)\\'" 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