Re: [PATCH] ox-icalendar.el: customizable vevent summary prefix

[Mikhail Skorzhisnkii]

Thanks for your comment, Ihor. I have addressed your comments, see some 
comments inline. Attaching new version of the patch.

Ihor Radchenko <yantar92@gmail.com> writes:

> Mikhail Skorzhisnkii <mskorzhinskii@eml.cc> writes:
>
>> I have signed FSF papers. Attaching a rebased patch with additional changes 
>> to
>> ORG-NEWS
>
> Thanks!
>
>> Subject: [PATCH 1/2] org-agenda.el: customize outline path in echo area
>>
>> * lisp/org-agenda.el (org-agenda-show-outline-path): add an option to
>> show document title in outline path (instead of file name)
>
> Please follow the commit message conventions as described in
> <https://orgmode.org/worg/org-contribute.html#commit-messages> In
> particular, start sentences from capital letters, end them with “.”,
> separate sentences with double space, and quote lisp symbols as
> `symbol’.
>
>> * lisp/org.el (org-get-title-from-buffer): a function to collect the
>                                              New
>> document title from the org-mode buffer
>                                          .

Sorry — missed these rules. Fixed now.

>> * lisp/org.el (org-display-outline-path): add logic that will collect a
>> document title and put it into the outline path if
>> org-agenda-show-outline-path set to ’title
>
> This is not what the patch does.  From this message, it looks like
> `org-agenda-show-outline-path’ is affecting the output of
> `org-display-outline-path’, which is not true.

Hm, for me it doesn’t look like it from my perspectrive, but my command of the
English is not that good. I have reworded it. Is it better now?

Feel free to reword it on final apply or let me know if you would like to
improve — I will someone to proofread it.

>>  (defcustom org-agenda-show-outline-path t
>> -  “Non-nil means show outline path in echo area after line motion.”
>> +  “Non-nil means show outline path in echo area after line motion.
>> +
>> +If set to ‘title, show document title.”
>
> This is not very clear. I’d rather put more detailed explanation as in
> the defcustom :type spec below.

Fixed.

>>    :group ’org-agenda-startup
>> -  :type ’boolean)
>> +  :type ’(choice
>> +      (const :tag “Don’t show outline path in agenda view.” nil)
>> +      (const :tag “Show outline path with prepended file name.” t)
>> + (const :tag “Show outline path with prepended document title. Fallback to
>> file name is no title is present.” title)))
>> -(defun org-display-outline-path (&optional file current separator 
>> just-return-string)
>> +(defun org-get-title-from-buffer (&optional buffer)
>> +  “Collect title from the provided `org-mode’ BUFFER.”
>> +  (let* ((buffer (or buffer (current-buffer)))
>> +         (buffer (or (buffer-base-buffer buffer)
>> +                     buffer))
>
> Why not just
>
> (or (buffer-base-buffer buffer)
>     buffer
>     (current-buffer))

Applied your suggestion.

>> +         title)
>> +    (with-current-buffer buffer
>> +      (pcase (org-collect-keywords ’(“TITLE”))
>> +        (`((“TITLE” . ,val))
>> +         (setq title (car val)))))
>> +    title))
>
> Extra `title’ variable is unnecessary here. You can simply do
>
>     (with-current-buffer buffer
>       (pcase (org-collect-keywords ’(“TITLE”))
>         (`((“TITLE” ,val . _))
>          val)))

Indeed — remnant of previous implementation iteration. Applied your suggestion.

> Also, what will happen in a file like
>
> #+TITLE: Begin title
> #+TITLE: .. end title
>
> ?

Hm, never did this myself. Now concatenate the list of property values. I have
tested it and space as a separator looks good, if the intention of several
titles is to have one big title.

But what if several titles are title and subtitles? We can provide a way control
that behaviour, but my gut feeling that it would be a rather confusing.

>> +(defun org-display-outline-path (&optional file-or-title current separator 
>> just-return-string)
>>    “Display the current outline path in the echo area.
>>
>> -If FILE is non-nil, prepend the output with the file name.
>> +If FILE-OR-TITLE is ‘title, prepend outline with file title.  If
>> +it is non-nil or title is not present in document, prepend
>> +outline path with the file name.
>>  If CURRENT is non-nil, append the current heading to the output.
>>  SEPARATOR is passed through to `org-format-outline-path’.  It separates
>>  the different parts of the path and defaults to \”/\“.
>> @@ -7407,6 +7421,8 @@ If JUST-RETURN-STRING is non-nil, return a string, 
>> don’t display a message.”
>>    (interactive “P”)
>>    (let* (case-fold-search
>>       (bfn (buffer-file-name (buffer-base-buffer)))
>> +         (title-prop (when (and file-or-title (eq file-or-title ’title))
>
> can be simply (eq file-or-title ’title)

Indeed. Fixed.

0001-org-agenda.el-customize-outline-path-in-echo-area.patch
Description: Text Data