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

[Gustav Wikström]

Hi again,

> -----Original Message-----
> From: Nicolas Goaziou <address@hidden>
> Sent: den 20 november 2018 00:52
> To: Gustav Wikström <address@hidden>
> Cc: Org Mode List <address@hidden>
> Subject: Re: [RFC] Link-type for attachments, more attach options
> 
> > 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.

Too bad, "@" was growing on me. @ is currently automatically set as a tag when 
attaching files to nodes. So it was fitting to use it also in links in my 
opinion. Is the cite-syntax the same as the regular link-pattern? [[@: ...]] ? 
Otherwise I'd suggest them to coexist. Or to change the cite-symbol to ... "c" 
maybe? @ is a pretty standard symbol for attachments. Just have a look at 
Gmail, Outlook etc.

I didn't change this in the attached patch. I'm hoping for second thoughts! :) 
The future ease of use (i.e. clarity and standardization of symbols in this 
case) should have precedence over current work in progress...

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

No, seems I missed it. 

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

Yeah, I don't know why it's configured like that from the start. A bit 
convoluted. Not sure of what way to go forward though. I can see at least two 
paths:

1) Rename `org-attach-allow-inheritance' to `org-attach-enable-inheritance' and 
always make attachments inherited when that is set to "t". Deprecate 
"ATTACH_DIR_INHERIT". With this I'd also change the precedence-logic between 
ATTACH_DIR & ID properties and make ID-based attachment inherit as well. The 
properties ATTACH_DIR and ID should be inherited from the closest node when 
resolving attachments, with ATTACH_DIR having precedence over ID if both exist 
for the same node.

2) remove `org-attach-allow-inheritance' and only rely on the 
"ATTACH_DIR_INHERIT" property of any of the parent nodes to decide if the 
"ATTACH_DIR" property should be inherited. This would be a smaller change from 
the user's perspective, where we're basically saying you cannot disable 
inheritance, but it's only active when a node has the 
ATTACH_DIR_INHERIT-property.

I prefer path (1). That would be a great default. But that change is bigger and 
should be separated from this patch anyways. To not increase the complexity 
I've removed the `org-attach-dir-inherit-by-default' customization.

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

Agreed, the parameter can be removed and a "do you want to create a folder?" 
question could be raised instead, when it's not intuitive for the program to 
create the folder by itself.

> >> > +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:address@hidden:papers/last.pdf= | file, path on remote 
> >> > machine        |
> >> > +| =/ssh:address@hidden: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:address@hidden                 | 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.

The problem I had with the second list is that it's just a list of examples. 
Nowhere does it say it's the complete list of commands. Anyways, I've tried to 
merge the two lists. Hope you'll find it more to your liking.

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

Ok, 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.

Ok

> 
> It should be 9.3, not 9.2.

Ok, fixed

> 
> >> > +(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?

Ok, got it.
> 
> >> > +            (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.

Ok, sure. Although to be fair it's an existing hack, which was expanded just a 
tiny bit. Removed anyways.
> 
> > +(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.

Yeah, that's better. Fixed.

> 
> > +  (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))

Yeah, that's nicer. Changed.

> 
> > +(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.

Ok.

> 
> > +    (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 ...))))

Yeah, clearly an improvement.

> 
> > +(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.

Ok.

> 
> > +      (search-forward (concat "@:" (org-link-escape link)))
> 
> What is the use for the line above?

Hmm, good question. Looking through my commit history I find no reason to 
justify it... I wonder how that got there. Effectively, I guess that row does 
nothing except stealing a few CPU-cycles... Removed. 

> > 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.

Ok, sure. I'll keep it on my local branch but it's omitted from the patch.

> 
> Thank you.
> 
> Regards,
> 
> --
> Nicolas Goaziou

New patch attached updated according to the comments.

Regards,
Gustav

0001-org-attach-org-manual-org-New-link-type-new-option.patch
Description: 0001-org-attach-org-manual-org-New-link-type-new-option.patch