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

[Gustav Wikström]

Hi!

> > +   (if should-get
> > +       (progn (message "Running git annex get \"%s\"." path-relative)
> > +              (call-process "git" nil nil nil "annex" "get" path-relative))
> > +     (error "File %s stored in git annex but it is not available, and was 
> > not
> retrieved"
> > +            path))))))
> 
> Nitpick:
> 
>     (unless should-get
>      (error "File %S stored in git annex but unavailable" path))
>     (message "Running git annex get %S." path-relative)
>     (call-process ...)

Ok, fixed.

> > +Selective means to respect the inheritance setting in
> > +`org-use-property-inheritance'."
> >    :group 'org-attach
> > +  :type '(choice
> > +     (const :tag "Don't use inheritance" nil)
> > +     (const :tag "Inherit parent node attachments" t)
> > +     (const :tag "Respect org-use-property-inheritance" selective)
> > +     )
> 
> Dangling paren spotted.

Fixed.

> > +      (setq attachment (or (org-attach-dir)
> > +                      (quote  "Can't find an existing attachment-folder")))
> 
> You forgot to remove that weird quote. Maybe you meant `error'?

Hmm, actually no. But the code is pretty bad so I've refactored it a
bit. The purpose of the change is for org-attach to give an indication
of the active attachment path, or to signal that there is none. But
for that I don't really need a separate variable. Thus it's slightly
refactored for code-clarity.

> > +    (if attach-dir
> > +   (progn (if (not (file-directory-p attach-dir))
> > +              (make-directory attach-dir t))
> > +          attach-dir)
> > +      (error "No attachment directory is associated with the current 
> > node"))))
> 
> Same nitpick as above:
> 
>     (unless attach-dir
>      (error "No attachment ..."))
>     (if (file-directory-p attach-dir) attach-dir
>       (make-directory attach-dir))

Ok, fixed.

> > +(defun org-attach-dir-from-id (id)
> > +  "Creates a path based on `org-attach-id-dir' and ID."
> > +  (expand-file-name
> > +   (funcall org-attach-id-to-path-function id)
> > +   (expand-file-name org-attach-id-dir)))
> 
> Creates path -> Return a file name.

Fixed.

> > +of the entry.  Creates relative links if `org-attach-dir-relative'
> > +is t.
> 
> Nitpick:
> 
>   is t -> is non-nil.

Ah, true. Fixed.

> If tests pass, feel free to apply the patches in master. Thank you!

Got it. Aaand one test failure. That test is unrelated to my changes
though, and fails also on master. Test-org-table/copy-down. So I'll
try to apply my patch asap regardless of that one test failing.

Just one more thing - a few days back I added a row to lisp/ox-html.el
regarding inline-images. I'm including that change as well since it
relates 100% to the new attachment link. I looked in the other
export-backends too but didn't add anything due to lack of time for
testing. Maybe the additions for other backends is as trivial as for
html. So someone who regularly export to those backends might want to
suggest patches for them to make attachment links to images actually
display as images? 😊

Final patches attached for full disclosure before applying them.

> 
> Regards,
> 
> --
> Nicolas Goaziou

0001-org-test-test-org-element-test-org-test-ox-test-prop.patch
Description: 0001-org-test-test-org-element-test-org-test-ox-test-prop.patch

0002-org-attach-org-org-manual-org-news-ox-html-testing.patch
Description: 0002-org-attach-org-org-manual-org-news-ox-html-testing.patch