Ihor Radchenko <yanta...@posteo.net> writes: > Alexander Adolf <alexander.ad...@condition-alpha.com> writes: > >> Subject: [PATCH 1/2] lisp/org-colview.el: add formatter parameter to colview >> dynamic block > > Thanks for the patches! > See my comments below.
Thanks for your swift review, and most helpful comments! While I'm implementing these, reactions of my own on a select, few comments of yours: > [...] >> +- =:formatter= :: >> + >> + A function for formatting the data in the dynamic block, overriding >> + the default formatting function set in >> + ~org-columns-dblock-formatter~. > > You can also mention that the function also inserts the data. Something > similar to what we do when describe the equivalent option for clock tables: > > - =:formatter= :: > > A function to format clock data and insert it into the buffer. > > Also, if you mention a variable in the manual, please add #+vindex: > entry. Maybe even #+cindex: entry for "formatter", to make the parameter > more discoverable. I kept it to the format of the existing parameter descriptions, which don't create index entries. Happy to add one though. #+cindex would seem more appropriate, as it's not a variable? On a loosely related note: the description of the :formatter parameter of the clock table does not have and index entry either. Should it get one too, then? Btw, I will also move the half-sentence about overriding the default formatting function from the manual to the docstring of org-dblock-write:columnview, where the :formatter parameter is explained, too. It somehow seems more appropriate there, since a user who is looking into implementing a formatting function will most likely be accessing that docstring anyway (so will find the information), whereas the information about the customization variable is likely adding more confusion than it tries to remove for the rest of the Org users (who will likely consult the manual only). > [...] > (defun org-columns--capture-view (maxlevel match skip-empty exclude-tags > format local &optional link) > >> + (push (if (and link (string= p "ITEM")) >> + (let ((search (org-link-heading-search-string >> + cell-content))) >> + (org-link-make-string >> + (if (not (buffer-file-name)) search >> + (format "file:%s::%s" (buffer-file-name) search)) >> + cell-content)) > > In org-clock, we do > > (org-link-make-string > (if (not (buffer-file-name)) search > (format "file:%s::%s" (buffer-file-name) search)) > ;; Prune statistics cookies. Replace > ;; links with their description, or > ;; a plain link if there is none. > (org-trim > (org-link-display-format > (replace-regexp-in-string > "\\[[0-9]*\\(?:%\\|/[0-9]*\\)\\]" "" > headline)))) > > Is there any reason why you did not remove the statistics cookies here > as well? > [...] Somehow (how?) the statistics cookies get removed in my current implementation. org-link-make-string does not remove them (I double checked). I would thus speculate that perhaps the overlay creation (to show description only) removes them? OTOH, I'm happy to add the org-trim part to make things more robust. Will email updated patches when I will have addressed all your comments. Many thanks and cheers, --alexander
