Hi Nicolas, Thanks for your detailed comments. Much appreciated. > -----Original Message----- > From: Nicolas Goaziou > Sent: den 4 november 2018 23:37 > To: Gustav Wikström > Cc: emacs-orgmode@gnu.org > Subject: Re: FW: [RFC] Link-type for attachments, more attach options > > Hello, > > Gustav Wikström 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: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. > > 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