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

[Gustav Wikström]

Hi,

Thanks!

> -----Original Message-----
> From: Nicolas Goaziou <address@hidden>
> Sent: den 15 juni 2019 15:29
> To: Gustav Wikström <address@hidden>
> Cc: emacs-orgmode <address@hidden>; Bastien Guerry <address@hidden>
> Subject: Re: [RFC] Link-type for attachments, more attach options


> > (Sidenote 2; The first patch is some minor unrelated fixups - it
> > breaks test-org-export/expand-include as a side-effect. But that's probably
> > because the test doesn't do what it should! 😲 )
> 
> Probably. Yet, we should do something about it. If you cannot fix it, at
> least please comment it out somehow.

Looked at it a few more minutes. The error comes from the fix of
org-test-with-temp-text-in-file, where it previously didn't work with
point. It does now, which means the test-specification was wrong. The
test should compare to 2 instead of 1 since the inserted node will
be one level above the current headline.

> >    (should
> >     (equal
> >      "1"
> > -    (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** 
> > <point>H2"
> > -      (org-entry-get (point) "A" t))))
> > +    (org-test-with-temp-text "* H\n:PROPERTIES:\n:A: 1\n:END:\n** H2"
> > +      (org-entry-get (point-max) "A" t))))
> 
> Not that it is wrong, but I don't see the benefit of this change.

No real benefit except that it now matches other tests that was also
defined. I spent a few minutes wondering why some was specified in one
way and the others another way. Realized there was no reason and
thought to save those minutes for the next person who might wonder the
same thing.

> >  (defmacro org-test-with-temp-text-in-file (text &rest body)
> > -  "Run body in a temporary file buffer with Org mode as the active mode."
> > +  "Run body in a temporary file buffer with Org mode as the active mode.
> > +If the string \"<point>\" appears in TEXT then remove it and
> > +place the point there before running BODY, otherwise place the
> > +point at the beginning of the buffer."
> >    (declare (indent 1))
> >    (let ((results (cl-gensym)))
> >      `(let ((file (make-temp-file "org-test"))
> > @@ -207,6 +210,8 @@ otherwise place the point at the beginning of the 
> > inserted text."
> >        ,results)
> >         (with-temp-file file (insert inside-text))
> >         (find-file file)
> > +       (when (re-search-forward "<point>" nil t)
> > +    (replace-match ""))
> >         (org-mode)
> >         (setq ,results (progn ,@body))
> >         (save-buffer) (kill-buffer (current-buffer))
> 
> While we're at fixing this macro, we may also wrap stuff within
> `unwind-protect' so as to properly delete temporary files in any cases.

Ok, tried to do that. Maybe added a few to many guardrails but it
should work as intended.

> > From 0180a900f890b2053d8184116c99c62cfa083055 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Gustav=20Wikstr=C3=B6m?= <address@hidden>
> > Date: Sun, 25 Nov 2018 21:38:44 +0100
> > Subject: [PATCH 2/2] org-attach*, org, org-manual, org-news,
> >  testing/**/test-org-attach*
> >
> > * org-attach.el
> >
> > Changed the way attachments deal with property-inheritance.  It now
> > adheres to the =org-use-property-inheritance= setting by default but
> > it can be customized if needed (I recommend to enable it!).
> > The property ATTACH_DIR is depricated in favour of the shorter and simpler
> > property DIR.
> 
> depricated -> deprecated

Whops! Fixed

> > * org-manual.org
> >
> > Attachments have been promoted into its own top-level node!  There are
> > a lot of goodies to mention here that didn't fit naturally with
> > "capture" and "archive" before.
> 
> This is true, but there is not enough information to require a top-level
> node.
> 
> Here is a suggestion. Split "Capture, Refile, Archive" into two parts:
> 
> 1. Refile, Copy and Archiving in one place, because they all relate to
>    moving existing data around
> 
> 2. Capture, Attachments, RSS Feeds, Protocols in another place, because
>    they all insert external data to the system.
> 
> WDYT?

Fair enough. Modified the manual to match that suggestion.

> > +File links and attachment links can contain additional information to
> 
> Since attachment links are file links, it seems redundant to talk about
> both everywhere. Just clarify clearly once that attachments links are
> file links for all purposes.

Modified it a bit to make it read better. I do think it's worth having small 
notes that search options apply also for attachment links. But I agree that 
we don't need to mention it everywhere.

> > +It is often useful to associate reference material with an outline
> > +node/task.
> 
> I would use "node" only: it is more generic.

Agreed, fixed.
 
> > +The attachment system has been contributed to Org by John Wiegley.
> 
> I would drop this reference, since John Wiegley is already mentioned in
> the Thanks entry.

Ok, fixed.
 
> > +** Attachment defaults and dispatcher
> > +By default, org-attach will use ID properties when adding attachments
> > +to outline nodes.  This makes working with attachments fully
> > +automated.  There is no decision needed for folder-name or location.
> > +ID-based directories are by default located in the =data/= directory,
> > +which lives in the same directory where your Org file lives[fn:87].
> > +For more control over the setup, see [[*Attachment options]].
> > +
> > +When attachments are made using ~org-attach~ a default tag =ATTACH= is
> > +added to the node that gets the attachments.
> 
> Note overly important, but we prefer the active voice whenever possible.

Can't say I really understand what was meant with this.

> > +  #+begin_example
> > +  ,* Chapter A ...
> > +    :PROPERTIES:
> > +    :DIR: Chapter A/
> > +    :END:
> > +  ,** Introduction
> > +  Some text
> > +
> > +  #+NAME: Image 1
> > +  [[Attachment:image 1.jpg]]
> > +  #+end_example
> > +
> > +  Without inheritance one would not be able to resolve the link to image
> > +  1.jpg, since the link is inside a sub-heading to Chapter A.
> 
> =1.jpg=  ... =Chapter A=

Fixed.
 
> > +  Inheritance works the same way for both =ID= and =DIR= property.  If
> > +  both properties are defined on the same headline then =DIR= takes
> > +  precedance.  This is also true if inheritance is enabled.  If =DIR= is
> > +  inherited from a parent node in the outline, that property still takes
> > +  precedence over an =ID= property defined on the node itself.
> > +
> > +- ~org-attach-method~ ::
> > +  #+vindex: org-attach-method
> > +  When attaching files using the dispatcher {{{kbd(C-c C-a)}}} it
> > +  defaults to copying files.  The behaviour can be changed by
> > +  customizing ~org-attach-method~.  Options are =Copy=, =Move/Rename=,
> > +  =Hard link= or =Symbolic link=.
> 
> I don't think =...= are warranted here.

Ok, fixed.

> 
> > +- ~org-attach-preferred-new-method~ ::
> > +  #+vindex: org-attach-preferred-new-method
> > +  This customization lets you choose the default way to attach to
> > +  nodes without existing ID and DIR property.  It defaults to =id= but
> > +  can also be set to =dir=, =ask= or =nil=.
> 
> ~id~, ~dir~ ~ask~ or ~nil~, since those are symbols.

Fixed.

> > +  Do not show the splash buffer with the attach dispatcher when
> > +  ~org-attach-expert~ is set to Non-nil.
> 
> non-~nil~

Fixed.

> > +** Attachment links
> > +Attached files and folders can be referenced using attachment-links.
> 
> Why the hyphen in "attachment-links"? I think "attachment links" is
> perfectly clear.

Fixed.
 
> > +This makes it easy to refer to the material added to an outline node.
> > +Especially if it was attached using the unique ID of the entry!
> > +
> > +#+begin_example
> > +,* TODO Some task
> > +  :PROPERTIES:
> > +  :ID:       95d50008-c12e-479f-a4f2-cc0238205319
> > +  :END:
> > +See attached document for more information: [[attachment:info.org]]
> > +#+end_example
> > +
> > +See [[* External Links]] for more information about these links.
> 
> No need for the space: [[*External Links]]
> 
> > +** Automatic version-control with git
> 
> Shouldn't it be "Git"?

Maybe...? Changed now anyways.
 
> > +If the directory attached to an outline node is a git repository, Org
> 
> Ditto.

Changed.

> > +can be configured to automatically commit changes to that repository
> > +when it sees them.
> > +
> > +To make org-mode take care of versioning of attachments for you, add
> 
> To make Org mode...

Fixed.
 
> > +(defcustom org-attach-dir-relative nil
> > +  "Choose whether directories in DIR property should be added as relative 
> > links or not.
> > +Defaults to absolute location."
> 
> First line is too long, maybe:
> 
>     Non-nil means directories in DIR property are added as relative links
> 

Fixed.

> > -(defcustom org-attach-annex-auto-get 'ask
> > -  "Confirmation preference for automatically getting annex files.
> > -If \\='ask, prompt using `y-or-n-p'.  If t, always get.  If nil, never 
> > get."
> > +(defun org-attach-id-folder-format (id)
> > +  (format "%s/%s"
> > +     (substring id 0 2)
> > +     (substring id 2))
> > +  )
> 
> No dangling parens, please. Also, this function needs a docstring.

Ah, ofc.

> > +(defcustom org-attach-id-to-path-function #'org-attach-id-folder-format
> > +  "The function parsing the ID parameter into a folder-path"
> 
> Missing full stop. Also: "Function parsing..."
> 
> >    :group 'org-attach
> > -  :package-version '(Org . "9.0")
> > -  :version "26.1"
> > -  :type '(choice
> > -     (const :tag "confirm with `y-or-n-p'" ask)
> > -     (const :tag "always get from annex if necessary" t)
> > -     (const :tag "never get from annex" nil)))
> > +  :package-version '(Org . "9.3")
> > +  :type 'function
> > +  )
> 
> See above.

Fixed.
 
> > +  "Return the directory associated with the current outline node.
> > +First check for DIR property, then ID property.
> > +`org-attach-use-inheritance' determines whether inherited
> > +properties also will be considered.
> > +
> > +If an ID property is found the default mechanism using that ID
> > +will be invoked to access the directory for the current entry.
> > +
> > +If CREATE-IF-NOT-EXIST-P is non-nil, `org-attach-dir-get-create'
> > +is run."
> > +  (if create-if-not-exists-p
> > +      (org-attach-dir-get-create)
> 
> Could you merge the `if' within the `cond' below?

Done.
 
> > +    (let (attach-dir id)
> > +      (cond
> > +       ((setq attach-dir (org-entry-get nil "DIR" 
> > org-attach-use-inheritance))
> > +   (org-attach-check-absolute-path attach-dir))
> > +       ;; Depricated and removed from documentation, but still
> 
> Deprecated. Also, please add a FIXME.

Fixed and added.
 
> > +    (if attach-dir
> > +   (progn (if (not (file-directory-p attach-dir))
> > +              (make-directory attach-dir t))
> > +          (if (file-exists-p attach-dir)
> > +              attach-dir
> > +            (error "Path does not exist: %s." attach-dir)))
> > +      (error "No attachment directory is associated with the current 
> > node."))))
> 
>     (unless attach-dir (error "No attachment ... current node"))
>     (unless (file-directory-p attach-dir) (make-directory ...))
>     attach-dir
> 
> Note that error messages never end with a full stop in Emacs. Also,
> I don't see how the inner error could trigger since you create the
> directory just before, and it would already have returned an error if
> that wasn't possible.

Ok, got it. Fixed. Removed inner error as well as I agree with your
conclusion. Tried to think of why it was added but couldn't remember
any reason.

> > -      (shell-command (format "rm -fr %s" attach-dir))
> > +    (when (and attach-dir
> > +          (or force
> > +              (y-or-n-p "Are you sure you want to remove all
> > attachments of this entry? ")))
> 
> Maybe the less mouthful:
> 
>    "Really remove all attachments of this entry? "
> 

Good suggestion, fixed.

> Also, what about using `yes-or-no-p' since this looks a sensitive operation?

Agreed! Didn't think of that.
 
> > +   (org-attach-tag (not files))))
> > +    (when (not attach-dir)
> > +      (org-attach-tag t))))
> 
>     (unless attach-dir (org-attach-tag t))

Fixed.
 
> > +  (interactive)
> > +  (let ((attach-dir (org-attach-dir)))
> > +    (if attach-dir
> > +   (org-open-file attach-dir)
> > +      (error "No attachment directory exist."))))
> 
> See above: no full stop at the end of error messages.

Fixed.

> > +  (let ((attach-dir (org-attach-dir)))
> > +    (if attach-dir
> > +   (dired attach-dir)
> > +      (error "No attachment directory exist."))))
> 
> Ditto.

Fixed
 
> > +  (let ((attach-dir (org-attach-dir)))
> > +    (if attach-dir
> > +   (let* ((files (org-attach-file-list attach-dir))
> > +          (file (if (= (length files) 1)
> > +                    (car files)
> > +                  (completing-read "Open attachment: "
> > +                                   (mapcar #'list files) nil t)))
> 
> I think a `pcase' is warranted here:
> 
>     (pcase (org-attach-file-list attach-dir)
>       (`(,file) file)
>       (files (completing-read ...)))
> 

And so I learnt something new. Pattern matching in emacs, didn't think
of that. Changed. Can't really say that the code will be easier to
understand now. But for those familiar with pcase I'm sure it looks
nicer.

> > +          (path (expand-file-name file attach-dir)))
> > +     (run-hook-with-args 'org-attach-open-hook path)
> > +     (org-open-file path in-emacs))
> > +      (error "No attachment directory exist."))))
> 
> Ahem. :)

Yeah. Fixed the full stop.

> 
> > diff --git a/testing/examples/att1/fileA b/testing/examples/att1/fileA
> 
> Would it be possible to use `org-test-with-temp-text-in file instead of
> creating files?

I couldn't think of a good way since I wanted to test relations
between an org-mode document and it's attachments. I would need to
dynamically create both org-mode files and it's attachment and link
them in the filesystem somehow... That wasn't intuitive for me so I
chose this solution instead.
 
> > +(ert-deftest test-org-attach/dir ()
> > +  "Test `org-attach-get' specifications."
> > +  (org-test-in-example-file org-test-attachments-file
> > +    ;; Link inside H1
> > +    (org-next-link)
> > +    (save-excursion
> > +      (org-open-at-point)
> > +      (should (equal "Text in fileA\n" (buffer-string))))
> 
> Please move `should' in the outermost part of the sexp, if possible. It
> makes debugging easier (i.e., evaluating the sexp without getting
> `should' in the way).

Ok, did that. I agree - it reads better. To get this to work better I
made a slight change in org-test-in-example-file as well. Added to
patch 0001 since that change isn't really related to attachments but
rather a general improvement.

One might also argue that this particular test should be split up. I
Agree with that, but would like that to not be a stopper for this first 
patch.
 
> I probably missed a few things, but it keeps the ball rolling.

I've added a few fixes to the patch myself. I haven't kept track of
the changes though, since I'm trying to keep the patch together in one
commit (well... two in this case).
 
> Thank you again for all the hard work.

And thank you for the review. Bastien gave me access to the Git
repository. Let me know how you want to proceed with this patch.

Kind regards
Gustav Wikström

0002-org-attach-org-org-manual-org-news-testing-test-org-.patch
Description: 0002-org-attach-org-org-manual-org-news-testing-test-org-.patch

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