emacs-citar / citar

Emacs package to quickly find and act on bibliographic references, and edit org, markdown, and latex academic documents.
GNU General Public License v3.0
505 stars 54 forks source link

Fix vanilla Emacs completions #708

Closed rudolf-adamkovic closed 1 year ago

rudolf-adamkovic commented 1 year ago

Describe the bug

Citar does not work well with Emacs' default completions.

To Reproduce

Steps to reproduce the behavior:

  1. Run citar-open
  2. Type TAB

Expected behavior

The completions buffer appears, like in M-x or C-x b.

Emacs version:

29

Additional context

bdarcus commented 1 year ago

AFAIK, citar should work with default completion. But it sounds like there may be some incompatibility with citar--select-multiple, and maybe the related citar--setup-multiple-keymap (assuming there's some issue with the exit keybinding)?

Do you get the same behavior, for example, with citar-open-entry (which doesn't use citar--select-multiple)? I'd guess not.

I don't really have time or expertise to figure this out. Do you think you could figure it out?

rudolf-adamkovic commented 1 year ago

Do you get the same behavior, for example, with citar-open-entry (which doesn't use citar--select-multiple)? I'd guess not.

It behaves the same way. After I execute citar-open-entry, Citar asks "Reference:". Then, I type TAB, and Citar says in "Reference: [sole completion]". Similarly, when I type "a" followed by TAB, Citar deletes the "a" and does nothing. No completions show in either case.

I don't really have time or expertise to figure this out. Do you think you could figure it out?

I know next to nothing about the completion system in Emacs. 😞

For the back story, I had this idea to try the built-in vanilla completion system in Emacs, and after configuring it, I discovered that it works rather well everywhere, except for Citar. Hence, I opened this issue.

bdarcus commented 1 year ago

I really have no clue, other than that it might be some keybinding issue.

I just tried to turn off vertico-mode and see if I could reproduce. I was able to get completions to pop up in a citar command, and select candidates with the mouse. But I couldn't figure out how to move up and down the list, to test what happen with RET!

rudolf-adamkovic commented 1 year ago

I was able to get completions to pop up in a citar command, and select candidates with the mouse.

Interesting. Just by pressing TAB?

bdarcus commented 1 year ago

I think I had to type a character first, but yeah.

rudolf-adamkovic commented 1 year ago

Thank you @bdarcus! That helped. I investigated further, seeing that it worked for you, and found that Citar works perfectly with (setq completion-styles '(basic)) but breaks with (setq completion-styles '(orderless basic)). Not sure why this poses a problem for Citar specifically. Should I close the bug?

bdarcus commented 1 year ago

I investigated further, seeing that it worked for you, and found that Citar works perfectly with (setq completion-styles '(basic)) but breaks with (setq completion-styles '(orderless basic)).

@oantolin - any ideas why this might be the case?

I don't really know anything about the default completion system.

oantolin commented 1 year ago

This is mysterious! I do see this weird behavior where, using default completion and orerless, TAB deletes what you have written. On the other hand, if you just don't press TAB, everything else seems to work with orderless: you can run switch-to-completions (bound by default to M-v) the completions popup and seem correct, and pressing RET (or clicking with the mouse) on one of them does select it correctly. So it's actually usable with orderless, but of course we should try to figure out why TAB isn't working.

oantolin commented 1 year ago

It seems citar binds TAB to minibuffer-complete-and-exit. In default completion that function is usually bound to RET and TAB is usually bound to minibuffer-complete. Citar's binding does seem reasonable since citar-open is basically completing-read in a loop, until you press RET to run citar--multiple-exit.

As we've said, just pressing TAB seems to just delete text and not select anything when using citar with orderless under default completion; but I discovered that if you first run minibuffer-complete you do see the correct completion appear in the minibuffer, and if then you press TAB, it does get selected. Now, that doesn't make sense, because I'd expect minibuffer-complete-and-exit to be basically the same as first running minibuffer-complete and then exiting!

EDIT: Sorry, that's wrong! I thought I was running minibuffer-complete with my old binding for <backtab> (shift + TAB), but I was running minibuffer-force-complete. Running minibuffer-complete deletes the text too, which is a good clue.

oantolin commented 1 year ago

I figured it out! It's a small bug in citar's completion table. The function citar--completion-table creates a predicate to pass to complete-with-action that combines the completion system's predicate with a citar filter. The combined predicate correctly calls filter on the citation key, and should call predicate on the display string of the citation but instead mistakenly calls it on the string the user is trying to complete in the minibuffer!

I'll submit a tiny pull request to correct this.

oantolin commented 1 year ago

That was initially pretty mysterious and was fun to figure out. 😃

aikrahguzar commented 1 year ago

@oantolin, that kind of thing is very hard to debug so thanks for figuring it out. But I am surprised the issue isn't present in vertico.

Citar's multiple selection is also a bit of hack where the intent was to support vertico without depending on it. It seems like it works for default completion too which is nice and probably a consequence of vertico's commitment to consistency with the default completion. I remember trying the fido-vertical-mode and icomplete-mode when I was working on it and they didn't work with multiple completion because the rebindings in minibuffer-with-setup-hook didn't seem to work. Multiple selection was off by default back then so I didn't do investigate much. I wonder if this problem was responsible? Now that multiple selection is default maybe if we should guard it with vertico-mode or maybe check if completing-read-function is completing-read-default?

bdarcus commented 1 year ago

Now that multiple selection is default maybe if we should guard it with vertico-mode or maybe check if completing-read-function is completing-read-default?

Or can just say in the README this is only guaranteed to work with Vertico?

FWIW, I did merge Omar's PR.

Does that solve your issue @salutis?

oantolin commented 1 year ago

@aikrahguzar As far as I can tell the strategy citar uses for multiple selections should work with icomplete (and thus with fido), so if it currently doesn't work perfectly it should be possible to fix without much trouble.

But I am surprised the issue isn't present in vertico.

That's because in the Vertico approach you never attempt to complete an entry so try-completion never gets used! In default completion what citar sets up TAB to do is attempt to complete the current minibuffer input; if the completion is unique, then select it (or deselect it, as the case may be). On the other hand under Vertico TAB doesn't attempt any completion at all, it just selects or deselects the current Vertico selection.

bdarcus commented 1 year ago

I notice that fido-vertical-mode doesn't play well with the citar display calculation code, for some reason. The width is apparently wider than it actually is.

image

oantolin commented 1 year ago

Yeah, the built-in icomplete-vertical-mode (which fido-vertical-mode uses) has a few rough edges. I think you just need to toggle-truncate-lines in the minibuffer. My (older!) icomplete-vertical-mode handles stuff like that for you automatically.

Jousimies commented 1 year ago

If enable complete-vertical-mode, after citar-open-files, the RET bind to citar--multiple-exit. If enable fido-vertical-mode, the RET bind to icomplete-fido-ret. It cause RET do not open files, instead of re-choose reference candidate.

Do not know how to fix, any help will be appreciate.