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

[Ihor Radchenko]

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

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

> +  ;; 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.

> 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
> * 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)
> 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.

> +*** 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.

> 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

> * 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.

>  ** 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

>  ** 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.

>  ;;;###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)

> +(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.

-- 
Ihor Radchenko,
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