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

[Mikhail Skorzhisnkii]

Hi, Ihor,

Sorry for the delay with fixes, took some time before I got time to finish 
this. Thanks for your review and looking forward for the next iteration. See 
new version in the attachment. Comments are inline.

Ihor Radchenko <yantar92@gmail.com> writes:

> Mikhail Skorzhisnkii <mskorzhinskii@eml.cc> writes:
>
>> Thank you for suggestion, I seen an announcement about this function, but 
>> somehow forgot about it.
>>
>> Sending next version of these patches. Changes from the next version:
>
> Thanks!
>
>> Subject: [PATCH 3/3] org-refile.el: show refile targets with a title
>>
>> * lisp/org-refile.el (org-refile-get-targets): Use a document
>> title (#+TITLE) instead of file or buffer name in outline path, if
>> a corresponding customisation option is set to ’title. Fallback to a
>> filename if there is no title in the document.
>
> Please use 2 spaces between sentences in docstrings, comments, and
> commit messages. Also, end sentences with “.”. See
> <https://orgmode.org/worg/org-contribute.html#commit-messages> and
> <https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html>
>

Fixed.

>>  (defcustom org-outline-path-complete-in-steps t
>>    “Non-nil means complete the outline path in hierarchical steps.
>> @@ -319,6 +320,11 @@ converted to a headline before refiling.”
>>               (push (list (and (buffer-file-name (buffer-base-buffer))
>>                                    (file-truename (buffer-file-name 
>> (buffer-base-buffer))))
>>                               f nil nil) tgs))
>> +               (when (eq org-refile-use-outline-path ’title)
>> +                 (push (list (or (org-get-title)
>> +                                 (and f (file-name-nondirectory f)))
>> +                             f nil nil)
>> +                       tgs))
>
> We have very too many whens in this function. It will be more succinct
> to use a single (pcase org-refile-use-outline-path …) instead.

Yes. But then I will be refactoring quite a lot of (working) code that I have 
not actually touching.

I would prefer doing that in the separate patch. You’ve suggested some changes 
in my patches which could be applied to some other places in org-mode files I 
have seen. May be once we finish this discussion I would send a new series of 
patches with restyling?

>> +  ;; When `org-refile-use-outline-path’ is `title’, return extracted
>> +  ;; document title
>> +  (should
>> +   (equal ’(“T” “T/H1”)
>> +     (org-test-with-temp-text-in-file “#+title: T\n* H1”
>
> You may as well add a test when multiple #+title lines are present.

Added.

>> From 62684b478ae5ceb03f66967fbebcc4d6163c826c Mon Sep 17 00:00:00 2001
>> From: Mikhail Skorzhinskii <mskorzhinskii@eml.cc>
>> Date: Sat, 12 Sep 2020 18:10:05 +0200
>> Subject: [PATCH 2/3] org-agenda.el: show document title in outline path
>                                      ^Show

Fixed.

>> * lisp/org.el (org-display-outline-path): Show a document title (#+TITLE
>> value) and an outline path in an echo area if the customisation option
>> is set to ’title. Fallback to a file or a buffer name if the document
>                   ^  Fallback ;; (double space between sentences)

Fixed.

>> title is absent.
>
>>  ** New options
>> -*** New custom settings `org-icalendar-scheduled-summary-prefix' and 
>> `org-icalendar-deadline-summary-prefix'
>
> This is removing an existing NEWS entry. I guess it is not intentional.

Yes. Sorry about that — fixed.

>> +*** A new option for custom setting `org-agenda-show-outline-path' to show 
>> document title
>>
>>  (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 outline path with prepended document
>> +title.  Fallback to file name is no title is present.”
>>    :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)))
>
> I think you can leave
> (const :tag “Show outline path with prepended document title.” title)
>
> This text will be displayed in drop menu in cutomize interface alongside
> with the full docstring. Mentioning the fallback in the docstring should
> be good enough.

Agreed. Fixed.

>> From 5b15f886b22dc542220b48ae9659c4c2d56dea78 Mon Sep 17 00:00:00 2001
>> From: Mikhail Skorzhinskii <mskorzhinskii@eml.cc>
>> Date: Thu, 8 Sep 2022 21:29:23 +0200
>> Subject: [PATCH 1/3] org-clock.el: rename org-clock-get-file-title
>                                     ^Rename

Fixed.

>> * lisp/org.el (org-get-title): A new function to collect a document
>> title from an org-mode buffer, based on a org-clock-get-file-title
>> implementation.
>
> `org-clock-get-file-title’. Elisp symbols should be quoted.

Done. (on a side note, all my email clients show ` and ’ quite differently. I 
wonder why.)

>>  ** New functions and changes in function arguments
>> +*** New function `org-get-title' to get `#+TITLE:' property from buffers
>
> We generally use `code' for Elisp symbols and `#+TITLE:' for verbatim
> non-code text. (This has not been consistently followed in etc/NEWS, but
> at least please change `#+TITLE' to `#+TITLE'). See
> doc/Documentation_Standards.org

Ah, yes. There are many occasions in the ORG-NEWS where this is not followed. 
Would you be interested in the patch fixing these irregularities?

And if you do, would you prefer to have a fixed-up commits for these ones or 
just one big commit? I recently learned about existence of git absorb and 
couldn’t recommend it enough.

>>  ** Removed or renamed functions and variables
>> +*** Rename `org-clock-get-file-title' to `org-get-file-title'
>> +
>> +This function is now part of the `org.el' file.
>
> You do not need to mention this. org-clock-get-file-title was
> introduced in recent commits on main. Main is development branch, and we
> do not need to document changes on the changes made after the last
> release.

OK. Fixed, left only a note about new function. I would say some users may find 
it interesting. I had a function that extracts title for many-many years. Used 
it for displaying document title in the frame title.

>>  ;;;###autoload
>>  (defun org-dblock-write:clocktable (params)
>>    “Write the standard clocktable.”
>> @@ -2739,7 +2729,7 @@ from the dynamic block definition.“
>>                           ”\n“)
>>
>>                       (if filetitle
>> -                         (org-clock-get-file-title file-name)
>> +                         (org-get-file-title file-name)
>>                         (file-name-nondirectory file-name))
>>                   (if level?    ”| “ ”“) ;level column, maybe
>>                   (if timestamp ”| “ ”“) ;timestamp column, maybe
>
> This may introduce a compiler warning. I suggest running make after
> applying your patch and fix possible compiler warnings. (I suspect that
> you may need to add declare-function on top of org-clock.el)

Hm, I have tried it on the latest stable emacs (28.2) and it does not produce 
me a warning. `make compile' was just clean. Could you please refer me to the 
library/documentation why would I need to call `declare-function'? This is 
something from cl library?

>> +(defun org-get-file-title (file-name)
>> +  “Collect title from `org-mode’ FILE-NAME.
>> +
>> +Return file name if title does not exist.”
>> +  (or (org-get-title (find-file-noselect file-name))
>> +      (file-name-nondirectory file-name)))
>> +
>> +(defun org-get-title (&optional buffer)
>> +  “Collect title from the provided `org-mode’ BUFFER.
>> +
>> +Returns nil if there are no #+TITLE property.”
>> +  (let ((buffer (or (buffer-base-buffer)
>> +                    buffer
>> +                    (current-buffer))))
>> +    (with-current-buffer buffer
>> +      (org-macro-initialize-templates)
>> +      (let ((title (assoc-default “title” org-macro-templates)))
>> +        (unless (string= “” title)
>> +          title)))))
>
> These two functions can be merged into a single `org-get-title’ that
> accepts buffer or file-name as an optional argument.

Merged, but a little bit differently. Now the function accepts buffer or file. 
Also, I removed the basebuffer logic, because this logic is already included in 
the `org-macro-initialize-templates'.

Thanks,
Mikhail Skorzhinskii

0003-org-refile.el-Show-refile-targets-with-a-title.patch
Description: Text Data

0002-org-agenda.el-Show-document-title-in-outline-path.patch
Description: Text Data

0001-org-clock.el-Rename-org-clock-get-file-title.patch
Description: Text Data