Hi again, > -----Original Message----- > From: Nicolas Goaziou > Sent: den 20 november 2018 00:52 > To: Gustav Wikström > Cc: Org Mode List > 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: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. 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