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

refactor: add optional parameter to refresh command, update docs #76

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

bdarcus
Copy link
Contributor

@bdarcus bdarcus commented Apr 12, 2021

Update bibtex-actions-refresh to allow it be invoked as a callback on file-notify-add-watch without requiring a lambda.

Also update the README to incorporate the change.

Relates to #73.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 12, 2021

@wenjie2wang - couple of things on this, if you have time and interest:

  1. I have updated the "refresh" command to remove the lambda, and so also changed the README instructions.
  2. I have added the two "watch" commands (but I'm not sure they work correctly ATM). WDYT of this idea? I'm still not sure.

"Add watches using 'file-notify-add-watch'.
For the paths specified in 'bibtex-actions-watch-list' will add a
watch that runs 'bibtex-actions-refresh' upon a change."
;; TODO return message of success or error
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 know how to do this!

@wenjie2wang
Copy link
Contributor

Thanks, @bdarcus !

I have updated the "refresh" command to remove the lambda, and so also changed the README instructions.

It is great to have a simplified interface.

I have added the two "watch" commands (but I'm not sure they work correctly ATM). WDYT of this idea? I'm still not sure.

I can help test them after work (I wish I knew more about Elisp and help you more on it). IMHO, the current file-notify-add-watch approach works well. As a user, I do not mind adding those configurations to my .emacs.d as long as it is documented in the README.

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 12, 2021

As a user, I do not mind adding those configurations to my .emacs.d as long as it is documented in the README.

Probably if in doubt, I shouldn't include the commands at this point then.

I can help test them after work.

OK, thanks.

It could be that I remove the commands, etc., and simply update the refresh command and README.

Let me know WYT after you have a chance to test.

@bdarcus bdarcus changed the title add watch commands, defcustom refactor,docs: add optional parameter to refresh command, update docs Apr 12, 2021
@bdarcus bdarcus changed the title refactor,docs: add optional parameter to refresh command, update docs refactor: add optional parameter to refresh command, update docs Apr 12, 2021
@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 12, 2021

Another option, if we get it working, is to include an example in the README like what I'm trying to do here:

bdarcus/.doom.d@a483642

I added this to the wiki, so people can easily improve it.

https://github.com/bdarcus/bibtex-actions/wiki/Configuration#automating-path-watches

@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 13, 2021

I guess that the additional call is triggered by the file-notify-add-watch set in the bibtex-completion.el.

Yeah, we call two different callbacks on the same paths.

But note: in bibtex-actions, I no longer use the bibtex-completion-init function, which is what sets those up.

So that should only happen if you use both bibtex-actions and ivy/helm-bibtex.

Edit: I will see if I can amend the example script on the wiki to first remove existing watches, and add a note about this issue.

So in the end, you think once I remove the new commands, this is ready to merge?

@wenjie2wang
Copy link
Contributor

So that should only happen if you use both bibtex-actions and ivy/helm-bibtex.

You are right. If I only use bibtex-actions, there will not be such an issue.

So in the end, you think once I remove the new commands, this is ready to merge?

Yes, this PR looks good to me. Thanks!

This allows us to avoid using a lambda on the file-notify callback.
@bdarcus
Copy link
Contributor Author

bdarcus commented Apr 13, 2021

Oops, sorry @wenjie2wang, I accidentally deleted your earlier comment with the code example you used from bibtex-completion.

I had want to reply to note since that code is now on the wiki, we (and others) can experiment with how to improve it.

What I merged ended super simple then: just makes the setup simpler.

@bdarcus bdarcus added this to the v1.0 milestone Apr 13, 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.

2 participants