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

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

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

> Yeah - I liked "attached" because I prefer clear keywords. But sure,
> we can keep it shorter. I'd suggest "@" instead in that case. Patch
> updated with that.

"@" syntax is a reserved syntax for citations in the "wip-cite" branch.
I'd rather not use it here. Also, years ago, "att" link type was used to
link to attachments, hence my proposal.

>> > 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'?

You didn't answer this question, did you?

Something is fishy here anyways. Why is "ATTACH_DIR_INHERIT" needed at
all? If `org-attach-allow-inheritance', "ATTACH_DIR" should be
inherited. Why depend on another property?

I'd rather not make these things more complex.

>> > - org-attach-dir-create-if-not-exist
>> 
>> What is the use-case for this one? It doesn't seem terribly useful at 
>> first glance.
>
> If you try to open attachments on a node where there is no ID or
>> ATTACH_DIR, the default behavior is to add an ID and create a folder
>> based on that ID. I would like Org-mode to not do that. This
>> customization give the user the option to change that. Instead of
>> providing this customization one could just change the default
>> behavior of org-attach, since it's a bit offensive to create folders
>> when I didn't ask for it. But instead of changing the default,
>> I thought this way was more graceful. I wouldn't mind skipping this
>> customization though, if the behavior was changed to what
>> I implemented with org-attach-dir-create-if-not-exist set to nil.

Considering attaching is about moving, or copying files somewhere,
creating a folder doesn't sound terribly offensive. You don't even have
to know the name of the folder.

Do you suggest to raise an error if there is no folder available for the
attached documents? If so, wouldn't it be better to ask the user first?

>> > - org-attach-dir-relative
>> 
>> I'm not sure to understand this one.
>
> When adding folders to nodes using option "s" in org-attach, the path
> is absolute. This makes attachment-directory paths relative to
> location of the file instead.

OK.

>> > +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.
>> 
>
> Currently the list in the documentation is just a bunch of examples.
>> I've looked at it a couple of times asking myself "What is the
>> complete list of built in link types?". This commit "fixes" that.
>> I wouldn't say its redundant since all the rows in the second list
>> are just examples.

It is still redundant. For example, the first table has

 | info | Info link |

whereas the second one tells us

 | info:org#External links | Info node link |

In this case,

 | Info link | info:org#External links |

would be sufficient enough. I have the feeling documentation can be
improved here.

Also, file+sys and file+emacs are deprecated. They can be removed.

> I can't say I understand the use of :safe here. But added it anyways.
> If you care, please explain why it's needed. Package-version is added.
> I set it to 9.2. Correct?

If :safe is not set, Emacs will warn every time the variable is set as
a local file variable.

It should be 9.3, not 9.2.

>> > +(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.
>
> Sorry, I don't understand what I'm supposed to do here... I changed
> the comment to (maybe?) make it read better. Other than that, I'm at
> a loss.

Every argument needs to be documented in the docstring. What is LINK
type? What is IN-EMACS?

>> > +	    (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.
>
> This is when parsing the buffer for images. I couldn't get org-mode to
> display images without this.

This is still a hack, and clearly not the way to go, IMO. If not already
possible, we could add a new parameter in `org-link-parameters' or some
such. This is another issue, tho.

> +(defcustom org-attach-dir-create-if-not-exists t
> +  "Choose whether ATTACH_DIR-directories should be created if they do
> not exist since before.

Too long. Maybe:

When non-nil, ATTACH_DIR is created automatically if it doesn't exist.
Otherwise, ...

> +Default is to create them."
> +  :group 'org-attach
> +  :type 'boolean
> +  :package-version '(Org . "9.2")
> +  :safe #'booleanp)
> +
> +(defcustom org-attach-dir-relative nil
> +  "Choose whether ATTACH_DIR-directories should be added as relative links or not.

Too long. Maybe something like:

Non-nil means ATTACH_DIR is relative to the attachment node directory. 

> +Defaults to not relative."

Defaults to absolute location.

> +  (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 default-directory
> +							  buffer-file-name)))
> +		    (attach-dir-relative (file-relative-name attach-dir current-dir)))
> +	       (if org-attach-dir-relative
> +		   (org-entry-put nil "ATTACH_DIR" attach-dir-relative)
> +		 (org-entry-put nil "ATTACH_DIR" attach-dir))))

(org-entry-put nil "ATTACH_DIR" (if org-attach-dir-relative 
                                    attach-dir-relative
                                  attach-dir))

> +(defun org-attach-open-link (link &optional in-emacs)
> +  "Attach link type LINK is expanded with the attached directory and opened.
> +This is done in the same way as file-links are opened."
> +  (interactive "P")
> +  (let (line search)
> +    (if (string-match "::\\([0-9]+\\)\\'" link)
> +        (setq line (string-to-number (match-string 1 link))
> +              link (substring link 0 (match-beginning 0)))
> +      (if (string-match "::\\(.+\\)\\'" link)
> +          (setq search (match-string 1 link)
> +                link (substring link 0 (match-beginning 0)))))

Use `cond' instead.

> +    (if (string-match "[*?{]" (file-name-nondirectory link))
> +        (dired (org-attach-expand link))
> +      (org-open-file (org-attach-expand link) in-emacs line search))))
> +
> +(defun org-attach-complete-link ()
> +  "Advise the user with the available files in the attachment directory."
> +  (let (file link attached-dir)
> +    (setq attached-dir (expand-file-name (org-attach-dir)))
> +    (setq file (read-file-name "File: " attached-dir))
> +    (let ((pwd (file-name-as-directory attached-dir))
> +          (pwd1 (file-name-as-directory (abbreviate-file-name
> +                                         attached-dir))))
> +      (cond
> +       ((string-match (concat "^" (regexp-quote pwd1) "\\(.+\\)") file)
> +        (setq link  (concat "@:" (match-string 1 file))))
> +       ((string-match (concat "^" (regexp-quote pwd) "\\(.+\\)")
> +                      (expand-file-name file))
> +        (setq link  (concat
> +                     "@:" (match-string 1 (expand-file-name file)))))
> +       (t (setq link (concat "@:" file)))))
> +    link))

I suggest:

  (let* ((attached-dir (expand-file-name (org-attach-dir)))
         (file (read-file-name "File: " attached-dir))
         (pwd (file-name-as-directory attached-dir))
         (pwd-relative (file-name-as-directory
                        (abbreviate-file-name attached-dir))))
   (cond
    ((string-match ...)
     (concat ...))
    ...
    (t (concat ...))))

> +(defun org-attach-export-link (link description format)
> +  "Translate \"attached\" (@) LINK from Org mode format to exported FORMAT.
> +Also includes the DESCRIPTION of the link in the export."
> +  (save-excursion
> +    (let (path desc)
> +      (if (string-match "::\\([0-9]+\\)\\'" link)
> +          (setq link (substring link 0 (match-beginning 0)))
> +        (if (string-match "::\\(.+\\)\\'" link)
> +            (setq link (substring link 0 (match-beginning 0)))))

Use `cond' instead.

> +      (search-forward (concat "@:" (org-link-escape link)))

What is the use for the line above?

> diff --git a/lisp/org.el b/lisp/org.el
> index eb1affbc7..9ed663bb9 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -4428,6 +4428,7 @@ This is needed for font-lock setup.")
>  		  (beg end))
>  (declare-function org-agenda-set-restriction-lock "org-agenda" (&optional type))
>  (declare-function org-agenda-skip "org-agenda" ())
> +(declare-function org-attach-expand "org-attach" (&optional if-exists))
>  (declare-function org-attach-reveal "org-attach" (&optional if-exists))
>  (declare-function org-gnus-follow-link "org-gnus" (&optional group article))
>  (declare-function org-indent-mode "org-indent" (&optional arg))
> @@ -18721,7 +18722,7 @@ boundaries."
>  	    ;; Check absolute, relative file names and explicit
>  	    ;; "file:" links.  Also check link abbreviations since
>  	    ;; some might expand to "file" links.
> -	    (file-types-re (format "[][]\\[\\(?:file\\|[./~]%s\\)"
> +	    (file-types-re (format "[][]\\[\\(?:file\\|@\\|[./~]%s\\)"
>  				   (if (not link-abbrevs) ""
>  				     (format "\\|\\(?:%s:\\)"
>  					     (regexp-opt link-abbrevs))))))
> @@ -18730,14 +18731,20 @@ boundaries."
>  	   ;; Check if we're at an inline image, i.e., an image file
>  	   ;; link without a description (unless INCLUDE-LINKED is
>  	   ;; non-nil).
> -	   (when (and (equal "file" (org-element-property :type link))
> +	   (when (and (or (equal "file" (org-element-property :type link))
> +                          (equal "@" (org-element-property :type link)))
>  		      (or include-linked
>  			  (null (org-element-contents link)))
>  		      (string-match-p file-extension-re
>  				      (org-element-property :path link)))
> -	     (let ((file (expand-file-name
> -			  (org-link-unescape
> -			   (org-element-property :path link)))))
> +	     (let ((file (if (equal "@" (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))))))
>  	       (when (file-exists-p file)
>  		 (let ((width
>  			;; Apply `org-image-actual-width' specifications.

This part can be omitted for now. Something better has to be found.

Thank you.

Regards,

-- 
Nicolas Goaziou