Re: [RFC] Link-type for attachments, more attach options

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Gustav Wikström <gustav@whil.se> writes:

> Some updates, and a fresh patch...!

Thank you.
>
> I won't say that much here since the patch speaks for itself (It 
> contains a chapter to be added to ORG-NEWS). It's patch 0002 that is the 
> important one. See sidenote below for a comment about patch 0001.
>
> I have to mention that the patch has grown quite a lot. With the 
> Additional changes, I've taken the liberty to promote attachments to a 
> level-1 headline within the documentation!

Hmm. I don't think it should be a top-level information. More on this below.

> Inheriting ID's for attachments is actually valuable. It makes it possible, 
> for example, to add attachment-links in subheadings to the node the  
> attachment is defined on. After having used this for quite some time now it
> feels quite natural to have inheritance switched on for attachments. No negative
> side-effects at all.

OK.

> (Sidenote 1; what do I need to do to gain access to the official 
> git-repository? Would be nice to be able to provide pull requests there!)

Pull requests are not the preferred way to interact with the project.
However, I think you should have write access to the repository. I'm
Cc'ing Bastien about it, as I cannot do it myself.

> (Sidenote 2; The first patch is some minor unrelated fixups - it 
> breaks test-org-export/expand-include as a side-effect. But that's probably 
> because the test doesn't do what it should! 😲 )

Probably. Yet, we should do something about it. If you cannot fix it, at
least please comment it out somehow.

>    (should
>     (equal
>      "1"
> -    (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** <point>H2"
> -      (org-entry-get (point) "A" t))))
> +    (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** H2"
> +      (org-entry-get (point-max) "A" t))))

Not that it is wrong, but I don't see the benefit of this change.

>    (should
>     (equal
>      "1"
> -    (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** <point>H2"
> +    (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** H2"

Ditto.

>        (let ((org-use-property-inheritance t))
> -	(org-entry-get (point) "A" 'selective)))))
> +	(org-entry-get (point-max) "A" 'selective)))))

Ditto.

>    (should-not
> -   (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** <point>H2"
> +   (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** H2"
>       (let ((org-use-property-inheritance nil))
> -       (org-entry-get (point) "A" 'selective))))
> +       (org-entry-get (point-max) "A" 'selective))))

Ditto.

>  (defmacro org-test-with-temp-text-in-file (text &rest body)
> -  "Run body in a temporary file buffer with Org mode as the active mode."
> +  "Run body in a temporary file buffer with Org mode as the active mode.
> +If the string \"<point>\" appears in TEXT then remove it and
> +place the point there before running BODY, otherwise place the
> +point at the beginning of the buffer."
>    (declare (indent 1))
>    (let ((results (cl-gensym)))
>      `(let ((file (make-temp-file "org-test"))
> @@ -207,6 +210,8 @@ otherwise place the point at the beginning of the inserted text."
>  	   ,results)
>         (with-temp-file file (insert inside-text))
>         (find-file file)
> +       (when (re-search-forward "<point>" nil t)
> +	 (replace-match ""))
>         (org-mode)
>         (setq ,results (progn ,@body))
>         (save-buffer) (kill-buffer (current-buffer))

While we're at fixing this macro, we may also wrap stuff within
`unwind-protect' so as to properly delete temporary files in any cases.

> From 0180a900f890b2053d8184116c99c62cfa083055 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Gustav=20Wikstr=C3=B6m?= <gustav@whil.se>
> Date: Sun, 25 Nov 2018 21:38:44 +0100
> Subject: [PATCH 2/2] org-attach*, org, org-manual, org-news,
>  testing/**/test-org-attach*
>
> * org-attach.el
>
> Changed the way attachments deal with property-inheritance.  It now
> adheres to the =org-use-property-inheritance= setting by default but
> it can be customized if needed (I recommend to enable it!).
> The property ATTACH_DIR is depricated in favour of the shorter and simpler
> property DIR.

depricated -> deprecated

> * org-manual.org
>
> Attachments have been promoted into its own top-level node!  There are
> a lot of goodies to mention here that didn't fit naturally with
> "capture" and "archive" before.

This is true, but there is not enough information to require a top-level
node.

Here is a suggestion. Split "Capture, Refile, Archive" into two parts:

1. Refile, Copy and Archiving in one place, because they all relate to
   moving existing data around

2. Capture, Attachments, RSS Feeds, Protocols in another place, because
   they all insert external data to the system.

WDYT?

> +File links and attachment links can contain additional information to

Since attachment links are file links, it seems redundant to talk about
both everywhere. Just clarify clearly once that attachments links are
file links for all purposes.

> +It is often useful to associate reference material with an outline
> +node/task.

I would use "node" only: it is more generic.

> +The attachment system has been contributed to Org by John Wiegley.

I would drop this reference, since John Wiegley is already mentioned in
the Thanks entry.

> +** Attachment defaults and dispatcher
> +By default, org-attach will use ID properties when adding attachments
> +to outline nodes.  This makes working with attachments fully
> +automated.  There is no decision needed for folder-name or location.
> +ID-based directories are by default located in the =data/= directory,
> +which lives in the same directory where your Org file lives[fn:87].
> +For more control over the setup, see [[*Attachment options]].
> +
> +When attachments are made using ~org-attach~ a default tag =ATTACH= is
> +added to the node that gets the attachments.

Note overly important, but we prefer the active voice whenever possible.

> +  #+begin_example
> +  ,* Chapter A ...
> +    :PROPERTIES:
> +    :DIR: Chapter A/
> +    :END:
> +  ,** Introduction
> +  Some text
> +
> +  #+NAME: Image 1
> +  [[Attachment:image 1.jpg]]
> +  #+end_example
> +
> +  Without inheritance one would not be able to resolve the link to image
> +  1.jpg, since the link is inside a sub-heading to Chapter A.

=1.jpg=  ... =Chapter A=

> +  Inheritance works the same way for both =ID= and =DIR= property.  If
> +  both properties are defined on the same headline then =DIR= takes
> +  precedance.  This is also true if inheritance is enabled.  If =DIR= is
> +  inherited from a parent node in the outline, that property still takes
> +  precedence over an =ID= property defined on the node itself.
> +
> +- ~org-attach-method~ ::
> +  #+vindex: org-attach-method
> +  When attaching files using the dispatcher {{{kbd(C-c C-a)}}} it
> +  defaults to copying files.  The behaviour can be changed by
> +  customizing ~org-attach-method~.  Options are =Copy=, =Move/Rename=,
> +  =Hard link= or =Symbolic link=.

I don't think =...= are warranted here.

> +- ~org-attach-preferred-new-method~ ::
> +  #+vindex: org-attach-preferred-new-method
> +  This customization lets you choose the default way to attach to
> +  nodes without existing ID and DIR property.  It defaults to =id= but
> +  can also be set to =dir=, =ask= or =nil=.

~id~, ~dir~ ~ask~ or ~nil~, since those are symbols.

> +  Do not show the splash buffer with the attach dispatcher when
> +  ~org-attach-expert~ is set to Non-nil.

non-~nil~

> +** Attachment links
> +Attached files and folders can be referenced using attachment-links.

Why the hyphen in "attachment-links"? I think "attachment links" is
perfectly clear.

> +This makes it easy to refer to the material added to an outline node.
> +Especially if it was attached using the unique ID of the entry!
> +
> +#+begin_example
> +,* TODO Some task
> +  :PROPERTIES:
> +  :ID:       95d50008-c12e-479f-a4f2-cc0238205319
> +  :END:
> +See attached document for more information: [[attachment:info.org]]
> +#+end_example
> +
> +See [[* External Links]] for more information about these links.

No need for the space: [[*External Links]]

> +** Automatic version-control with git

Shouldn't it be "Git"?

> +If the directory attached to an outline node is a git repository, Org

Ditto.

> +can be configured to automatically commit changes to that repository
> +when it sees them.
> +
> +To make org-mode take care of versioning of attachments for you, add

To make Org mode...

> +(defcustom org-attach-dir-relative nil
> +  "Choose whether directories in DIR property should be added as relative links or not.
> +Defaults to absolute location."

First line is too long, maybe:

    Non-nil means directories in DIR property are added as relative links

> -(defcustom org-attach-annex-auto-get 'ask
> -  "Confirmation preference for automatically getting annex files.
> -If \\='ask, prompt using `y-or-n-p'.  If t, always get.  If nil, never get."
> +(defun org-attach-id-folder-format (id)
> +  (format "%s/%s"
> +	  (substring id 0 2)
> +	  (substring id 2))
> +  )

No dangling parens, please. Also, this function needs a docstring.

> +(defcustom org-attach-id-to-path-function #'org-attach-id-folder-format
> +  "The function parsing the ID parameter into a folder-path"

Missing full stop. Also: "Function parsing..."

>    :group 'org-attach
> -  :package-version '(Org . "9.0")
> -  :version "26.1"
> -  :type '(choice
> -	  (const :tag "confirm with `y-or-n-p'" ask)
> -	  (const :tag "always get from annex if necessary" t)
> -	  (const :tag "never get from annex" nil)))
> +  :package-version '(Org . "9.3")
> +  :type 'function
> +  )

See above.

> +  "Return the directory associated with the current outline node.
> +First check for DIR property, then ID property.
> +`org-attach-use-inheritance' determines whether inherited
> +properties also will be considered.
> +
> +If an ID property is found the default mechanism using that ID
> +will be invoked to access the directory for the current entry.
> +
> +If CREATE-IF-NOT-EXIST-P is non-nil, `org-attach-dir-get-create'
> +is run."
> +  (if create-if-not-exists-p
> +      (org-attach-dir-get-create)

Could you merge the `if' within the `cond' below?

> +    (let (attach-dir id)
> +      (cond
> +       ((setq attach-dir (org-entry-get nil "DIR" org-attach-use-inheritance))
> +	(org-attach-check-absolute-path attach-dir))
> +       ;; Depricated and removed from documentation, but still

Deprecated. Also, please add a FIXME.

> +    (if attach-dir
> +	(progn (if (not (file-directory-p attach-dir))
> +		   (make-directory attach-dir t))
> +	       (if (file-exists-p attach-dir)
> +		   attach-dir
> +		 (error "Path does not exist: %s." attach-dir)))
> +      (error "No attachment directory is associated with the current node."))))

    (unless attach-dir (error "No attachment ... current node"))
    (unless (file-directory-p attach-dir) (make-directory ...))
    attach-dir

Note that error messages never end with a full stop in Emacs. Also,
I don't see how the inner error could trigger since you create the
directory just before, and it would already have returned an error if
that wasn't possible.

> -      (shell-command (format "rm -fr %s" attach-dir))
> +    (when (and attach-dir
> +	       (or force
> +		   (y-or-n-p "Are you sure you want to remove all
> attachments of this entry? ")))

Maybe the less mouthful:

   "Really remove all attachments of this entry? "

Also, what about using `yes-or-no-p' since this looks a sensitive operation?

> +	(org-attach-tag (not files))))
> +    (when (not attach-dir)
> +      (org-attach-tag t))))

    (unless attach-dir (org-attach-tag t))

> +  (interactive)
> +  (let ((attach-dir (org-attach-dir)))
> +    (if attach-dir
> +	(org-open-file attach-dir)
> +      (error "No attachment directory exist."))))

See above: no full stop at the end of error messages.

> +  (let ((attach-dir (org-attach-dir)))
> +    (if attach-dir
> +	(dired attach-dir)
> +      (error "No attachment directory exist."))))

Ditto.
>  

> +  (let ((attach-dir (org-attach-dir)))
> +    (if attach-dir
> +	(let* ((files (org-attach-file-list attach-dir))
> +	       (file (if (= (length files) 1)
> +			 (car files)
> +		       (completing-read "Open attachment: "
> +					(mapcar #'list files) nil t)))

I think a `pcase' is warranted here:

    (pcase (org-attach-file-list attach-dir)
      (`(,file) file)
      (files (completing-read ...)))

> +	       (path (expand-file-name file attach-dir)))
> +	  (run-hook-with-args 'org-attach-open-hook path)
> +	  (org-open-file path in-emacs))
> +      (error "No attachment directory exist."))))

Ahem. :)

> diff --git a/testing/examples/att1/fileA b/testing/examples/att1/fileA

Would it be possible to use `org-test-with-temp-text-in file instead of
creating files?

> +(ert-deftest test-org-attach/dir ()
> +  "Test `org-attach-get' specifications."
> +  (org-test-in-example-file org-test-attachments-file
> +    ;; Link inside H1
> +    (org-next-link)
> +    (save-excursion
> +      (org-open-at-point)
> +      (should (equal "Text in fileA\n" (buffer-string))))

Please move `should' in the outermost part of the sexp, if possible. It
makes debugging easier (i.e., evaluating the sexp without getting
`should' in the way).

I probably missed a few things, but it keeps the ball rolling.

Thank you again for all the hard work.

Regards,

-- 
Nicolas Goaziou