(Hopefully) fixed version of #717

[aikrahguzar]

In the end issue was obvious and was introduced by the attempt to fix the problem where exiting without input resulted in the previous input being selected.

I think it is fixed now but probably best to test it more this time.

[bdarcus]

I'm again not sure if I'm doing something wrong with getting it loaded correctly, but I think it is, in the sense that new symbols are accessible.

But I still get the same behavior.

Shrug; will try to experiment more.

[aikrahguzar]

I'm again not sure if I'm doing something wrong with getting it loaded correctly, but I think it is, in the sense that new symbols are accessible.

But I still get the same behavior.

Shrug; will try to experiment more.

Can you check if the last commit is included? It doesn't involve any new symbols. Without it when using citar-insert-citation if I select any entry without any input, nothing gets inserted, with the last commit, the entry does get inserted.

[bdarcus]

Hmm ... this is weird.

If I do org-cite-insert, I get abort.

If I do citar-insert-citations, I get in the buffer [cite:].

The same thing happens whether I hit TAB or RET.

Git log is telling me your latest commit is there.

How I set it up with Doom (I can test vanilla later):

I have my local citar repo as my package repo.

I run doom upgrade.

Even if I open citar.el, no change.

As I said, though, I'll check on my vanilla install a bit later and report back.

[aikrahguzar]

How I set it up with Doom (I can test vanilla later):

I have my local citar repo as my package repo.

I run doom upgrade.

I think local-repos have some weirdness. You will probably need doom build -r

But you don't really need that. If the commit is in the file just do gR with the file open. That is quicker for testing.

[bdarcus]

You will probably need doom build -r.

Still no change.

If the commit is in the file just do gR with the file open.

gR = revert?

[aikrahguzar]

gR = revert?

No, +eval/buffer in normal-mode. Doom also has bunch of bindings for evaluating stuff on gr in normal mode.

[bdarcus]

I still can't get it work, either in Doom, or with the vanilla "test" script in the repo.

Can you confirm you can do the latter?

To do that, you should just be able to cd to /test/manual and then run ./run.sh, and set the correct path for the bibliography.

[aikrahguzar]

I still can't get it work, either in Doom, or with the vanilla "test" script in the repo.

Can you confirm you can do the latter?

To do that, you should just be able to cd to /test/manual and then run ./run.sh, and set the correct path for the bibliography.

Yes, ./run.sh works for me. If I set the path and then open /tmp/test.org I am able to insert citations using both citar-insert-citation and org-cite-insert both with and without input and I can recall previous input using M-n.

[bdarcus]

And are you sure your local repo and whatever you pushed matches?

E.g. you don't have any uncommitted and pushed code?

If not that, I have no other ideas why the discrepancy.

[aikrahguzar]

And are you sure your local repo and whatever you pushed matches? E.g. you don't have any uncommitted and pushed code? If not that, I have no other ideas why the discrepancy.

I have this in my packages.el now,

(package! citar :recipe (:host github :repo "aikrahguzar/citar" :branch "retain-input"))
(unpin! citar)
(unpin! citar-embark)

(Notice the unpinning of citar-embark otherwise the pin of citar-embark overrules the unpinning of citar since they are in the same repo. I think the pin on citar-emabrk should be removed in biblio modue. Another way would be to give a :files that removes exclusion of citar-embark.)

and everything works for me in doom after doom sync and doom up. So I have no idea what is causing the discrepancy :shrug:

[bdarcus]

Notice the unpinning of citar-embark otherwise the pin of citar-embark overrules the unpinning of citar since they are in the same repo.

Oh, that's probably it; will check in a bit.

I think the pin on citar-emabrk should be removed in biblio modue. Another way would be to give a :files that removes exclusion of citar-embark.)

How do you think we should approach this? Ask Henrik on Discord?

Edit: actually, I think he'll want to pin it anyway.

[bdarcus]

Unpinning alone doesn't solve it, and that doesn't explain why the test script shows the same behavior.

[aikrahguzar]

Unpinning alone doesn't solve it, and that doesn't explain why the test script shows the same behavior.

I wasn't expecting it change anything on your side, it was mostly to check that nothing was happening due to local chnages. Can you switch to my repo on doom as above and see if it changes anything?

How do you think we should approach this? Ask Henrik on Discord?

Edit: actually, I think he'll want to pin it anyway.

It is good to ask him how to deal with this problem. I hadn't noticed this before but the pin is not on the package but on the repo so if citar is pinned so is citar-embark.

[bdarcus]

Can you switch to my repo on doom as above and see if it changes anything?

I'm starting to think something's just wrong with my doom and/or straight dirs, as making this change doesn't work as I expect. Usually when this happens, I just wipe the straight directory and reinstall everything.

[bdarcus]

I did manage just now to get it working as expected with my elpaca-based vanilla config, and this:

(elpaca-use-package
  (citar :branch "retain-input" :host github :repo "aikrahguzar/citar")
  :custom
  (citar-bibliography bd/bibliography)
  (citar-notes-paths bd/notes)
  (citar-library-paths bd/library-files)
  (org-cite-global-bibliography '("~/bib/references.bib"))
  (org-cite-insert-processor 'citar)
  (org-cite-follow-processor 'citar)
  (org-cite-activate-processor 'citar))

(elpaca-use-package
  (citar-embark :branch "retain-input" :host github :repo "aikrahguzar/citar")
  :after citar embark
  :no-require
  :config (citar-embark-mode))

I still want to figure out why the earlier wasn't working, but will maybe need to look again with fresh eyes tomorrow!

[bdarcus]

Hmm ... maybe I'm crazy, but testing with diff just now, I don't think the file here on the PR on github is the same as in your repo.

I reinstalled all packages with doom, using your repo for the recipes.

With that, repo and build file are the same, as they should be.

But if I download the file from this PR, it's (again, unless I'm crazy) different.

If I'm right, then maybe I'll have to merge it to a local branch and update the file before merging. Unless you have any other ideas?

[aikrahguzar]

But if I download the file from this PR, it's (again, unless I'm crazy) different.

ediff doesn't report any difference between my local copy of citar.el and the file in your link and the I see the latest changes I did in it but maybe github is being weird?

If I'm right, then maybe I'll have to merge it to a local branch and update the file before merging. Unless you have any other ideas?

I can try rebasing on top of current main? Maybe that will help. But I generally deal with git by fumbling around with magit so I don't actually know if it will help or hurt.

[bdarcus]

... maybe github is being weird?

That's what I'm wondering ATM.

Except, you're seeing the files as the same, so not sure then.

I can try rebasing on top of current main? Maybe that will help. But I generally deal with git by fumbling around with magit so I don't actually know if it will help or hurt.

You can try, though my guess is there will be no change.

OTOH, if there is a change, that may be it.

If you end up with a conflict, you can just abort the rebase.

[aikrahguzar]

You can try, though my guess is there will be no change.

OTOH, if there is a change, that may be it.

If you end up with a conflict, you can just abort the rebase.

Rebasing is done. Everything still works for me. Do you see any change?

[bdarcus]

Looking again today, it's still not making any sense to me.

Here's the checksum for the file doom installs from your repo branch:

8d16b8cb3602f64def656c18458e6d8f

... and here's the checksum for the file that github provides here:

c4050db34b52b790218185e32d2264eb

Clearly that's wrong (right?).

So I don't know what's going to happen if I merge it.

I may just create an intermediate branch here as a target for the merge, so that at least I can test it and see. Does that make sense?

EDIT: I'm downloading your repo now to check that.

[bdarcus]

Hmm ... this doesn't fully explain it in my mind, but it seems your repo it out of date, so your main is not the same as this main.

See https://medium.com/@topspinj/how-to-git-rebase-into-a-forked-repo-c9f05e821c8a.

[aikrahguzar]

Hmm ... this doesn't fully explain it in my mind, but it seems your repo it out of date, so your main is not the same as this main.

See https://medium.com/@topspinj/how-to-git-rebase-into-a-forked-repo-c9f05e821c8a.

Why does my main branch matter? It has some other stuff on it from a previous time so I made the new branch and I am using the new branch for this pr.

I may just create an intermediate branch here as a target for the merge, so that at least I can test it and see. Does that make sense?

This seems like a good approach to me. It might be better to even test the pr for a few days to see that there is no edge case.

[bdarcus]

IUC, because your branch is off a different state of the history, so the diff is different?

But as I say, it doesn't make total saw en sense to me, since I would expect GitHub would indicate a merge conflict.

[aikrahguzar]

IUC, because your branch is off a different state of the history, so the diff is different? But as I say, it doesn't make total saw en sense to me, since I would expect GitHub would indicate a merge conflict.

But I didn't branch retain-input from my main branch. It started from the main here (at the time when I first started the pr) which I why the first pr worked.

[bdarcus]

Ah; ok!

[bdarcus]

This seems like a good approach to me. It might be better to even test the pr for a few days to see that there is no edge case.

OK, I'll do that.

[bdarcus]

OK, merged to the retain-input-pr branch here, and confirmed the two files are the same!

But I haven't yet gotten it working :-(

Will keep plugging away.

[aikrahguzar]

OK, merged to the retain-input-pr branch here, and confirmed the two files are the same!

But I haven't yet gotten it working :-(

Will keep plugging away.

For me

(package! citar :recipe (:host github :repo "emacs-citar/citar" :branch "retain-input-pr"))
(unpin! citar)
(unpin! citar-embark)

works as expected on doom.

[bdarcus]

I'm at this point utterly perplexed. I'm using the exact same doom code in my packages.el, and rebuilt it all, and I still get the previous errors.

What version of Emacs are you using?

Tomorrow I'll try on a different machine, as well as with vanilla, and see if anything changes.

[aikrahguzar]

What version of Emacs are you using?

I am on emacs-29 and that is the problem. Using a function as the require-match argument in completing-read is new feature in 29. I don't have a good idea of how to differentiate the default from empty input without it so this has to wait a little bit.

Sorry about this!

[bdarcus]

That at least explains it (I'm on 28 on that machine), though odd Emacs doesn't give me any warning about that!

[bdarcus]

Thought: perhaps it's feasible, if there's no easy way to implement this pre-28, that the functionality is only available post?

That way it doesn't fail generally; one just loses that feature?

[aikrahguzar]

Thought: perhaps it's feasible, if there's no easy way to implement this pre-28, that the functionality is only available post?

That way it doesn't fail generally; one just loses that feature?

I think it can be implemented pre-29 by comparing the last input with what completing-read hands us. This will have one edge case which I think should be extremely rare considering how long the citar strings are: if the user inserts the whole citar candidate in the minibuffer, presses TAB to select it and then right afterwards tries to deselect it without any input, instead of deselecting it, the UI will exit with the candidate selected.

I think the scenario of user entering the whole candidate is extremely unlikely, and probably will be done only by mistake. On vertico the only reasonable way of attempting that is vertico-insert usually bound to TAB which we shadow.

[bdarcus]

... odd Emacs doesn't give me any warning about that!

I think we need to add tests with features like this, so the CI can catch such problems.

In this case, the test should be trivial.

[aikrahguzar]

... odd Emacs doesn't give me any warning about that!

I think we need to add tests with features like this, so the CI can catch such problems.

In this case, the test should be trivial.

It doesn't look trivial to me at all. What changed was how completing-read handles one of its arguments in emacs-29, I don't see what emacs-28 could have warned about since for 28 you could pass a function to require-match argument and it would behave as a generic non-nil value. Any warning would have required completing-read to enforce some types as far as I can see.

[bdarcus]

I was just thinking that IIRC this returns nil pre-29, while it should return a string token, but I admittedly didn't think it through:

(citar-select-refs (list "foo"))

[aikrahguzar]

I was just thinking that IIRC this returns nil pre-29, while it should return a string token, but I admittedly didn't think it through:

(citar-select-refs (list "foo"))

I think the failures we had required interactive input from the user and involved selection not working.

[bdarcus]

Using a function as the require-match argument in completing-read is new feature in 29.

May be worth checking the compat library to see if that adds that functionality to earlier versions?

[aikrahguzar]

May be worth checking the compat library to see if that adds that functionality to earlier versions?

I checked and it doesn't as far I can tell.

Doing that would also be against a few of points list in limitations section of its manual.