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

[Gustav Wikström]

Hi Nicolas,

Thanks for your detailed comments. Much appreciated.

> -----Original Message-----
> From: Nicolas Goaziou <address@hidden>
> Sent: den 4 november 2018 23:37
> To: Gustav Wikström <address@hidden>
> Cc: address@hidden
> Subject: Re: FW: [RFC] Link-type for attachments, more attach options
> 
> Hello,
> 
> Gustav Wikström <address@hidden> 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?

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.

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

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.

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

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

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

Ofc, sorry for that.

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

Done.

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

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?

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

Ok

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

Ah, yes ofc. 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 load-file-name
> > +                                                     buffer-file-name)))
> 
> Why `load-file-name'? Why not using `default-directory'?

Good question. I don't remember having any particular arguments for it. I'll 
change to your suggestion.
 
> 
> > +(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.

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

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

Same comment as above.

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

Same comment as above.

> 
> Regards,
> 
> --
> Nicolas Goaziou

Thanks again for your comments. Updated patch is attached/att/@!

Kind regards,
Gustav

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