Re: columnview dynamic block - different time summing behaviour for EFFORT and CLOCKSUM

[Alexander Adolf]

Ihor Radchenko <yantar92@posteo.net> writes:

> Alexander Adolf <alexander.adolf@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