search

galeo / corfu-doc

Documentation popup for Corfu
GNU General Public License v3.0
64 stars 4 forks source link

`corfu-doc-terminal` broke after commit caccd94 #18

Closed AkibAzmain closed 2 years ago

AkibAzmain commented 2 years ago

After commit caccd9475783cfc12ab5c49d44b6ca7535e84a4a, corfu-doc-terminal broke (corfu-doc-terminal#2). Can you revert the change? I need corfu-doc--get-cf-popup-edges to make corfu-doc-terminal work.

If this change is for add corfu-doc to Corfu, then I would suggest making this kind of changes in a dedicated branch, so that the MELPA version is not affected.

minad commented 2 years ago

@AkibAzmain This breakage is due to refactorings necessary for inclusion in Corfu itself - I asked @galeo to specifically not care about corfu-doc-terminal in the PR for the refactorings. So this breakage is my fault to a certain extent. See https://github.com/minad/corfu/pull/178. API stability cannot guaranteed for the transition. I am not sure how @galeo wants to proceed with this repository here and the released corfu-doc version. Maybe keep it up to date with the PR until corfu-doc is merged into Corfu itself?

I looked at corfu-doc-terminal - the way it is implemented is a recipe for breakage since you override tens of functions. In the long term this solution won't work. Is there a better way to implement corfu-doc-terminal? Do we need better abstractions? When I compare corfu-doc-terminal with corfu-terminal, corfu-terminal is very short, advises three functions, has only 200 lines and will be very stable in the way forward. In contrast corfu-doc-terminal is even LONGER than corfu-doc itself. Did you consider instead reimplementing it from scratch for the terminal?

galeo commented 2 years ago

Hi @AkibAzmain

The commit has been reverted, temporarily. I agree with minad that corfu-doc-terminal should be considered to be implemented from scratch. Therefore, this commit may be applied again in the future.

Maybe keep it up to date with the PR until corfu-doc is merged into Corfu itself?

I think yes, I want to transition the code as much as possible.

AkibAzmain commented 2 years ago

@AkibAzmain This breakage is due to refactorings necessary for inclusion in Corfu itself - I asked @galeo to specifically not care about corfu-doc-terminal in the PR for the refactorings. So this breakage is my fault to a certain extent. See minad/corfu#178. API stability cannot guaranteed for the transition. I am not sure how @galeo wants to proceed with this repository here and the released corfu-doc version. Maybe keep it up to date with the PR until corfu-doc is merged into Corfu itself?

I understand the matter. I wouldn't open the issue if someone didn't open an issue in corfu-doc-terminal.

I looked at corfu-doc-terminal - the way it is implemented is a recipe for breakage since you override tens of functions. In the long term this solution won't work. Is there a better way to implement corfu-doc-terminal? Do we need better abstractions?

You are right, the advices are ugly. But unfortunately I can't remove any advice, since there are many functions in corfu-doc that use the frame directly.

When I compare corfu-doc-terminal with corfu-terminal, corfu-terminal is very short, advises three functions, has only 200 lines and will be very stable in the way forward.

That's because you have only two function that uses the frame (corfu--make-frame doesn't count since that isn't called outside of that two functions). Maybe this is because Corfu used to use overlay to show popup.

In contrast corfu-doc-terminal is even LONGER than corfu-doc itself. Did you consider instead reimplementing it from scratch for the terminal?

Oh, I didn't notice that, but its unfortunately true. I actually wanted to reuse some code from corfu-doc. So it doesn't contain code for getting the documentation and deciding when to show/hide/update the popup, but still its larger than corfu-doc because I need to do line wrapping, scrolling, show margins, etc by hand (Emacs does these itself for corfu-doc). So if i reimplement corfu-doc-terminal, it would actually increase the size of it (probably it would exceed thousand lines), without any notable benefit.

minad commented 2 years ago

@AkibAzmain

That's because you have only two function that uses the frame (corfu--make-frame doesn't count since that isn't called outside of that two functions). Maybe this is because Corfu used to use overlay to show popup.

This is certainly a reason. I also layered the APIs in Corfu intentionally and wrapped all the popup code away in three popup-related functions. Therefore it becomes easier to override the display code.

You are right, the advices are ugly. But unfortunately I can't remove any advice, since there are many functions in corfu-doc that use the frame directly.

I hope we can figure out a better solution. When corfu-doc will get merged into Corfu we will not expose all these functions on such a fine-grained level. We will have to refactor corfu-doc such that it gets along with fewer popup-related functions similar to Corfu itself, such that the interface you have to override hopefully gets smaller.

minad commented 2 years ago

@AkibAzmain I wonder, as soon as corfu-doc enters Corfu you will probably add the support code to corfu-terminal itself? I assume there won't be a separate package?

AkibAzmain commented 2 years ago

@AkibAzmain I wonder, as soon as corfu-doc enters Corfu you will probably add the support code to corfu-terminal itself? I assume there won't be a separate package?

Exactly.