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.

Sign up for GitHub

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

Add groups to citar-open functions #491

Closed
wants to merge 8 commits into from
Closed

Add groups to citar-open functions #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
68f6902
Add completing-read to citar--open-note, when multiple notes
localauthor Dec 16, 2021
90cc615
Add citar-select-resources, to replace citar-select-files
localauthor Dec 16, 2021
297ee59
Use consult-multi category to set categories depending on type
localauthor Dec 20, 2021
23fedc2
Use 'citar-select-resources' for 'citar--open-note'
localauthor Dec 20, 2021
df3c13b
Bug fix
localauthor Dec 20, 2021
34190bf
Fix 'open' commands for use with embark-act all
localauthor Dec 21, 2021
c9b5446
Unify open-link behavior; add prompt to 'citar--open-note'
localauthor Dec 21, 2021
9d763d4
Declare embark-default-action-overrides
bdarcus Dec 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 13 additions & 15 deletions citar-file.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ of files found in two ways:

(defun citar-file-open (file)
"Open FILE."
(funcall citar-file-open-function file))
(funcall citar-file-open-function (expand-file-name file)))

(defun citar-file-open-external (file)
"Open FILE with external application."
Expand All @@ -311,20 +311,18 @@ of files found in two ways:
nil 0 nil
file)))

(defun citar-file--get-note-filename (key dirs extensions)
"Return existing or new filename for KEY in DIRS with extension in EXTENSIONS.

This is for use in a note function where notes are one-per-file,
with citekey as filename.

Returns the filename whether or not the file exists, to support a
function that will open a new file if the note is not present."
(let ((files (citar-file--directory-files dirs (list key) extensions
citar-file-additional-files-separator)))
(or (car (gethash key files))
(when-let ((dir (car dirs))
(ext (car extensions)))
(expand-file-name (concat key "." ext) dir)))))
(defun citar-file--get-note-filenames (key dirs extensions)
"Return list of existing notes or a new filename for KEY in DIRS with extension in EXTENSIONS.

If no notes exist, returns a filename to support a function that
will open a new file if the note is not present."
(if-let* ((files
(gethash key (citar-file--directory-files dirs
(list key)
extensions
citar-file-additional-files-separator))))
files
(list (concat (car dirs) "/" key "." (car extensions)))))

(provide 'citar-file)
;;; citar-file.el ends here
141 changes: 84 additions & 57 deletions citar.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
(defvar embark-meta-map)
(defvar embark-transformer-alist)
(defvar embark-multitarget-actions)
(defvar embark-default-action-overrides)
(defvar citar-org-open-note-function)

;;; Variables
Expand Down Expand Up @@ -419,26 +420,37 @@ documentation for the return value and the meaning of
REBUILD-CACHE and FILTER."
(citar-select-ref :rebuild-cache rebuild-cache :multiple t :filter filter))

(defun citar-select-files (files)
"Select file(s) from a list of FILES."
;; TODO add links to candidates
(completing-read-multiple
"Open related file(s): "
(lambda (string predicate action)
(if (eq action 'metadata)
`(metadata
; (group-function . citar-select-group-related-sources)
(category . file))
(complete-with-action action files string predicate)))))

(defun citar-select-group-related-sources (file transform)
"Group by FILE by source, TRANSFORM."
(let ((extension (file-name-extension file)))
(when transform file
;; Transform for grouping and group title display.
(defun citar-select-resource (files &optional links)
"Select resource from a list of FILES, and optionally LINKS."
(let* ((files (mapcar
(lambda (cand)
(abbreviate-file-name cand))
files))
(resources (append files (remq nil links))))
(dolist (item resources)
(cond ((string-match "http" item 0)
(add-text-properties 0 (length item) `(consult-multi (url . ,item)) item))
(t
(add-text-properties 0 (length item) `(consult-multi (file . ,item)) item)))
(push item resources))
(completing-read
"Select resource: "
(lambda (string predicate action)
(if (eq action 'metadata)
`(metadata
(group-function . citar-select-group-related-resources)
(category . consult-multi))
(complete-with-action action (delete-dups resources) string predicate))))))

(defun citar-select-group-related-resources (resource transform)
"Group RESOURCE by type or TRANSFORM."
(let ((extension (file-name-extension resource)))
(if transform
resource
(cond
((string= extension (or "org" "md")) "Notes")
(t "Library Files")))))
((member extension citar-file-note-extensions) "Notes")
((string-match "http" resource 0) "Links")
(t "Library Files")))))

(defun citar--get-major-mode-function (key &optional default)
"Return KEY from 'major-mode-functions'."
Expand Down Expand Up @@ -885,13 +897,16 @@ into the corresponding reference key. Return

;;;###autoload
(defun citar-open (keys-entries)
"Open related resources (links or files) for KEYS-ENTRIES."
"Open any related resource (links, files, or notes) for KEYS-ENTRIES."
(interactive (list (citar-select-refs
bdarcus marked this conversation as resolved.
Show resolved Hide resolved
:rebuild-cache current-prefix-arg)))
(when (and citar-library-paths
(stringp citar-library-paths))
(message "Make sure 'citar-library-paths' is a list of paths"))
(let* ((key-entry-alist (citar--ensure-entries keys-entries))
(let* ((embark-default-action-overrides '((consult-multi . citar-open-multi)
(file . citar-file-open)
(url . browse-url)))
Copy link
Contributor

@minad minad Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here the consult-multi override should go away and the url override probably too, in order to allow the user to customize the action on the side of Embark.

Copy link
Contributor Author

@localauthor localauthor Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for looking at this, and for the comments.

Embark and consult-multi work just as you describe when it comes to categorizing files and urls, but (as I describe here) because the source of the target is minibuffer completion, Embark sets the default action for all targets, regardless of type, to the command that opened the minibuffer in the first place (per the docstring of embark-default-action-overrides). That means that, in this case, the default action for any target type is set to citar open, which is a function that does not take files or urls as arguments. Hence the overrides. It's a bit of a hail mary, I guess, but I'm afraid I'm running up against the limits of my lisp/emacs knowledge here. ;)

As for the citar-open-multi function, it was only added to allow embark-act-all to work on properly on targets of multiple types. When using embark-act on single candidates, consult-multi works just as expected and this function isn't called. When calling embark-act-all on targets of multiple types, they haven't been transformed by the candidate transformer yet --- so they remain typed as consult-multi. Hence, another override. Again, bit of a hail mary -- but functional!

Thanks again for your comments. Really appreciate your taking the time.

Copy link
Contributor

@minad minad Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, maybe I missed something here but this seems wrong. The default action is supposed to be the command that started the completion. Overriding it with something else is counterintuitive and goes against the usual Embark behavior.

Note that Embark has a minibuffer argument injection mechanism which works even if functions don't take arguments. I suggest you dig deeper into the Embark documentation.

Copy link
Contributor

@minad minad Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That means that, in this case, the default action for any target type is set to citar open, which is a function that does not take files or urls as arguments. Hence the overrides.

I am specifically referring to this. This is wrong. There is the minibuffer injection mechanism.

When calling embark-act-all on targets of multiple types, they haven't been transformed by the candidate transformer yet --- so they remain typed as consult-multi.

I will check this, but if this is the case it is a bug or issue in Embark.

I know that oantolin added some target type checking to embark-act-all and there is indeed a problem when acting on candidates of multiple categories at once. Anyways this should be handled properly by Embark itself.

EDIT: Okay, I checked Embark. Embark is doing everything alright. When you act on candidates of multiple categories at once then indeed the candidates are not transformed. Then the candidates remain consult-multi. They are only transformed if you act only on files, only on buffers or only on urls. But still, it should not be necessary to override the default action since the command itself should then be capable of handling these consult-multi candidates. You can look into consult--multi and consult-buffer as examples. There I also don't overwrite the embark default actions and everything works as expected :)

Copy link
Contributor

@bdarcus bdarcus Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I experimented a bit with removing these lines, and things seem to work fine, until I run embark-act-all on the resulting resources. So to recap:

  1. citar-open
  2. narrow candidates and then embark-act-all
  3. from there I get a list of resources, if I run embark-act-all, I then get the error below:
embark PCH: (user-error "No target found")

... which is I think why he introduced the citar-open-multi function.

image

Copy link
Contributor

@minad minad Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Did you load embark-consult? I don't know what is wrong here exactly, but the override should not be needed due to the design of embark. consult-buffer also works without override.

Copy link
Contributor

@bdarcus bdarcus Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Did you load embark-consult?

I did require it now, and get:

(execute-extended-command nil "citar-open" nil)

I don't know what is wrong here exactly, but the override should not be needed due to the design of embark. consult-buffer also works without override.

I'll take another look.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bdarcus What's the status here? Did you figure out the issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you figure out the issues?

Not yet. Likely in the next few days.

(key-entry-alist (citar--ensure-entries keys-entries))
(files
(citar-file--files-for-multiple-entries
key-entry-alist
Expand All @@ -903,17 +918,21 @@ into the corresponding reference key. Return
(lambda (key-entry)
(citar-get-link (cdr key-entry)))
key-entry-alist))
(resource-candidates (delete-dups (append files (remq nil links))))
(resources
(when resource-candidates
(completing-read-multiple "Related resources: " resource-candidates))))
(if resource-candidates
(dolist (resource resources)
(cond ((string-match "http" resource 0)
(browse-url resource))
(t (citar-file-open resource))))
(selection (citar-select-resource files links)))
(if files
(cond ((string-match "http" selection 0)
(browse-url selection))
(t (citar-file-open selection)))
(message "No associated resources"))))

(defun citar-open-multi (selection)
Copy link
Contributor

@minad minad Dec 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This command is unnecessary and should be removed. Embark already dispatches consult-multi to file and url. That's the purpose of consult-multi and the corresponding Embark candidate transformer.

"Act appropriately on SELECTION when type is 'consult-multi'.

For use with 'embark-act-all'."
(cond ((string-match "http" selection 0)
(browse-url selection))
(t (citar-file-open selection))))

(defun citar--library-files-action (keys-entries action)
"Run ACTION on files associated with KEYS-ENTRIES."
(if-let ((fn (pcase action
Expand All @@ -926,12 +945,9 @@ into the corresponding reference key. Return
citar-file-extensions)))
(if (and citar-file-open-prompt
(> (length files) 1))
(let ((selected-files
(citar-select-files files)))
(dolist (file selected-files)
(funcall fn file)))
(dolist (file files)
(funcall fn file)))
(let ((selection (citar-select-resource files)))
(funcall fn selection))
(funcall fn (car files)))
(message "No associated file")))

;;;###autoload
Expand All @@ -941,10 +957,11 @@ into the corresponding reference key. Return
With prefix, rebuild the cache before offering candidates."
(interactive (list (citar-select-refs
:rebuild-cache current-prefix-arg)))
(let ((embark-default-action-overrides '((file . citar-file-open))))
(when (and citar-library-paths
(stringp citar-library-paths))
(message "Make sure 'citar-library-paths' is a list of paths"))
(citar--library-files-action keys-entries 'open))
(citar--library-files-action keys-entries 'open)))

(make-obsolete 'citar-open-pdf
'citar-open-library-files "1.0")
Expand All @@ -955,21 +972,31 @@ With prefix, rebuild the cache before offering candidates."
With prefix, rebuild the cache before offering candidates."
(interactive (list (citar-select-refs
:rebuild-cache current-prefix-arg)))
(when (and (null citar-notes-paths)
(equal citar-format-note-function
'citar-org-format-note-default))
(error "You must set 'citar-notes-paths' to open notes with default notes function"))
(dolist (key-entry (citar--ensure-entries keys-entries))
(funcall citar-open-note-function (car key-entry) (cdr key-entry))))
(let ((embark-default-action-overrides '((file . find-file))))
(when (and (null citar-notes-paths)
(equal citar-format-note-function
'citar-org-format-note-default))
(error "You must set 'citar-notes-paths' to open notes with default notes function"))
(dolist (key-entry (citar--ensure-entries keys-entries))
(funcall citar-open-note-function (car key-entry) (cdr key-entry)))))

(defun citar--open-note (key entry)
"Open a note file from KEY and ENTRY."
(if-let* ((file (citar-file--get-note-filename key
citar-notes-paths
citar-file-note-extensions))
(file-exists (file-exists-p file)))
(find-file file)
(funcall citar-format-note-function key entry file)))
(if-let* ((raw-files (citar-file--get-note-filenames key
citar-notes-paths
citar-file-note-extensions))
(files
(mapcar
(lambda (file)
(abbreviate-file-name file))
raw-files))
(file (when (= (length files) 1)
(car files))))
(if (file-exists-p file)
(find-file (expand-file-name file))
(when (yes-or-no-p (format "No note associated with %s. Create one?" key))
(funcall citar-format-note-function key entry (expand-file-name file))))
(find-file (citar-select-resource files))))

;;;###autoload
(defun citar-open-entry (keys-entries)
Expand Down Expand Up @@ -1031,15 +1058,14 @@ directory as current buffer."
"Open URL or DOI link associated with the KEYS-ENTRIES in a browser.

With prefix, rebuild the cache before offering candidates."
;; (browse-url-default-browser "https://google.com")
(interactive (list (citar-select-refs
:rebuild-cache current-prefix-arg)))
(dolist (key-entry (citar--ensure-entries keys-entries))
(let ((link (citar-get-link (cdr key-entry))))
(if link
(browse-url-default-browser link)
(message "No link found for %s" (car key-entry))))))

(let ((embark-default-action-overrides '((url . browse-url))))
(dolist (key-entry (citar--ensure-entries keys-entries))
(let ((link (citar-get-link (cdr key-entry))))
(if link
(browse-url link)
(message "No link found for %s" (car key-entry)))))))

;;;###autoload
(defun citar-insert-citation (keys-entries &optional arg)
Expand Down Expand Up @@ -1118,10 +1144,11 @@ With prefix, rebuild the cache before offering candidates."
With prefix, rebuild the cache before offering candidates."
(interactive (list (citar-select-refs
:rebuild-cache current-prefix-arg)))
(let ((embark-default-action-overrides '((file . mml-attach-file))))
(when (and citar-library-paths
(stringp citar-library-paths))
(message "Make sure 'citar-library-paths' is a list of paths"))
(citar--library-files-action keys-entries 'attach))
(citar--library-files-action keys-entries 'attach)))

(make-obsolete 'citar-add-pdf-attachment 'citar-attach-library-files "0.9")

Expand Down