Re: Bug: org-agenda-overriding-columns-format destroyed on revert [9.2.1 (9.2.1-2-gc6d37c-elpaplus)]

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Allen Li <darkfeline@felesatra.moe> writes:

> I have attached a patch implementing this on maint.

Thank you. Some comments follow.

> Subject: [PATCH] Fix buffer local org-agenda-overriding-columns-format bug
>
> Setting org-agenda-overriding-columns-format as a buffer local value
> interferes with how it is used as a dynamically scoped var, so use a
> separate variable for buffer local setting.

You need to include variables and functions modified in the commit
message, so grepping through them more fruitful.

>  (defvar org-agenda-overriding-columns-format)  ; From org-colview.el
> +(defvar org-agenda-local-overriding-columns-format)  ; From org-colview.el

You can remove the inline comment at the end of the line (and the one
above).

>  (defun org-agenda-finalize ()
>    "Finishing touch for the agenda buffer, called just before displaying it."
>    (unless org-agenda-multi
> @@ -3783,7 +3784,7 @@ FILTER-ALIST is an alist of filters we need to apply when
>  	(unless org-agenda-with-colors
>  	  (remove-text-properties (point-min) (point-max) '(face nil)))
>  	(when (bound-and-true-p org-agenda-overriding-columns-format)
> -	  (setq-local org-agenda-overriding-columns-format
> +	  (setq-local org-agenda-local-overriding-columns-format

Since this is a local variable, `setq' is enough, isn't it?

>  		      org-agenda-overriding-columns-format))
>  	(when org-agenda-view-columns-initially
>  	  (org-agenda-columns))
> diff --git a/lisp/org-colview.el b/lisp/org-colview.el
> index 746426bc7..2fbb5aa6c 100644
> --- a/lisp/org-colview.el
> +++ b/lisp/org-colview.el
> @@ -567,7 +567,13 @@ for the duration of the command.")
>  
>  (defvar org-agenda-overriding-columns-format nil
>    "When set, overrides any other format definition for the agenda.
> -Don't set this, this is meant for dynamic scoping.")
> +Don't set this, this is meant for dynamic scoping.  Set
> +`org-agenda-local-overriding-columns-format' instead.")

The first line of a docstring cannot contain an incomplete sentence (due
to `apropos` design).

> +(defvar-local org-agenda-local-overriding-columns-format nil
> +  "When set, overrides any other format definition for the agenda.
> +This can be set as a buffer local value to avoid interfering with
> +dynamic scoping for `org-agenda-overriding-columns-format'.")

The two variable names are somewhat confusing. Could
`org-agenda-local-overriding-columns-format' be renamed into
`org-local-columns-format'? Since it is defined in "org-colview.el", it
doesn't deserve the "org-agenda" prefix (and neither does the original
`org-agenda-overriding-columns-format'.).

Regards,

-- 
Nicolas Goaziou