Re: Allowing multiple date trees in a single file

[Carsten Dominik <dominik@uva.nl>]

[-- Attachment #1: Type: text/plain, Size: 17802 bytes --]

Hi Nicolas,

thank you for taking the time to look at the proposed changes in detail.
My replies and comments are below.

On Sat, Feb 4, 2017 at 1:48 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:

> Hello,
>
> Carsten Dominik <dominik@uva.nl> writes:
>
> > Attached is a patch that does the following:
> >
> > It consolidates all four different org-capture target types that have to
> do
> > with
> > date/week trees into a single one, called `file+olp+datetree'.  This
> target
> > allows for an optional outline path specification to tell capture to
> build
> > the
> > datetree under a specific headline.  To switch to a week tree, or to
> force
> > a date prompt is now the matter of setting one of the properties in the
> > org-capture-template variable.
>
> It sounds good. Thank you.
>

Allright.


>
> > Everything works transparently, so users can update the way they
> > write their datetree captures, but they don't have to - the old syntax
> > remains
> > supported and will automatically switched when one uses customize to
> change
> > the variable.
>
> I am a bit worried by this compatibility layer. I mean, it is good to
> preserve compatibility with old templates, but it ought to be an
> ephemeral solution. I.e., no more
> `org-table--error-on-old-row-references' lingering around for ages.
>
> We could, for example, generate a deprecation warning when old templates
> are used. Then we will be able to remove this unnecessary piece of code
> in next major release.
>

We do disagree here a bit.  This little bit of extra work just keeps the
existing templates working.  We do not introduce a really different
structure of the org-capture-templates.  Rather, the code introduces a new
target type, and it makes some older target types be implemented as special
versions of the new ones.  The old targets are no longer in the manual, any
customize user will be switched automatically.  What remains is a small bit
of code that makes sure that the setup of user who might have been using
that for a long time continues to work.

In my eyes, this is worth it.  Breaking something in a new version is no
big deal for people who use Org regularly, but I am sure there are a lot of
users out there who have not changed their setup for a long time, have not
followed the discussions here and would be frustrated if their setup breaks
after getting a new version of Emacs, for example.  So we can shoot a
warning, but I would vote for just keeping this piece of code indefinitely.


>
> See for example the end of `org-open-file', although it is a bit more
> drastic (it raises an error, not a warning).
>
> > After a bit more testing, I'd like to apply this patch.  Please let me
> know
> > if you agree. And additional testers would be useful.  Anyone?  Make sure
> > to backup your capture templates if something goes wrong.
>
> Some comments follow.
>
> > -@item (file+weektree+prompt "path/to/file")
> > -Will create a heading in a week tree, but will prompt for the date.
> > +one matched.}.  If the optional outline path is given, the tree will be
> built
> > +under the node it is pointing to. Check out the @code{:time-prompt} and
>
> There's a missing space above.
>

I will fix that in the next patch.


>
> > +(defun org-capture-upgrade-templates (templates)
> > +  "Update the template list to the new format.
> > +The new format unifies all the date/week tree targets into one that
> > +also allows for an optional outline path to specify a target."
> > +  (let (target props)
> > +    (mapcar
> > +     (lambda (ee)
> > +       (setq target (car (nth 3 ee)))
> > +       (when (memq target '(file+datetree file+datetree+prompt
> > +                                       file+weektree
> file+weektree+prompt))
> > +      (setq target (symbol-name target) props nil)
> > +      (if (string-match "prompt" target) (setq props '(:time-prompt t)))
> > +      (if (string-match "week" target)
> > +          (setq props (append '(:tree-type week) props)))
> > +      (setcar (nth 3 ee) 'file+olp+datetree)
> > +      (setcdr (nthcdr 4 ee) (append props (nthcdr 5 ee))))
> > +       ee)
> > +     templates)))
>
> I suggest the following. Less `setq', `setcar', `setcdr' makes Org
> a better place.
>
> (defun org-capture-upgrade-templates (templates)
>   "Update the template list to the new format.
> TEMPLATES is a template list, as in `org-capture-templates'. The
> new format unifies all the date/week tree targets into one that
> also allows for an optional outline path to specify a target."
>   (mapcar
>    (lambda (template)
>      (pcase template
>        ;; Match templates with an obsolete "tree" target type. Replace
>        ;; it with common `file+olp-datetree'.  Add new properties
>        ;; (i.e., `:time-prompt' and `:tree-type') if needed.
>        (`(,key ,desc ,type (file+datetree . ,path) ,template . ,props)
>         `(,key ,desc ,type (file+olp+datetree ,@path) ,@props))
>        (`(,key ,desc ,type (file+datetree+prompt . ,path) ,template .
> ,props)
>         `(,key ,desc ,type (file+olp+datetree ,@path) :time-prompt t
> ,@props))
>        (`(,key ,desc ,type (file+weektree . ,path) ,template . ,props)
>         `(,key ,desc ,type (file+olp+datetree ,@path) :tree-type week
> ,@props))
>        (`(,key ,desc ,type (file+weektree+prompt . ,path) ,template .
> ,props)
>         `(,key ,desc ,type (file+olp+datetree ,@path) :tree-type week
> :time-prompt t ,@props))
>        ;; Other templates are left unchanged.
>        (_ template)))
>    templates))
>

OK, no objections to a different implementation.  I am not familiar with
pcase, looks general and useful, should learn about it.


>
> > -             (file+weektree+prompt \"path/to/file\")
> > -                 Will create a heading in a week tree, prompts for date
> > +             (file+olp+datetree \"path/to/file\" \"Level 1 heading\"
> ...)
> > +                 Will create a heading in a date tree for today's date.
> > +                 If no headings are given, the tree will be on top
> level.
>
> Nitpick: It may be just me, but "If no heading is given" sounds better.
>

OK.


>
> >  ELisp programs can set KEYS to a string associated with a template
> >  in `org-capture-templates'.  In this case, interactive selection
> >  will be bypassed.
> > @@ -902,6 +920,7 @@ Store them in the capture property list."
> >          (insert "* " headline "\n")
> >          (beginning-of-line 0)))
> >       (`(file+olp ,path . ,outline-path)
> > +      (setq outline-path (org-capture-sanitize-olp outline-path))
>
> See below about `org-capture-sanitize-olp'.
>
> Also, it is better to make the `setq' a let-binding when possible.
>

Hmm, maybe I misunderstand pcase.  I was under the impression that when
pcase does the match, it will bind the path to outline-path locally (with
let or something similar), so that I can, in the scope of the current
match, use setq to change the variable.

Is my understanding incorrect here?


> >        (let ((m (org-find-olp (cons (org-capture-expand-file path)
> >                                     outline-path))))
> >          (set-buffer (marker-buffer m))
> > @@ -922,59 +941,65 @@ Store them in the capture property list."
> >          (org-capture-put :exact-position (point))
> >          (setq target-entry-p
> >                (and (derived-mode-p 'org-mode) (org-at-heading-p)))))
>
> [...]
>
> > +     (`(file+olp+datetree ,path . ,outline-path)
> > +      (setq outline-path (org-capture-sanitize-olp outline-path))
>
> Ditto.
>
> > +      (let ((m (if outline-path
> > +                   (org-find-olp (cons (org-capture-expand-file path)
> > +                                       outline-path))
> > +                 (set-buffer (org-capture-target-buffer path))
> > +                 (move-marker (make-marker) (point)))))
> > +        (set-buffer (marker-buffer m))
> > +        (org-capture-put-target-region-and-position)
> > +        (widen)
> > +        (goto-char m)
> > +        (set-marker m nil)
> > +        (require 'org-datetree)
> > +        (org-capture-put-target-region-and-position)
> > +        (widen)
> > +        ;; Make a date/week tree entry, with the current date (or
> > +        ;; yesterday, if we are extending dates for a couple of hours)
> > +        (funcall
> > +         (if (eq (org-capture-get :tree-type) 'week)
> > +             #'org-datetree-find-iso-week-create
> > +           #'org-datetree-find-date-create)
> > +         (calendar-gregorian-from-absolute
> > +          (cond
> > +           (org-overriding-default-time
> > +            ;; Use the overriding default time.
> > +            (time-to-days org-overriding-default-time))
> > +           ((org-capture-get :time-prompt)
> > +            ;; Prompt for date.
> > +            (let ((prompt-time (org-read-date
> > +                                nil t nil "Date for tree entry:"
> > +                                (current-time))))
> > +              (org-capture-put
> > +               :default-time
> > +               (cond ((and (or (not (boundp 'org-time-was-given))
> > +                               (not org-time-was-given))
> > +                           (not (= (time-to-days prompt-time)
> (org-today))))
> > +                      ;; Use 00:00 when no time is given for another
> > +                      ;; date than today?
> > +                      (apply #'encode-time
> > +                             (append '(0 0 0)
> > +                                     (cl-cdddr (decode-time
> prompt-time)))))
> > +                     ((string-match "\\([^ ]+\\)--?[^ ]+[ ]+\\(.*\\)"
> > +                                    org-read-date-final-answer)
> > +                      ;; Replace any time range by its start.
> > +                      (apply #'encode-time
> > +                             (org-read-date-analyze
> > +                              (replace-match "\\1 \\2" nil nil
> > +                                             org-read-date-final-answer)
> > +                              prompt-time (decode-time prompt-time))))
> > +                     (t prompt-time)))
> > +              (time-to-days prompt-time)))
> > +           (t
> > +            ;; Current date, possibly corrected for late night
> > +            ;; workers.
> > +            (org-today))))
> > +         ;; the following is the keep-restriction argument for
> > +         ;; org-datetree-find-date-create
>
>   ;; The following...
>   ;; ... `org-datetree-find-date-create'.
>
> > +         (if outline-path 'subtree-at-point)
>
>   (and outline-path 'subtree-at-point)
>
> > +         )))
>
> These trailing parens need to be moved above.
>

OK.


>
> >       (`(file+function ,path ,function)
> >        (set-buffer (org-capture-target-buffer path))
> >        (org-capture-put-target-region-and-position)
> > @@ -1432,6 +1457,13 @@ Use PREFIX as a prefix for the name of the
> indirect buffer."
> >    (unless (org-kill-is-subtree-p tree)
> >      (error "Template is not a valid Org entry or tree")))
> >
> > +(defun org-capture-sanitize-olp (olp)
> > +  "Keep only non-white strings in the list OPL."
> > +  (let (res e)
> > +    (while (setq e (pop olp))
> > +      (and (stringp e) (string-match "\\S-" e) (push e res)))
> > +    (nreverse res)))
>
> This function is really a one-liner:
>
>   (cl-remove-if-not #'org-string-nw-p olp)
>
> Therefore, I don't think it deserves a dedicated function.
>
> BTW is there any reason to prune non-white strings from the list? Is an
> empty string even invalid, since document can contain blank headlines?
>
> The outline path is provided by the user. We can assume he knows what he
> is doing.
>
> Another option would be to barf if the outline path contains any invalid
> value instead of silently "fixing" it on user's behalf. E.g.,
>
>   (when (cl-some #'invalid-value-check olp)
>     (user-error "Invalid outline path in template %S" template))
>
> for some value of #'invalid-value-check.
>
> WDYT?
>

Well, I agree with you that we should not even have this code in here.  It
is a hack to solve an issue I was not able to crack - maybe you can.  Let
me explain:

When I use customize to insert a template definition with the new
file+olp+datetree target, I want the outline path to be optional.

Here is the relevant part of the customize type definition:

 (list :tag "File [ & Outline path ] & Date tree"
(const :format "" file+olp+datetree)
,file-variants
(repeat :tag "Outline path" :inline t
(string :tag "Headline")))

The problem is that customize sets this up assuming at least one string, in
the customize buffer it looks like this

            Target location: Value Menu File [ & Outline path ] & Date tree:
            Filename       : Value Menu Literal:
            Outline path:
            INS DEL Headline:
            INS
            Template       : Value Menu String:

As you can see, without the user clicking INS, there is already a string
there. So the user would have to click DEL to make the old empty.  I
figured people would forget all the time, therefore the hack to remove
empty headlines.  It is critical that the outline path to be empty, because
that is used to decide if the date trree will be on top level or under a
heading.

Do you or anyone know how to tweak customize to start out with an empty OLP
for this application?  The we can remove that part entirely.  Otherwise, I
am happy to make it a oneliner.


>
> >  When it is nil, the buffer will be widened to make sure an existing date
> > -tree can be found."
> > +tree can be found.  If it is the sympol `subtree-at-point', then the
> tree
> > +will be built under the headline at point."
> >    (setq-local org-datetree-base-level 1)
> > -  (or keep-restriction (widen))
> >    (save-restriction
> > -    (let ((prop (org-find-property "DATE_TREE")))
> > -      (when prop
> > -     (goto-char prop)
> > -     (setq-local org-datetree-base-level
> > -                 (org-get-valid-level (org-current-level) 1))
> > -     (org-narrow-to-subtree)))
> > +    (if (eq keep-restriction 'subtree-at-point)
> > +     (progn
> > +       (or (org-at-heading-p) (error "Not at heading"))
>
> Nitpick:
>
>   (unless (org-at-heading-p) (error "Not at heading"))
>

OK


>
> > +       (widen)
> > +       (org-narrow-to-subtree)
> > +       (setq-local org-datetree-base-level
> > +                   (org-get-valid-level (org-current-level) 1)))
> > +      (or keep-restriction (widen))
>
>   (unless keep-restriction (widen))
>

OK


>
> > +      ;; support the old way of tree placement, using a property
>
>   ;; Support ... using a property.
>

OK


>
> > +      (let ((prop (org-find-property "DATE_TREE")))
> > +     (when prop
> > +       (goto-char prop)
> > +       (setq-local org-datetree-base-level
> > +                   (org-get-valid-level (org-current-level) 1))
> > +       (org-narrow-to-subtree))))
> >      (goto-char (point-min))
> >      (let ((year (calendar-extract-year d))
> >         (month (calendar-extract-month d))
> > @@ -84,18 +93,26 @@ tree can be found."
> >    "Find or create an ISO week entry for date D.
> >  Compared to `org-datetree-find-date-create' this function creates
> >  entries ordered by week instead of months.
> > -If KEEP-RESTRICTION is non-nil, do not widen the buffer.  When it
> > -is nil, the buffer will be widened to make sure an existing date
> > -tree can be found."
> > +When it is nil, the buffer will be widened to make sure an existing date
> > +tree can be found.  If it is the sympol `subtree-at-point', then the
> tree
> > +will be built under the headline at point."
> >    (setq-local org-datetree-base-level 1)
> > -  (or keep-restriction (widen))
> >    (save-restriction
> > -    (let ((prop (org-find-property "WEEK_TREE")))
> > -      (when prop
> > -     (goto-char prop)
> > -     (setq-local org-datetree-base-level
> > -                 (org-get-valid-level (org-current-level) 1))
> > -     (org-narrow-to-subtree)))
> > +    (if (eq keep-restriction 'subtree-at-point)
> > +     (progn
> > +       (or (org-at-heading-p) (error "Not at heading"))
> > +       (widen)
> > +       (org-narrow-to-subtree)
> > +       (setq-local org-datetree-base-level
> > +                   (org-get-valid-level (org-current-level) 1)))
> > +      (or keep-restriction (widen))
> > +      ;; support the old way of tree placement, using a property
> > +      (let ((prop (org-find-property "WEEK_TREE")))
> > +     (when prop
> > +       (goto-char prop)
> > +       (setq-local org-datetree-base-level
> > +                   (org-get-valid-level (org-current-level) 1))
> > +       (org-narrow-to-subtree))))
>
> See above, since the same suggestions apply. Also, it looks like there
> is some code duplication involved here. Would it make sense to factor
> the common part out of them?
>

I'll take a look if it does make sense and do it if it is easy.  I see it
as a separate issue since the week tree was implemented using a copy of the
date tree function.  But I can merge this change into the patch I am making.


>
> >      (goto-char (point-min))
> >      (require 'cal-iso)
> >      (let* ((year (calendar-extract-year d))
> >       Modified lisp/org.el
> > diff --git a/lisp/org.el b/lisp/org.el
> > index 38fce70..ac56d71 100644
> > --- a/lisp/org.el
> > +++ b/lisp/org.el
> > @@ -16615,6 +16615,8 @@ only headings."
> >        end found flevel)
> >      (unless buffer (error "File not found :%s" file))
> >      (with-current-buffer buffer
> > +      (unless (derived-mode-p 'org-mode)
> > +     (error "Buffer %s needs to be in org-mode" buffer))
>
> Nitpick: "Buffer %s needs to be in Org mode"
>

OK


>
> Ideally, a bunch of tests in test-org-capture.el would be nice.
>

Will do so after we have converged.

Regards

Carsten


>
>
> Regards,
>
> --
> Nicolas Goaziou
>

[-- Attachment #2: Type: text/html, Size: 24793 bytes --]