RE: attachment: link type export to HTML invalid attach dir

["Gustav Wikström" <gustav@whil.se>]

[-- Attachment #1: Type: text/plain, Size: 10666 bytes --]

Hi again,

> -----Original Message-----
> From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
> Sent: den 20 januari 2020 02:25
> To: Gustav Wikström <gustav@whil.se>
> Cc: emacs-orgmode@gnu.org
> Subject: Re: attachment: link type export to HTML invalid attach dir
> 
> Gustav Wikström <gustav@whil.se> writes:
> 
> > Ok, noted. To my defense I was thinking more in the terms of subtyping
> > then hardcoding. Because we have multiple link types which try to
> > share an interface. But the interface isn't perfect for all different
> > kinds of links.
> 
> Then, I suggest to improve the interface.

That was kind of what I was trying to do, by allowing subtyping, so that the parser would be more knowledgeable about particular types of links, by allowing extended attributes. Maybe not implemented in the most extensible way though I'll admit.

My suggestion to the link parser would be to have the following properties as the catch-all properties for links:
- type :: Which type of link protocol applies. E.g. file, http, ftp, attachment, duckduckgo (ex. for a custom link abbreviation to search ddg)
- raw-path :: The path to use for the particular protocol. Would contain everything in the link following "protocol:"
- format :: Which format the link has. Plain, bracket or angular bracket
- description :: The description part of the link ([[protocol:path][description]])
- begin, end, post-blank :: The default properties used for all elements. Not sure if contents-begin and contents-end are a part here as well.

In addition to that each protocol would be able to declare their own properties using a :parse function. All would probably implement a path property. Some would implement an options property, and some maybe would maybe declare their own properties.

As you've stated previously, from a performance perspective maybe it will be best to not expand the specific properties and instead let the expansion of those be handled in code by the link type owner (e.g. org-attach.el for attachment links). But not letting specific protocol parsers expand their own properties into a link makes it more difficult to implement support outside of emacs for the org specification, which in my opinion includes also a certain set of link protocols which do have their own specifications.

> > So it doesn't seem too farfetched that some of those link types would
> > benefit from additional custom properties, specific for those types.
> > =application= and =search-option= for example isn't universally
> > applicable to all link types.
> 
> As a side note, :application is on its way out, i.e., "file+emacs" stuff
> is in "org-compat.el".
> 
> Also, IIRC, "docview" links have "go to page" option, and they don't
> touch the parser.

Ah yes. Just briefly looking at the docview code. Docview doesn't seem to use the parser when extracting information about that "go to page" information from the link. Which means docview links doesn't really care how the link information is encoded in the parser anyways. Maybe if docview were allowed to encode custom docview information into the parsed link in the parser it and others could reused that encoded information later? Docview would be able to add a ":go-to-page" option, for example. Just a thought.

> > What I'm trying to argue for here is: Maybe it's not that crazy after
> > all to allow links to have additional properties in the parser based
> > on its type? (While certainly still trying to avoid it if possible!)
> 
> If new link types may not function correctly without touching the
> parser, how do you create new link types out of Org's core? This is not
> modular at all.

Most link types probably won't need custom link properties anyways. But could maybe let the parser know stuff by use of higher order functions? Or push for being important enough to be included into Org and allowed to be known by the parser.

> > (Off topic) I'm sorry to hear that you think I'm intentionally making
> > things fuzzy.
> 
> Not at all! My wording is terrible. When I wrote
> 
>     Also, you sometimes seem to blur, on purpose, the difference between
>     "attachment" and "file" links.
> 
> I meant something like:
> 
>    It seems to me that your intent is to have "attachment" links
>    transparently become "file" links to the user.
> 
> Hence my suggestion to use link abbreviations. There's nothing negative
> in it.

Ok, got it. Thanks for explaining. I'll admit it hasn't been totally clear to me what the best way forward is. After refreshing my understanding on links the conclusion I came to was that link abbreviations in its current form was not a fitting concept for attachment links. Attachments are pretty much similar to file links though, so even if my intention isn't to blur the meaning they will and should be treated very similar, since functionality is so similar.

> > One can indeed use the buffer position to derive the part of the path
> > that comes from the subtree. But leaving it at that puts more
> > requirements on the user of the parsed link in order to use it.
> 
> And higher order functions in "org-attach" could take care of it. Note
> that expanding a link in the parser means all attachment links are
> always expanded, even if they are not used. There's a cost to consider.

Higher order functions were used for exporting attachments previously. But the intention to keep functionality as close to file links as possible made me move the code from there into the separate exporters, to let all functionality be inherited by the way file exports were done, with the minor tweak needed to make it work for attachment links as well.

I've attached a patch that lets org-attach take care of expanding the link element into a path, instead of sub-typing inside org-element. While I'm still pro sub-typing links according to their type, this patch removes that. I.e. the performance cost of parsing is moved to the points where the information actually is used, with a sort of "informational cost" of not letting the org parser know about the peculiarities of certain link types.
 
> Besides, parser focus on the contents of the buffer. I think it is out
> of its scope to infer file names for abbreviation links. It's more the
> job of the parts consuming the parsed data.
> 
> > There is no :raw-path available in the properties for a parsed link.
> > If there were we surely wouldn't have this conversation and I'd be
> > using that already! :)
> 
> I meant :raw-link, sorry.
> 
> > I.e. I like this suggestion. I considered using the :raw-link
> > property, but the way it's currently used by the parser (i.e.
> > expanding abbreviations) stopped me.
> 
> Please take how link abbreviations are handled in the parser out of the
> equation. I already stated this is not a good way to handle them. We
> should probably expand them in a "temp-link" variable, and parse it,
> while keeping original link in :raw-link (barring white spaces fixes).
> That would not be perfect either, because we would still be inferring
> stuff.
> 
> That's another topic, really. Let's just ignore it for now.
> 
> > So, we're discussing regarding attachment as either: an abbreviation
> > or a proper separate link type? I'm not sure what other options we
> > have? I personally can't categorize it as an abbreviation since that
> > is handled by a separate piece of code with other specifications. I.e.
> > attachment wouldn't fit well inside org-link-abbrev-alist.
> 
> What makes you think it wouldn't fit well?

That list seems more like a feature to let end users expand links using find-replace with a tag in an existing path. Actually looking more into it now I see that it can also use expansion with functions. So maybe that could be used as a way of implementation. I'm not sure if there are other side effects of that though so it's nothing I'm pursuing now.

> > Attachments should function in the same way as file links, with search
> > options and all. But be limited to the current set of attached files.
> 
> This isn't incompatible with the above, AFAICT.

The path for attachment links needs to be complemented with the part from the attachment directory to make it work in the file system. That is the difference between the link types. 

> > That's basically the way it was before the patches to fix stardiviners
> > issues. Except not functioning well enough. It would require quite
> > some code in org-attach to replicate existing file links
> > functionality. Mostly code duplication I presume.
> 
> I didn't read stardiviner issues. Would you mind summarizing them? Or,
> at least could you explain what is not functioning well enough?

The expansions of image links didn't work correctly for attachment links. Simply because org-attach-export-link didn't support that.

> Anyhow, you may have missed the "let's spot the needs and improve these
> tools" part. If current tools lead to code duplication, we can fix that.

Speaking from the perspective of attachment links, if there were a file link type exporter function available that could be used by attachment links without code duplication. Another thing might be to make the higher order functions in the interface for adding new link types be able to use the parsed link instead of just certain properties of the link. Not sure if performance would be an issue then though.

> > The raw-path option sounds better to my ears.
> 
> Except I was meaning :raw-link :)
> 
> > One improvement I can think of (outside of the discussion above) is to
> > make it possible to pass the argument to org-link-open along to each
> > separate link type specialization.
> 
> Isn't that the job of :follow when defining a new link type?

Yes, and the function that implements that would also need the user argument as input. In addition to the link path.

> > That one has bugged me for some time now, when I've wanted to force
> > opening attachment links in emacs but couldn't.
> 
> IMO, forcing users to open stuff in a specific, non-configurable, way is
> a bad idea. Why would we know better than them?

I was not clear enough with what I meant. I meant not to force the user in the code to a specific choice, but to let the user force emacs to open the link. By use of arg as is specified in org-link-open.

Anyways, patch attached that makes org-element not know about attachments any longer, and moves most of the responsibility to org-attach.el. The exporters still needs to know to use the new function inside org-attach.el though.

Regards
Gustav

[-- Attachment #2: attachment-links.patch --]
[-- Type: application/octet-stream, Size: 10107 bytes --]

diff --git a/lisp/ol.el b/lisp/ol.el
index 31c34ec7b..10ce83f50 100644
--- a/lisp/ol.el
+++ b/lisp/ol.el
@@ -75,6 +75,7 @@
 (declare-function org-src-source-type "org-src" ())
 (declare-function org-time-stamp-format "org" (&optional long inactive))
 (declare-function outline-next-heading "outline" ())
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
 
 \f
 ;;; Customization
@@ -934,7 +935,7 @@ a \"file\" link."
     (cond
      ((member type '("file" "attachment"))
       (when (string= type "attachment")
-	(setq path (org-element-property :attachment-path link)))
+	(setq path (org-attach-link-expand link)))
       (if (string-match "[*?{]" (file-name-nondirectory path))
 	  (dired path)
 	;; Look into `org-link-parameters' in order to find
diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index f5c81e01f..8709c8b88 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -41,6 +41,7 @@
 (require 'org-id)
 
 (declare-function dired-dwim-target-directory "dired-aux")
+(declare-function org-element-property "org-element" (property element))
 
 (defgroup org-attach nil
   "Options concerning attachments in Org mode."
@@ -640,6 +641,21 @@ See `org-attach-open'."
 Basically, this adds the path to the attachment directory."
   (expand-file-name file (org-attach-dir)))
 
+(defun org-attach-link-expand (link &optional buffer-or-name)
+  "Return the full path to the attachment in the LINK element.
+Takes LINK which is a link element, as defined by
+`org-element-link-parser'.  If LINK `:type' is attachment the
+full path to the attachment is expanded and returned.  Otherwise,
+return nil.  If BUFFER-OR-NAME is specified, LINK is expanded in
+that buffer, otherwise current buffer is assumed."
+  (let ((type (org-element-property :type link))
+	(file (org-element-property :path link))
+	(pos (org-element-property :begin link)))
+    (when (string= type "attachment")
+      (with-current-buffer (or buffer-or-name (current-buffer))
+	(goto-char pos)
+	(org-attach-expand file)))))
+
 (org-link-set-parameters "attachment"
                          :complete #'org-attach-complete-link)
 
diff --git a/lisp/org-element.el b/lisp/org-element.el
index 9a62c5b94..85663ef57 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -3116,11 +3116,7 @@ When at a link, return a list whose car is `link' and cdr a plist
 with `:type', `:path', `:format', `:raw-link', `:application',
 `:search-option', `:begin', `:end', `:contents-begin',
 `:contents-end' and `:post-blank' as keywords.  Otherwise, return
-nil.  Additionally, in the context of attachment links one
-further property, `:attachment-path' is set.  That property
-contains the attachment link expanded into a full filesystem
-path.
-
+nil.
 
 Assume point is at the beginning of the link."
   (catch 'no-object
@@ -3229,27 +3225,18 @@ Assume point is at the beginning of the link."
 	(when trans
 	  (setq type (car trans))
 	  (setq path (cdr trans))))
-      (let ((link
-	     (list 'link
-		   (list :type type
-			 :path path
-			 :format format
-			 :raw-link (or raw-link path)
-			 :application application
-			 :search-option search-option
-			 :begin begin
-			 :end end
-			 :contents-begin contents-begin
-			 :contents-end contents-end
-			 :post-blank post-blank))))
-	;; Add additional type specific properties for link types that
-	;; need it
-	(when (string= type "attachment")
-	  (org-element-put-property
-	   link :attachment-path
-	   (file-relative-name
-	    (org-attach-expand path))))
-	link))))
+      (list 'link
+	    (list :type type
+		  :path path
+		  :format format
+		  :raw-link (or raw-link path)
+		  :application application
+		  :search-option search-option
+		  :begin begin
+		  :end end
+		  :contents-begin contents-begin
+		  :contents-end contents-end
+		  :post-blank post-blank)))))
 
 (defun org-element-link-interpreter (link contents)
   "Interpret LINK object as Org syntax.
diff --git a/lisp/ox-ascii.el b/lisp/ox-ascii.el
index 019c26c24..4ffb44a97 100644
--- a/lisp/ox-ascii.el
+++ b/lisp/ox-ascii.el
@@ -34,6 +34,7 @@
 ;;; Function Declarations
 
 (declare-function aa2u "ext:ascii-art-to-unicode" ())
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
 
 ;;; Define Back-End
 ;;
@@ -1573,7 +1574,7 @@ INFO is a plist holding contextual information."
 	 (raw-path (org-element-property :path link))
 	 (path (cond
 		((string= type "attachment")
-		 (setq raw-path (org-element-property :attachment-path link))
+		 (setq raw-path (org-attach-link-expand link))
 		 (concat type ":" raw-path))
 		(t (concat type ":" raw-path)))))
     (cond
diff --git a/lisp/ox-html.el b/lisp/ox-html.el
index 39ccae3ea..fa30bde95 100644
--- a/lisp/ox-html.el
+++ b/lisp/ox-html.el
@@ -42,6 +42,7 @@
 (declare-function org-id-find-id-file "org-id" (id))
 (declare-function htmlize-region "ext:htmlize" (beg end))
 (declare-function mm-url-decode-entities "mm-url" ())
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
 
 (defvar htmlize-css-name-prefix)
 (defvar htmlize-output-type)
@@ -3074,7 +3075,7 @@ INFO is a plist holding contextual information.  See
 	    (url-encode-url (concat type ":" raw-path)))
 	   ((member type '("file" "attachment"))
 	    (when (string= type "attachment")
-	      (setq raw-path (org-element-property :attachment-path link)))
+	      (setq raw-path (org-attach-link-expand link)))
 	    ;; During publishing, turn absolute file names belonging
 	    ;; to base directory into relative file names.  Otherwise,
 	    ;; append "file" protocol to absolute file name.
diff --git a/lisp/ox-latex.el b/lisp/ox-latex.el
index b307ff49a..1bc15a818 100644
--- a/lisp/ox-latex.el
+++ b/lisp/ox-latex.el
@@ -32,6 +32,8 @@
 
 ;;; Function Declarations
 
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
+
 (defvar org-latex-default-packages-alist)
 (defvar org-latex-packages-alist)
 (defvar orgtbl-exp-regexp)
@@ -2360,7 +2362,7 @@ LINK is the link pointing to the inline image.  INFO is a plist
 used as a communication channel."
   (let* ((parent (org-export-get-parent-element link))
 	 (path (let ((raw-path (if (string= (org-element-property :type link) "attachment")
-				   (org-element-property :attachment-path link)
+				   (org-attach-link-expand link)
 				 (org-element-property :path link))))
 		 (if (not (file-name-absolute-p raw-path)) raw-path
 		   (expand-file-name raw-path))))
@@ -2528,7 +2530,7 @@ INFO is a plist holding contextual information.  See
 		       (concat type ":" raw-path))
 		      ((member type '("file" "attachment"))
 		       (when (string= type "attachment")
-			 (setq raw-path (org-element-property :attachment-path link)))
+			 (setq raw-path (org-attach-link-expand link)))
 		       (org-export-file-uri raw-path))
 		      (t
 		       raw-path)))))
diff --git a/lisp/ox-man.el b/lisp/ox-man.el
index 5de4c5ea5..b6925c696 100644
--- a/lisp/ox-man.el
+++ b/lisp/ox-man.el
@@ -42,6 +42,8 @@
 
 ;;; Function Declarations
 
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
+
 (defvar org-export-man-default-packages-alist)
 (defvar org-export-man-packages-alist)
 (defvar orgtbl-exp-regexp)
@@ -616,7 +618,7 @@ INFO is a plist holding contextual information.  See
                  (concat type ":" raw-path))
                 ((member type '("file" "attachment"))
 		 (when (string= type "attachment")
-		   (setq raw-path (org-element-property :attachment-path link)))
+		   (setq raw-path (org-attach-link-expand link)))
 		 (org-export-file-uri raw-path))
                 (t raw-path))))
     (cond
diff --git a/lisp/ox-md.el b/lisp/ox-md.el
index 61b31f987..7515df3a2 100644
--- a/lisp/ox-md.el
+++ b/lisp/ox-md.el
@@ -35,6 +35,8 @@
 \f
 ;;; Function Declarations
 
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
+
 ;;; User-Configurable Variables
 
 (defgroup org-export-md nil
@@ -405,7 +407,7 @@ INFO is a plist holding contextual information.  See
 		 (concat type ":" raw-path))
 		((member type '("file" "attachment"))
 		 (when (string= type "attachment")
-		   (setq raw-path (org-element-property :attachment-path link)))
+		   (setq raw-path (org-attach-link-expand link)))
 		 (org-export-file-uri (funcall link-org-files-as-md raw-path)))
 		(t raw-path))))
     (cond
diff --git a/lisp/ox-odt.el b/lisp/ox-odt.el
index 4f91e4a29..5d0ee7bfe 100644
--- a/lisp/ox-odt.el
+++ b/lisp/ox-odt.el
@@ -34,6 +34,8 @@
 
 ;;; Function Declarations
 
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
+
 ;;; Define Back-End
 
 (org-export-define-backend 'odt
@@ -2706,7 +2708,7 @@ INFO is a plist holding contextual information.  See
 		 (concat type ":" raw-path))
 		((member type '("file" "attachment"))
 		 (when (string= type "attachment")
-		   (setq raw-path (org-element-property :attachment-path link)))
+		   (setq raw-path (org-attach-link-expand link)))
 		 (org-export-file-uri raw-path))
 		(t raw-path)))
 	 ;; Convert & to &amp; for correct XML representation
diff --git a/lisp/ox-texinfo.el b/lisp/ox-texinfo.el
index 22ea86e1c..89c6746d8 100644
--- a/lisp/ox-texinfo.el
+++ b/lisp/ox-texinfo.el
@@ -30,6 +30,8 @@
 
 ;;; Function Declarations
 
+(declare-function org-attach-link-expand "org-attach" (link &optional buffer-or-name))
+
 (defvar orgtbl-exp-regexp)
 
 
@@ -1058,7 +1060,7 @@ INFO is a plist holding contextual information.  See
 		 (concat type ":" raw-path))
 		((member type '("file" "attachment"))
 		 (when (string= type "attachment")
-		   (setq raw-path (org-element-property :attachment-path link)))
+		   (setq raw-path (org-attach-link-expand link)))
 		 (org-export-file-uri raw-path))
 		(t raw-path))))
     (cond