Re: [O] [PATCH] org-agenda: Add 'none setting for org-agenda-overriding-header

[Adam Porter]

Hi Nicolas,

Nicolas Goaziou <address@hidden> writes:

> Adam Porter <address@hidden> writes:
>
>> Here are the patches.  Please let me know if any other changes are
>> needed.
>
> Thank you! Comments follow.
>
>> +(defmacro org-agenda--insert-overriding-header (&key default)
>
> There is no "&key" in `defmacro'.

You're right, that's why IIRC I used cl-defmacro originally.  The
issue here seems to be using the keyword argument.  It seemed like a
good idea to specify it with the ":default: keyword argument for two
reasons:

1.  To make it explicitly clear that the body of code passed to the
macro is only the default header, not necessarily the one that will be
used.

2.  To make it easier to add other arguments in the future.  For
example, I could imagine adding an argument to change the face or
properties of the overriding header, a prefix or suffix, etc., and those
would be much easier to handle as keyword args, to avoid calling it as
something like "nil nil t nil", as happens with some other Emacs
function signatures (e.g. I often have to look up how to call
re-search-forward as opposed to replace-regexp-in-string).

> It should be (default).
>
>> +  "Insert header into agenda view depending on value of 
>> `org-agenda-overriding-header'.
>> +If the empty string, don't insert a header.  If any other string,
>> +insert it as a header.  If nil, insert DEFAULT, which should
>> +evaluate to a string."
>> +  (declare (debug (&key form)))
>
> It needs to be updated according to the above.

I'm not sure what you mean here, but I guess it depends on the previous matter.

>> +                        ;; Format week number span
>> +                        (cond ((< (- d2 d1) 350)
>> +                               (if (= w1 w2)
>> +                                   (format " (W%02d)" w1)
>> +                                 (format " (W%02d-W%02d)" w1 w2)))
>> +                              (t ""))
>
>   (cond ((<= 350 (- d2 d1)) "")
>         ((= w1 w2) (format " (W%02d)" w1))
>         (t (format " (W%02d-W%02d)" w1 w2)))

I see.  That doesn't seem to be exactly the same logic as the nested if,
and I didn't follow the surrounding logic well enough to try to
transform it that way.  But I assume you know what you're doing, so I'll
swap that in.  :)

>> -                (let ((n 0) s)
>> -                  (mapc (lambda (x)
>> -                          (setq s (format "(%d)%s" (setq n (1+ n)) x))
>> -                          (if (> (+ (current-column) (string-width s) 1) 
>> (frame-width))
>> -                              (insert "\n                     "))
>> -                          (insert " " s))
>> -                        kwds))
>> +                (cl-loop for keyword in kwds
>> +                         and num from 1
>> +                         for string = (format "(%d)%s" num keyword)
>> +                         when (> (+ (current-column) (string-width string) 
>> 1)
>> +                                 (window-width))
>> +                         do (insert "\n                     ")
>> +                         do (insert " " string))
>
> Ouch. Why `cl-loop' over `dolist'? Also it looks wrong since the last
> `do' is not always executed? (or is it?).

Yes, it is always executed: the "when" only applies to the next clause,
and I tested it to be sure, both by executing it and expanding the
macro.  I've used cl-loop a lot lately, so it is familiar to me.

> I know there is more than one way to skin a cat, but I'd rather use
> a straightforward one:
>
>   (let ((n 0))
>     (dolist (k kwds)
>       (let ((s (format "(%d)%s" (cl-incf n) k)))
>         (when (> (+ (current-column) (string-width s) 1) (frame-width))
>           (insert "\n                     "))
>         (insert " " s))))

I guess this is a matter of style, as I prefer the cl-loop version,
which doesn't hide the incrementing in the format call and avoids
another level of nesting just for the counter variable.  :)  But if you
want me to use the dolist instead, it's up to you.

Let me know about these issues and I'll send another patch series.

Thanks.