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

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

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

> I’ve attached a patch with some suggested additions to org-attach.
> Patch comments below. Please review.

Thank you. Some comments follow.

> * Add new linktype "attached" for attachments

This seem a bit long. Why not "att" links?

> * Add further options for ATTACH_DIR
>
> When working with ATTACH_DIR there are now a couple of new options available:
> - org-attach-dir-inherit-by-default

What is the difference between this and `org-attach-allow-inheritance'?

> - org-attach-dir-create-if-not-exist

What is the use-case for this one? It doesn't seem terribly useful at
first glance.

> - org-attach-dir-relative

I'm not sure to understand this one.
>
> +This list shows the full set of built-in external link types:
> +| http       | web                                 |
> +| https      | secure web                          |
> +| doi        | DOI for electronic resources        |
> +| file       | file-links                          |
> +| file+sys   | file-links forced to open via OS    |
> +| file+emacs | file-links forced to open via emacs |
> +| attached   | links to attachments for nodes      |
> +| docview    | doc-view mode                       |
> +| id         | Link to heading by id               |
> +| news       | Usenet link                         |
> +| mailto     | mail link                           |
> +| mhe        | MH-E folder link                    |
> +| rmail      | Rmail link                          |
> +| gnus       | Gnus link                           |
> +| bbdb       | BBDB link                           |
> +| irc        | IRC link                            |
> +| info       | Info link                           |
> +| shell      | shell command                       |
> +| elisp      | interactive elisp command link      |
> +
> +The following list shows examples for each link type.
> +
> +| =http://www.astro.uva.nl/=dominik=        | on the web                          |
> +| =doi:10.1000/182=                         | DOI for an electronic resource      |
> +| =file:/home/dominik/images/jupiter.jpg=   | file, absolute path                 |
> +| =/home/dominik/images/jupiter.jpg=        | same as above                       |
> +| =file:papers/last.pdf=                    | file, relative path                 |
> +| =./papers/last.pdf=                       | same as above                       |
> +| =file:/ssh:me@some.where:papers/last.pdf= | file, path on remote machine        |
> +| =/ssh:me@some.where:papers/last.pdf=      | same as above                       |
> +| =file:sometextfile::NNN=                  | file, jump to line number           |
> +| =file:projects.org=                       | another Org file                    |
> +| =file:projects.org::some words=           | text search in Org file[fn:28]      |
> +| =file:projects.org::*task title=          | heading search in Org file          |
> +| =file+sys:/path/to/file=                  | open via OS, like double-click      |
> +| =file+emacs:/path/to/file=                | force opening by Emacs              |
> +| =attached:projects.org=                   | file in folder attached to headline |
> +| =attached:projects.org::some words=       | text search in attached file        |
> +| =docview:papers/last.pdf::NNN=            | open in doc-view mode at page       |
> +| =id:B7423F4D-2E8A-471B-8810-C40F074717E9= | link to heading by ID               |
> +| =news:comp.emacs=                         | Usenet link                         |
> +| =mailto:adent@galaxy.net=                 | mail link                           |
> +| =mhe:folder=                              | MH-E folder link                    |
> +| =mhe:folder#id=                           | MH-E message link                   |
> +| =rmail:folder=                            | Rmail folder link                   |
> +| =rmail:folder#id=                         | Rmail message link                  |
> +| =gnus:group=                              | Gnus group link                     |
> +| =gnus:group#id=                           | Gnus article link                   |
> +| =bbdb:R.*Stallman=                        | BBDB link (with regexp)             |
> +| =irc:/irc.com/#emacs/bob=                 | IRC link                            |
> +| =info:org#External links=                 | Info node link                      |
> +| =shell:ls *.org=                          | shell command                       |
> +| =elisp:org-agenda=                        | interactive Elisp command           |
> +| =elisp:(find-file "Elisp.org")=           | Elisp form to evaluate              |

I'm not sure to like the change above. It introduces a lot of
redundancy.

>  Here is the syntax of the different ways to attach a search to a file
>  link, together with explanations for each:
>  
>  #+begin_example
> -[[file:~/code/main.c::255]]
> -[[file:~/xx.org::My Target]]
> -[[file:~/xx.org::*My Target]]
> -[[file:~/xx.org::#my-custom-id]]
> -[[file:~/xx.org::/regexp/]]
> +  [[file:~/code/main.c::255]]
> +  [[file:~/xx.org::My Target]]
> +  [[file:~/xx.org::*My Target]]
> +  [[file:~/xx.org::#my-custom-id]]
> +  [[file:~/xx.org::/regexp/]]
> +  [[attached:~/code/main.c::255]]
>  #+end_example

Please discard this change.

> +Another method is /attachments/, which are files located in a
> +directory belonging to an outline node.  Org uses directories either
> +named by the unique ID of each entry, or by the =ATTACH_DIR= property.
> +ID-based directories are by default located in the ~data~ directory

... =data/= directory, which ...

> +(defcustom org-attach-dir-inherit-by-default nil
> +  "Defines whether ATTACH_DIR-directories should be inherited by
> +  subheadings by default when created. Defaults to not being
> +  inherited."
> +  :group 'org-attach
> +  :type 'boolean)

First line in a docstring must be a full sentence.  Indentation is wrong
in the subsequent lines.  Also, the `defcustom' is missing keywords,
e.g., :package-version and :safe.

> +(defcustom org-attach-dir-create-if-not-exists t
> +  "Choose whether ATTACH_DIR-directories should be created if
> +they do not exist since before. Default is to create them."
> +  :group 'org-attach
> +  :type 'boolean)
> +
> +(defcustom org-attach-dir-relative nil
> +  "Choose whether ATTACH_DIR-directories should be added as
> +relative links or not. Defaults to not relative."
> +  :group 'org-attach
> +  :type 'boolean)

Ditto.

>  (defcustom org-attach-commit t
>    "If non-nil commit attachments with git.
>  This is only done if the Org file is in a git repository."
> @@ -280,20 +299,26 @@ Throw an error if we cannot root the directory."
>    "Set the ATTACH_DIR node property and ask to move files there.
>  The property defines the directory that is used for attachments
>  of the entry.  When called with `\\[universal-argument]', reset \
> -the directory to
> -the default ID based one."
> +the directory to the default ID based one. Creates relative links

You need to use two spaces to separate sentences.

> +  (let ((old (org-attach-dir nil))
> +	(new
> +	 (progn
> +	   (if arg (org-entry-delete nil "ATTACH_DIR")
> +	     (let* ((attach-dir (read-directory-name
> +				 "Attachment directory: "
> +				 (org-entry-get nil
> +						"ATTACH_DIR")))
> +		    (current-dir (file-name-directory (or load-file-name
> +							  buffer-file-name)))

Why `load-file-name'? Why not using `default-directory'?

> +(defun org-attach-open-link (link &optional in-emacs)
> +  "LINK is expanded with the attached directory and opened the same
> +way as file-links are."

You need to expound the arguments in the docstring.

> +	    (file-types-re (format "[][]\\[\\(?:file\\|attached\\|[./~]%s\\)"
>  				   (if (not link-abbrevs) ""
>  				     (format "\\|\\(?:%s:\\)"
>  					     (regexp-opt link-abbrevs))))))

Why is it needed? "attached" link type is already registered, so you
don't need to also hard-code it here.

> +	   (when (and (or (equal "file" (org-element-property :type link))
> +                          (equal "attached" (org-element-property :type link)))

Ditto.

> +	     (let ((file (if (equal "attached" (org-element-property :type link))
> +			     (require 'org-attach)
> +                             (org-attach-expand
> +                              (org-link-unescape
> +			       (org-element-property :path link)))
> +			   (expand-file-name
> +			    (org-link-unescape
> +			     (org-element-property :path link))))))

Ditto.

Regards,

-- 
Nicolas Goaziou