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

[Ihor Radchenko <yantar92@posteo.net>]

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.

> * lisp/org-colview.el (org-dblock-write:column view): Factor out the
> existing formatting code to new function
> org-columns-dblock-write-default, and honour new dblock parameter
> :formatter for specifying a different formatting function.

In the changelog entries, we quote the symbol and variable names like
`this'.

> +- =: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.

> +*** New option ~org-columns-dblock-formatter~
> +
> +=colview= dynamic blocks now understand a new ~:formatter~ parameter
> +to use a specific function for formatting the contents of the dynamic
> +block. This new option can be used to set the global default

... and inserting its contents.

It would also be helpful to refer to the existing :formatter parameter
in clock tables.

> +*** =colview= dynamic block supports custom formatting function
> +
> +The =colview= dynamic block understands a new ~:formatter~ parameter,
> +which specifies a user-supplied function to format the data in the

... and insert the data

> +(defcustom org-columns-dblock-formatter #'org-columns-dblock-write-default
> +  "Function to format data in column view dynamic blocks.
> +For more information, see `org-columns-dblock-write-default'."
> +  :group 'org-properties
> +  :version "30.0"

We do not need :version tag when there is :package-version.

> +(defun org-columns-dblock-write-default (ipos table params)
> +  "Write out a columnview table at position IPOS in the current buffer.
> +TABLE is a table with data as produced by `org-columns--capture-view'.
> +PARAMS is the parameter property list obtained from the dynamic block
> +definition."
> +  (let ((width-specs
>           (mapcar (lambda (spec) (nth 2 spec))
>                   org-columns-current-fmt-compiled)))
>      (when table

This new function ignores IPOS parameter. It should not.
 
> From 4d69c3407c7c00fe0a1f52c383ced572f3524076 Mon Sep 17 00:00:00 2001
> From: Alexander Adolf <alexander.adolf@condition-alpha.com>
> Date: Mon, 15 Apr 2024 18:01:40 +0200
> Subject: [PATCH 2/2] lisp/org-colview.el: add link parameter to colview
>  dynamic block

*Add
(capitalize)

> * lisp/org-colview.el (org-columns--capture-view): Add new link
> parameter, which when non-nil causes ITEM headlines to be linked to
> their origins.
> (org-dblock-write:columnview): Pass new link parameter to
> org-columns--capture-view, and explain its use in the docstring.

`org-columns--capture-view'

> -(defun org-columns--capture-view (maxlevel match skip-empty exclude-tags format local)
> +(defun org-columns--capture-view (maxlevel match skip-empty exclude-tags link format local)

While this is an internal function and we are free to change it as we
need, it is generally more robust to not make changes in the order of
function arguments. I recommend

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

> `make compile` didn't complain at all, and `make test` ended with the
> following:
> ...
> 4 unexpected results:
>    FAILED  ob-calc/matrix-inversion
>    FAILED  test-ob-shell/bash-uses-assoc-arrays
>    FAILED  test-ob-shell/bash-uses-assoc-arrays-with-lists
>    FAILED  test-org-table/sort-lines

MacOS? There are known issues with locale rules in MacOS that may cause
test failures.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>