Add corfu-doc

[galeo]

Hi @minad

According to your request, I refactored the code in corfu.el that creates the child frame and sets the frame position and size. I also made some modifications to meet the needs of corfu-doc. As suggested by Stefan's code review, the version of corfu-doc is set to 0.8, also to transition from my repo version.

Please review the code when you have time.

Thanks.

[minad]

I took a first look at your PR. Can you please turn on package-lint and fix the warnings, add missing docstrings, etc.?

[minad]

@galeo I merged your modifications to corfu.el and made a few minor changes (450cf9d, 0bc9262).

Can you please update corfu-doc.el accordingly? Then I will proceed with a more detailed review of corfu-doc.el.

[minad]

There were two things I disagreed with, the REDISPLAY-HACK argument and the CONTENT-HANDLER. Can you explain why the REDISPLAY-HACK flag is needed for corfu-doc? The content handler is unnecessary since you can just modify the buffer after corfu--make-frame given the refactoring I made.

[galeo]

The content handler is unnecessary since you can just modify the buffer after corfu--make-frame given the refactoring I made.

I'll have a look and update corfu-doc.el.

Can you explain why the REDISPLAY-HACK flag is needed for corfu-doc?

When this flag is on, the code (sleep-for 0.01) will cause a delay to move the doc frame.

[galeo]

Can you please update corfu-doc.el accordingly?

Done.

[minad]

Done.

Great thanks! I'll take another look at the code.

[minad]

@galeo I just tested the current version again. It works beautifully now with the addition of corfu-doc-transition! Don't you prefer the behavior like this too? Maybe we could just get rid of corfu-doc-transition? I usually tend to avoid adding configuration options if it is not really necessary. Packages get more complex over time, so it is better to start small.

[galeo]

@minad

I tried corfu-doc again and unfortunately the auto popup doesn't appear reliably for me. Sometimes it appears and after a while it stops appearing. Then I can manually summon the popup using the corfu-doc-popup-toggle, but it won't update anymore automatically. Did you observe this too?

I haven't spent much time on Emacs recently due to work. I feel that it may be related to the previous problem.
Because it is not sure when the corfu popup is first displayed for the preselected item, a repeating timer is used. Here is the code in corfu-doc-popup--setup:

      (if corfu-doc-popup-auto
          (progn
            (setq corfu-doc-popup--preselect-first-timer
                  (run-with-timer 0.1 0.1 #'corfu-doc-popup--post-command))
            (add-hook 'post-command-hook #'corfu-doc-popup--post-command
                      'append 'local))

The timer will be canceled in corfu-doc-popup--post-command:

(when corfu-doc-popup--preselect-first-timer
      (cancel-timer corfu-doc-popup--preselect-first-timer)
      (setq corfu-doc-popup--preselect-first-timer nil))

I feel that this problem should be caused by the execution of the repeating timer. Is there be a problem that the timer is not actually canceled and causes corfu-doc-popup--post-command to be executed repeatedly? I need to find some time to reproduce this problem. Please help check here if you have time.

The second issue is that the popup placement is a bit unpredictable. I'd like to always have the popup placed below, to the right or the left during a single completion session. I find it disturbing if the popup jumps around. My proposal is to compute the largest area next to the popup and always use that for the popup. You can choose some weights, e.g., such that the space to the left or right is rated higher.

The position of the doc popup is indeed set according to the calculated largest area. Here is the code:

  (pcase-let*
      ((`(,width . ,height)  ;; popup inner width and height
         (corfu-doc-popup--size width height))
       (`(,v-x ,v-y ,v-w ,v-h)
         (corfu-doc-popup--display-area-vertical width height)))
    (if (and (>= v-h height) (>= v-w width))
        (list v-x v-y v-w v-h)
      (pcase-let ((`(,h-x ,h-y ,h-w ,h-h)
                    (corfu-doc-popup--display-area-horizontal width height)))
        (if (>= (* v-w v-h) (* h-w h-h))
            (list v-x v-y v-w v-h)
          (list h-x h-y h-w h-h)))))

Because corfu popup sometimes has a large width(especially some programming languages ​​with long completion items), and there is not enough space in the horizontal direction. So the vertical direction is first selected to display. The horizontal direction is selected only if there is not enough space in the vertical direction. If there is not enough space in the horizontal direction, the display position will be selected according to the calculated size. During a single completion session, the size of the corfu popup changes, resulting in a change in the size of the area available to display the doc popup. If the position of the popup is limited to a certain direction, it may cause insufficient display space. It is not difficult to adjust here, the above code is the corfu-doc-popup--display-area function. In addition, it may be related to personal habits, or this issue can be put on hold.

[minad]

@galeo Thanks for looking into this.

I feel that this problem should be caused by the execution of the repeating timer.

I agree. I guess we have to find a better solution than the timer since it is too unreliable like this. Maybe it makes more sense to use an :after advice on corfu--exhibit and update the doc popup there according to the Corfu state. Did you have that before changing to post-command-hooks?

During a single completion session, the size of the corfu popup changes, resulting in a change in the size of the area available to display the doc popup. If the position of the popup is limited to a certain direction, it may cause insufficient display space.

This doesn't sound right. I've even observed the jumping around when scrolling around. During scrolling the Corfu candidates popup size doesn't change. So this also has something to do with the size of the doc popup?

Anyway, I find it disturbing enough that the Corfu candidate popup itself changes size and jumps around. The doc popup should stick to a direction. It should be sufficient to store the last direction and keep on using this direction even if the Corfu popup changes its size.

[minad]

I just checked - in your original corfu-doc package you advice the corfu--popup functions, which I don't really like. But in contrast advising corfu--exhibit seems okay, since this is the main function which updates the Corfu state.

https://github.com/galeo/corfu-doc/blob/da931367802d01e87e1e496ba5b51aa9126a705d/corfu-doc.el#L484-L485

[galeo]

@minad

I guess we have to find a better solution than the timer since it is too unreliable like this. Maybe it makes more sense to use an :after advice on corfu--exhibit and update the doc popup there according to the Corfu state.

But in contrast advising corfu--exhibit seems okay, since this is the main function which updates the Corfu state.

I've updated the code to use an after advice for corfu--exhibit. Please help verify that the first issue is resolved.

This doesn't sound right. I've even observed the jumping around when scrolling around. During scrolling the Corfu candidates popup size doesn't change. So this also has something to do with the size of the doc popup?

Yes. The size of the doc popup is calculated dynamically and its size may change when the current candidate changes. In addition, when scrolling, the size of the corfu popup will indeed change(e.g. when the length of the selected candidate exceeds the width of the popup).

Anyway, I find it disturbing enough that the Corfu candidate popup itself changes size and jumps around. The doc popup should stick to a direction. It should be sufficient to store the last direction and keep on using this direction even if the Corfu popup changes its size.

I've also updated the code so that the doc popup will stick to a direction during a completion session. Please try again and see how it works.

Thanks.

[minad]

@galeo Thanks! Sounds good.

[AkibAzmain]

@AkibAzmain

Please address the issues I pointed out to allow using corfu-doc-popup on terminal.

Please be patient regarding supporting corfu-doc-popup in corfu-terminal. I've tried corfu-doc a few times recently but unfortunately it still has some issues, which prevent the feature from being merged. We should focus first on getting corfu-doc ready, before we discuss how terminal support should be implemented.

I'm not going to support corfu-doc-popup in corfu-terminal right now, because corfu-doc-popup is pretty unstable at this point. That was just a remainder for you, so that you remember to keep the child frame part separate, otherwise implementing terminal support is impossible without reinventing the wheel.

[minad]

@AkibAzmain Sure it would be good if we can avoid reinventing the wheel. But note that I strongly disagree with the current implementation of corfu-doc-terminal (see the "advice hell" in https://codeberg.org/akib/emacs-corfu-doc-terminal/src/commit/be9fd5cd6c293dea862f1a1877378f1391e6a7e5/corfu-doc-terminal.el#L612-L631). I would argue that reinventing the wheel in the case of corfu-doc-terminal may be easier than trying to reuse corfu-doc. Anyway, first things first - when I find time I will @galeo's next iteration of corfu-doc-popup a try.

[AkibAzmain]

@minad Yes, advices are often bad, since they often depend on the internals of a package. corfu-doc (the version on MELPA) currently has a child-frame dependent part and a child-frame independent part, both clearly separated (like Corfu). There are ten advices in corfu-doc-terminal, but each of them just override the corresponding function (I use :around to use the original function in GUI), replacing the whole child-frame dependent part with Popon equivalent (similar to a Company frontend). I have tried my best to keep things simple, and these advices (and those in corfu-terminal) are actually some of the most sane advices I have ever seen (I don't like to praise my own code, but these advices are really better that others). IIUC the advices will break if and only if the advised function's behavior change. So maintaining the terminal UI shouldn't be hard if we cooperate. (If you don't think so, you're welcome review the code, but please don't judge by number of advices.) Thanks to @galeo for cooperating to implement corfu-doc-terminal (galeo/corfu-doc#12 and galeo/corfu-doc#13)!

However, I think we should focus on completing this new corfu-doc-popup before implementing half-complete terminal support for half-complete corfu-doc-popup.

[minad]

@AkibAzmain 10 advices are way too much and I won't give you guarantees about this. If you look for example at corfu.el or vertico.el, I use two or three advices. The same applies to your corfu-terminal, which is fine. We have to find a better solution for corfu-doc-terminal.

[AkibAzmain]

@minad I don't need guarantee. I have no problem if you break it from time to time, as long as you keep child-frame dependent part clearly separated, so that I can override that.

[minad]

@AkibAzmain Of course. But if we have to split up the code such that you can install ten advices, we won't do it. This is not a good design. I hope we can refactor the code somewhat that you don't need as many advices and such that the separation between popup and child frame code is more clear. I understand your sentiment about "good advices", which cleanly replace an implementation, but the pure number should still somewhat stay in check.

Note that I did the same in corfu.el itself, where we have a separation between frame and popup. This separation is what enabled corfu-terminal in the first place. If we can do the same for corfu-doc-popup, great, if not, there is no other way than reinventing the wheel for the terminal 🤷

I should add - your corfu-doc-terminal package is longer or as long as corfu-doc itself. So I think you wouldn't be in a worse situation if you just reinvent the wheel. This complexity is also a symptome of the excessive number of advices and the mismatch in requirements on terminal and graphical display. The goal should be to figure out an implementation where more of the code can be shared.

[minad]

Okay, I've decided to merge this now as is. We can still improve it as part of Corfu.

[galeo]

@minad That's great, thank you!

[AkibAzmain]

your corfu-doc-terminal package is longer or as long as corfu-doc itself

Yes, because it has to show a buffer in a popon, which needs some code. If I remove three biggest functions, line count decreases from 668 to 390, less than corfu-doc. My code is only for showing the Popon, if I add the doc fetch code from corfu-doc, it would almost certainly reach thousand lines.

[minad]

@AkibAzmain I've reworked and simplified the extension heavily. Please take a look and let me know where we would have to make changes. The problem is that the extension is relying a lot on frame functions. It would be best if you can create a PR. Is there a chance that you can get the copyright assignment?

[AkibAzmain]

@minad I've noticed that after installing the latest Corfu from elpa-devel. I've just opened the issue #248 for this discussion, with two proposals to help implement terminal support. I've also opened akib/corfu-terminal#16 on Codeberg.

And quoting from one of my emails to emacs-devel list:

Before someone asks me to consider assigning my copyright, I would like
to say that a printed copy of assignment agreement is in my hands right
now, but I can't sign it now, and I don't know when I'll able to sign
(don't get me wrong, I really want to sign it).

[AkibAzmain]

Looks like you have renamed the extension, just noticed now.

[AkibAzmain]

@minad,

It would be best if you can create a PR. Is there a chance that you can get the copyright assignment?

I can't create a merge-able PR now, but I can indeed create a PR for you (or anyone else who completed the paperwork, perhaps @galeo) to take inspirations from make the real changes. Should I try to do this?

[minad]

@AkibAzmain No, this won't be compatible with the copyright. As I wrote in #248, I'd like to let this rest for a while. There are already bug reports coming in (#247, #249)! Of course, if someone with copyright assignment comes along I'd appreciate a PR.

[AkibAzmain]

I think you got me wrong, that PR wouldn't be for merging, that would be for someone (with the papers, of course) to take some inspiration from and do the real change themselves.

[minad]

@AkibAzmain Look, this won't be an approach which is legally sound. End of discussion.

[AkibAzmain]

@minad Thanks for the clarification. EOD (end of discussion).