Consider to render only visible candidates instead of company-box-max-candidates like now

[kiennq]

Hi @sebastiencs, thanks for the great package.
I notice that company-box is quite slow, mostly because it has to re-render all of candidates.
Would it makes sense to only render visible candidates only?
This will be a huge performance gain, and also removes the restriction of company-box-max-candidates too.

Also since company-box actually do the rendering of the candidate itself, it was missing quite a few properties from company-mode like matches highlighting for flx match.
Should we drop the company-box--make-line and use company--create-lines instead?

I've push a commit here kiennq@8f675f7 as POC.
You can try the other improvements in my repo https://github.com/kiennq/company-box/ too.

Let me know if you think this is possible to be merged back to main stream project.

[sebastiencs]

Hi @kiennq, thanks for your comment.

I would prefer to not use company--create-lines, the default front-end is based on an overlay and all the code around it is "limited" by that.
Overlays are good, but company-box has it's own frame which allows us to make use of all buffer properties, window etc. which I intend to use for this package instead of just using the default frontend code and only prepend on each line an icon.

It has its downsides, I know, such as missing features etc. But I would prefer to keep it this way.

All the missing features just need to be implemented, I guess.
I'm not familiar with flx, how does it highlight the matches for the default front-end ? Is there a way to support it in company-box ?

For performance, that's interesting, I never encounter such issue. Would you please reproduce with the profiler ?
What is the value of company-box-max-candidates ?
As you say company-box renders all lines right now, but I'm sure there are lots of area to improve this.

[dgutov]

I would prefer to not use company--create-lines, the default front-end is based on an overlay and all the code around it is "limited" by that.

company-posframe uses this function too, and it's not based on overlays.

[sebastiencs]

@dgutov Right, it's not based on overlays. But its output is intended to be used in overlays, so it doesn't make use of features available outside of overlay context, unless I am missing something ?

@kiennq What other features are you missing that is available with company--create-lines but not in company-box, other than flx ?

[dgutov]

@sebastiencs

Right, it's not based on overlays.

company-posframe is not based on overlays.

[sebastiencs]

@dgutov I didn't say company-posframe is based on overlay, did I ?

But its output is intended to be used in overlays, so it doesn't make use of features available outside of overlay context, unless I am missing something ?

I was talking about the output of the function company--create-lines here

[dgutov]

it doesn't make use of features available outside of overlay context

Which features are those?

[sebastiencs]

@dgutov company--create-lines doesn't use windows margins which I just used to support company-show-numbers, or the display property align-to margin etc..

My point is child frames gives a freedom to the implementation of company-box and using company--create-lines would not make full use of that

[dgutov]

At some point you start to have a lot of duplication in the code.

Not using company--create-lines is perhaps fine if you're prepared to reimplement it and deal with the resulting bugs, but having a separate set of commands such as company-box-mode-map doesn't seem like a good idea (look at all the associated bug reports).

[sebastiencs]

Sure, which is why I replied:

It has its downsides, I know, such as missing features etc. But I would prefer to keep it this way.

I would have just used company--create-lines and add an icon to each line if I wanted to go the easy way.
I'm fine dealing with bugs, I wouldn't make company-box open source if I wasn't.

I don't see many bugs about reimplementing company--create-lines or company-box-mode-map though.

[sebastiencs]

@dgutov I agree that company-box-mode-map is not a good idea. I will find a solution and change that