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

ekg-setup-notes-buffer' setq-local' default-directory' to ekg-db-file' parent directory. #182

Merged
merged 29 commits into from
Sep 28, 2024

Conversation

monkpearman
Copy link
Contributor

@monkpearman monkpearman commented Sep 9, 2024

ekg-setup-notes-buffer setq-local of default-directory to ekg-db-file parent directory.
The intention here is that the notes buffer should default to the parent directory of the user's ekg-db-file. I was surprised when evaluating ekg-show-notes-latest-captured (and similar functions), that the "*ekg <FOO>*" notes buffer defaulted to whichever directory the ekg-show-<FOO> function was called-interactively from. I expected these types of ekg commands would default to a consistent location. It makes more sense to me that the consistent location be the parent directory of the ekb-db-file rather than an arbitrary directory that potentially changes each time a notes buffer is presented. Note, the first attempt at commit c6f3a9b to let bind default-directory doesn't work, but the setq-local of default-directory at commit 0bd4050 does work as intended.

Added two new basic built-in ekg-tags completion functions:
ekg-tags-complete and ekg-tags-complete-doc

These seemed lacking in the API, and would be useful (esp. for beginning ekg users).

Presumably, as the number of tags increase, there may be a need to filter some tags presented via completion, hence I've used CL style keywords in anticipation of that eventual need and to cut down on the cognitive overhead of 8 &optional arguments.

See idea discussion 181.

@ahyatt
Copy link
Owner

ahyatt commented Sep 12, 2024

Thank you for the contribution!

Two questions: can you explain more why you think the directory should be consistent? It might be nice, but also using the directory where the db file is seems pretty arbitrary. I've always thought that the user wouldn't really need to do file-related operations when using ekg. How has this been useful for you?

And for the completion methods, we have in general been doing completion with completing-read directly. Can you explain how you see your new method being used? Should we be using it where we are now using completing-read within ekg? Can you let me know how you are using it now, in your own code?

@monkpearman
Copy link
Contributor Author

monkpearman commented Sep 12, 2024

Two questions: can you explain more why you think the directory should be consistent? It might be nice, but also using the directory where the db file is seems pretty arbitrary. I've always thought that the user wouldn't really need to do file-related operations when using ekg. How has this been useful for you?

(save-excursion 
  (info "(ekg)Installing by hand")
  (ekg-show-notes-in-trash)
  (dired default-directory))

(save-excursion
  (find-file 
   (concat (file-name-sans-extension (locate-library "ekg")) ".el"))
  (ekg-show-notes-in-trash)
  (dired default-directory))

As just two (of possibly many) examples, without the submitted patch to
ekg-setup-notes-buffer, the above code will open the ekg-show-notes-<FOO>
function's buffer to the directory where my ekg.info and ekg.el files are located. As
currently written, the ekg-show-notes-<FOO> related buffers take their
default-directory from whichever directory the show-notes-<FOO> function was
invoked from. Why should, for example, the "*ekg Trash*" buffer opened with
ekg-show-notes-in-trash take it's default-directory from my ekg.info or
ekg.el file? What direct relationship is there between the "*ekg Trash*" buffer
and my ekg.info file or ekg.el installation directory? As the above illustrations
should make clear, the relationship is arbitrary and unnecessarily variant.

The utility of doing (setq-local default-directory ... in
ekg-setup-notes-buffer is that it provides the user a guarantee that all
ekg-show-notes-<FOO> buffers will open in a common directory. To the extent
that the user doesn't need to do file related operations using ekg notes buffers, then it
shouldn't matter where the ekg-show-notes-<FOO> take their default-directory
value from, and therefor the best choice is to default to the directory where
the actual ekg notes are stored. Technically speaking, that directory is where the
ekg-db-file is located.

And for the completion methods, we have in general been doing completion with completing-read directly. Can you explain how you see your new method being used? Should we be using it where we are now using completing-read within ekg? Can you let me know how you are using it now, in your own code?

Regarding ekg-tags-complete my immediate use case when i submitted the patch
was that i had just started using ekg and after capturing 5-10 ekg notes I could
no longer remember which tags I had assigned. Poking around at the ekg.el code
it seemed to me that the most straightforward way to recall which tags I had
assigned would be to simply do a completing-read on return value of ekg-tags.
This is my current use case for both ekg-tags-complete and ekg-tags-complete-doc.
Although, to be fair, I included ekg-tags-complete-doc in the pull request mostly
because it illustrates that ekg-tags-complete (with CL style keywords) is more easily
extensible than the vanilla completing-read. Given that ekg allows for an uncontrolled
set of tags, and given that ekg provides an informal prefixing schema for it's tags, it made
sense to show an example of how ekg-tags-complete could be readily leveraged to
filter prefixed tags, eg "doc/<FOO>" style tags, which is what ekg-tags-complete-doc
should make clear. As such, my hope is that ekg-tags-complete-doc might serve
as an example to any future ekg user examining the ekg.el source as to how they may
extend ekg-tags-complete for their own purposes.

Since submitting my patch, I have been using ekg daily and have become more
familiar with it's interface. As you point out, ekg.el has been doing completion
directly with completing-read (mostly wrapped within interactive forms).
It has since occurred to me that ekg might one day benefit from alternative
completion interfaces.

If I were defining the ekg.el API, i would provide two customizable variables:
ekg-tags-complete-function and ekg-tags-complete-multiple-function
and default these to completing-read and completing-read-multiple. The
docstring for these two custom variables would specify that the user can
provide an alternative completion interface for ekg tags completion, so long as their
argument list's adhered to that of the ekg-tags-complete interface as per
completing-read. I would also add a new function ekg-tags-complete-multiple
for congruence with ekg-tags-complete. These two functions, and their associated
customizable variables, would allow users to specify alternative ekg completion
interfaces in future.

IF there were an ekg-tags-complete-function variable this would require some
tweaks to ekg-tags-complete to conditionalize aroundekg-tags-complete-function.
Something like the following will suffice (and has been added to the pull request):

(apply ekg-tags-complete-function
        {... completion args ...})

With the above (or equivalent) additions to ekg-tags-complete in place,
and with the addition of the variable ekg-tags-complete-multiple-function, and
the function ekg-tags-complete-multiple in place, I would then modify the existing
ekg.el functions which invoke completing-read and completing-read-multiple
directly to use ekg-tags-complete and ekg-tags-complete-multiple instead.
I believe this would provide a cleaner and more extensible ekg, both now and in future,
and with the added benefit that users would have a ready made ekg-tags-complete
function as part of the ekg API. Indeed, as my patch illustrates, if ekg doesn't provide
a tag's completion interface for users, then users will create their own :)

As such, I have recently updated the current pull request with the new custom variables
ekg-tags-complete-function ekg-tags-complete-multiple-function, and have added
the new function ekg-tags-complete-multiple. Likewise, I have also updated the
existing ekg code that evaluates completing-read to use ekg-tags-complete
and code that evaluates completing-read-multiple to use ekg-tags-complete-multiple.

… Add new

custom variable `ekg-tags-complete-multiple-function'
…e predicate

is not needed with CL style keywords
… strings

in certain arguments to `ekg-tags-complete' functions
@ahyatt
Copy link
Owner

ahyatt commented Sep 14, 2024

Thanks for the extensive explanations. Let's take each part of this at a time. You've convinced me about the default-directory. Can we just make this pull request that default-directory change?

For the other one, normally if a user wanted to change how completion worked by substituting a different function, they could customize completing-read-function. I'm unsure we really need anything added to ekg for this - I think it's pretty customizable on its own right now, but you could put your function that are useful to you in the wiki perhaps (which is a bit unused at the moment). I'm a little reluctant to add functions that aren't called, and whose utility I don't know about. If they prove useful to many, it may be worth adding and documenting.

@monkpearman
Copy link
Contributor Author

Please incorporate only the code you feel is appropriate to the needs of the project. 🙂

@ahyatt
Copy link
Owner

ahyatt commented Sep 15, 2024

It's a bit odd to just merge in parts of a pull request via Github, so to fix the default-directory issue, I'd probably just re-implement it. I'd prefer to merge in a change of yours to do that, because this is your contribution, and it's always nice to have that contribution recorded with you as the author. Let me know, either way would be fine. Thanks again for the contribution!

@monkpearman
Copy link
Contributor Author

monkpearman commented Sep 17, 2024

If it's all the same to you, I'd prefer if you merged my change for the contributor credit, but this pull request is a mess so definitely do what is easiest for you :)

Commit 0bd4050 has the relevant changes.

@ahyatt ahyatt merged commit 839c6ce into ahyatt:develop Sep 28, 2024
2 checks passed
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.

2 participants