Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counsel.el (counsel-org-file): Handle ATTACH_DIR property #2042

Closed
wants to merge 1 commit into from

Conversation

ericdanan
Copy link
Contributor

Also list attachments from directories specified with the ATTACH_DIR rather than the ID property.

counsel.el Outdated
(when (string= "ID" (match-string-no-properties 1))
(setq dir (expand-file-name
(concat (substring dir 0 2) "/" (substring dir 2))
org-attach-directory)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the org-attach-dir API instead of duplicating a subset of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's the best way to go. On the one hand, using org-attach-dir would simplify the code. On the other hand, it would unnecessarily parse the whole property block of the current entry when we have already matched the relevant property line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's the best way to go.

IME, Org is a complicated and fickle beast, so whenever there is a stable API, we should try to reuse it.

On the one hand, using org-attach-dir would simplify the code.

And, more importantly, make it more correct and robust.

On the other hand, it would unnecessarily parse the whole property block of the current entry when we have already matched the relevant property line.

Does this really cause a noticeable slowdown? I doubt it, but I could be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a very large org file to test. Since counsel-org-files was not using org-attach-dir before, I would rather know what @abo-abo thinks before changing that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the current implementation. Org will not break this API unless they want to break the users' existing attachment directories.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abo-abo

Org will not break this API unless they want to break the users' existing attachment directories.

I never said Org might break the ATTACH_DIR API. What I said is:

Why not use the org-attach-dir API instead of duplicating a subset of it?

Emphasis on "subset". The function org-attach-dir handles not only ATTACH_DIR, but also ATTACH_DIR_INHERIT and related Org settings. If it is possible to reuse the longstanding org-attach-dir API instead of emulating a subset of it, then we absolutely should.

counsel.el Outdated
(concat (substring id 0 2) "/" (substring id 2))
org-attach-directory))
ids))))
(let ((files (let (res dir)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Is the nested let really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

counsel.el Outdated
(let* ((ids (let (res)
(save-excursion
(let (files res dir)
(setq files (save-excursion
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's redundant to have both files and res since they're the same thing:

diff --git a/counsel.el b/counsel.el
index 6d2be7c..3c39700 100644
--- a/counsel.el
+++ b/counsel.el
@@ -3294,24 +3294,23 @@ counsel-org-files
 contained files are listed, so the return value could conceivably
 include attachments of other Org buffers."
   (require 'org-attach)
-  (let (files res dir)
-    (setq files (save-excursion
-                  (goto-char (point-min))
-                  (while (re-search-forward "^:\\(ATTACH_DIR\\|ID\\):[\t ]+\\(.*\\)$" nil t)
-                    (setq dir (match-string-no-properties 2))
-                    (when (string= "ID" (match-string-no-properties 1))
-                      (setq dir (expand-file-name
-                                 (concat (substring dir 0 2) "/" (substring dir 2))
-                                 org-attach-directory)))
-                    (when (file-exists-p dir)
-                      (push dir res)))
-                  (nreverse res)))
+  (let (dirs)
+    (save-excursion
+      (goto-char (point-min))
+      (while (re-search-forward "^:\\(ATTACH_DIR\\|ID\\):[\t ]+\\(.*\\)$" nil t)
+        (let ((dir (match-string-no-properties 2)))
+          (when (string= "ID" (match-string-no-properties 1))
+            (setq dir (expand-file-name
+                       (concat (substring dir 0 2) "/" (substring dir 2))
+                       org-attach-directory)))
+          (when (file-exists-p dir)
+            (push dir dirs)))))
     (cl-mapcan
      (lambda (dir)
        (mapcar (lambda (file)
                  (file-relative-name (expand-file-name file dir)))
                (org-attach-file-list dir)))
-     files)))
+     (nreverse dirs))))
 
 ;;;###autoload
 (defun counsel-org-file ()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied your patch, thanks.

Also list attachments from directories specified with the ATTACH_DIR rather
than the ID property.
@abo-abo
Copy link
Owner

abo-abo commented May 6, 2019

Thanks.

abo-abo added a commit that referenced this pull request May 6, 2019
@abo-abo
Copy link
Owner

abo-abo commented May 6, 2019

Simplified to use org-attach-dir. I think the performance penalty is negligible here. Please test.

@basil-conto
Copy link
Collaborator

Simplified to use org-attach-dir.

Thanks. I bet there's an API for avoiding the surrounding regexp search as well, but I'm not that familiar with Org.

@ericdanan ericdanan deleted the orgfile branch May 6, 2019 19:38
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
Also list attachments from directories specified with the ATTACH_DIR rather
than the ID property.

Fixes abo-abo#2042
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants