Improve note creation functionality (and tweak API)

[roshanshariff]

This PR continues #658.

citar-open and citar-open-notes can now create notes

Both these functions now offer to create notes for the chosen keys. The customization variable citar-open-always-create-note controls whether the "create" option is offered only for keys that don't already have notes. Regardless of this variable's value, the citar-create-note command can always be used to create multiple notes for any key, if supported by the citar-note-source.

This new feature is implemented as an extra option to citar--select-resource, which has now learned about the create-note pseudo-resource type. The citar-open-notes and citar--open-resource functions also learned to create new notes.

Other Changes

There is a new command called citar-open-note, which prompts to select a note from all the available notes and opens it. It can also be called non-interactively with a note returned by citar-get-notes, which is its primary purpose.

The customization variable citar-open-prompt can now be set to a list of commands that will always prompt before selecting resources. The default is to always prompt for citar-open, citar-attach-files, and citar-open-note. This should fix the UX issue identified in #656.

The new customization variable citar-open-resources controls which resources are offered by citar-open. By default it is set to '(:files :links :notes :create-notes), but any of those can be removed if desired.

API changes

The first commit in this PR modifies the API to make the above changes easier.

The citar-get-files, citar-get-links, and citar-get-notes functions now return hash tables mapping each key to a list of associated resources. They used to just return lists of resources for all the given keys, but that interface doesn't allow callers to determine which key corresponds to each resource.

The :items function in the notes source API has also been modified, and must now return a hash table instead of a list of notes.

emacs-citar/citar-org-roam#6 updates the citar-org-roam note source to conform to this change.

Remaining work

We don't yet have a "multi-select" interface to citar-open and friends. embark-act-all can be used to act on all the presented candidates, but this doesn't allow you to select some smaller subset.

[bdarcus]

If appropriate, can you include a screenshot when you get a chance?

[localauthor]

Here's what citar-open looks like with citar-open-always-create-note set to t:

[bdarcus]

Are you also a geographer Grant???

[localauthor]

Are you also a geographer Grant???

Not exactly. My dissertation was on literary and geographical discourse in 19th-C America, but my degree is in English. So I'm sort geography adjacent. :)

[bdarcus]

I don't want to open a new issue for this, but @roshanshariff, can you please explain why you use cl-callf2 here, which seems pretty esoteric (compared to add-to-list or cl-pushnew)?

citar/citar.el

Line 849 in c5ed78e

(cl-callf2 assq-delete-all name citar-notes-sources))

... and also setf just above?

I'm sure there's a reason, but I'd like to understand it.

Perhaps even code comments there might be helpful?

[roshanshariff]

The line

(setf (alist-get name citar-notes-sources) config)

means "in the citar-notes-sources alist, set the value associated with name to config". You use (alist-get name citar-notes-sources) to return the value associated with that key in the alist. The setf macro lets you set that value rather than just reading it (see "setting generalized variables"). This is a uniform way to set the values of "places" in Elisp, and in this case it takes care of either creating a new alist entry if it doesn't exist, or updating an existing entry.

It's also possible to extend this machinery, so if you're writing a library that has a (get-something "foo") function, you can let users set the value using (setf (get-something "foo") "new-value") instead of having another (set-something "foo" "new-value") function. See "adding generalized variables".

(cl-callf2 func x y) is short for (setf y (func x y)), except you don't have to write y twice. Here y is just a normal variable, so it's short for (setq citar-notes-sources (assq-delete-all name citar-notes-sources)) but less repetitive.

Finally, why is it not enough to just call (assq-delete-all name citar-notes-sources), since that is supposed to modify the alist in-place? You can experiment with this using delq on a list:

ELISP> (setq mylist '(foo bar baz))
(foo bar baz)

ELISP> (delq 'bar mylist)
(foo baz)

ELISP> mylist
(foo baz)

ELISP> (delq 'foo mylist)
(baz)

ELISP> mylist
(foo baz)

You can see that although delq returned a list without 'foo in the last example, it didn't update the value of mylist.

delq removes an element from a list by setting the cdr of the previous element to the rest of the list following the element to be removed (i.e. to the cdr of that element). This works as long as you aren't removing the first element in the list, which has no previous element. That's why delq also returns the modified list; this will only be different from the list you gave it when it deleted the head of the list, in which case it returns the cdr of the list. The correct way to use it is (setq mylist (delq 'foo mylist). The same reasoning applies to assq-delete-all.

I'm not sure if code comments are the right approach here. I find that they're not as helpful as the built-in documentation to understand a construct you might not have seen before. In this case, the documentation would be here. I myself learned about them by stumbling across them in other Elisp code and looking up what they did.

[bdarcus]

Many thanks.

I recall part of the discussion on the Doom discord was about setf compatibility and "slots" for this defcustom etc.

[roshanshariff]

You're welcome! I realized I forgot to address the first part, why not use add-to-list or cl-pushnew to add things to alists. Well:

ELISP> (setq myalist '((foo . "bar") (baz . "qux")))
((foo . "bar")
 (baz . "qux"))

ELISP> (cl-pushnew '(foo . "bar") myalist)
((foo . "bar")
 (foo . "bar")
 (baz . "qux"))

ELISP> (cl-pushnew '(foo . "???") myalist)
((foo . "???")
 (foo . "bar")
 (foo . "bar")
 (baz . "qux"))

So cl-pushnew will happily add duplicate keys to the alist, and in the first example it will even allow "duplicate" (key . value) pairs. That's because by default it uses eq to compare list elements. The elements of an alist are cons pairs, which are are only eq if they're actually the same object in memory, i.e. resulting from the same call to cons. The appropriate comparison function for alists is to use eq on the car of the list elements, which is what assq and alist-get do. This is also why you don't use delq to remove things from an alist, but rather assq-delete-all.

add-to-list is the same, except it's supposed to be used only in user configurations and such, because it'll create the variable if it's not already bound rather than signalling an error. (That's why it takes the variable name as a quoted symbol, so it's not an error if unbound.)

As an aside, cl-pushnew and push are macros and also work on generalized variables. So, for example, if you have a hash table mapping keys to lists, and you want to push a new element onto one of those lists, all of these will work and are (almost) equivalent:

(push 42 (gethash 'foo ht))

(cl-callf2 cons 42 (gethash 'foo ht))

(setf (gethash 'foo ht) (cons 42 (gethash 'foo ht)))

(puthash 'foo (cons 42 (gethash 'foo ht)) ht)

This works because (gethash 'foo ht) is a generalized variable with puthash being its setter. If 'foo isn't already a key in the hash table ht, gethash will return nil and (cons 42 nil) will be a new single-element list.

[bdarcus]

OK, so they're more fine-grained tools, and cl-pushnew and add-to-list are kind of dumb.

[bdarcus]

Two things on the PR, @roshanshariff:

1. Feel free to merge it when you think it's ready. I'm not sure if I'll have time to test it, it seems sensible on cursory look, and @localauthor seems happy with it.
2. In the future, feel free also to branch off the main repo. It's slightly easier for me to test etc in that circumstance.

[roshanshariff]

Sounds good, I'll merge it now. I was hoping to get to the multi-select functionality, but I'll have to leave that for later when I have time.

[bdarcus]

Great!

What else, if anything, do you think needs to be done before 1.0?