[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] ox-icalendar.el: customizable vevent summary prefix
From: |
Mikhail Skorzhisnkii |
Subject: |
Re: [PATCH] ox-icalendar.el: customizable vevent summary prefix |
Date: |
Sun, 23 Oct 2022 21:15:10 +0200 |
User-agent: |
mu4e 1.8.7; emacs 28.2 |
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
- Re: [PATCH] ox-icalendar.el: customizable vevent summary prefix,
Mikhail Skorzhisnkii <=