Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update completer part 3 #12219

Merged
merged 4 commits into from
Apr 8, 2022
Merged

Conversation

trungleduc
Copy link
Member

References

This PR allows completer providers to customize the completer widget model and fixes an issue of activating completer before all providers are registered.

Code changes

  • Add a new signal (providersActivated), that is emitted when all providers are registered.
  • Add an optional modelFactory method to the provider interface.

User-facing changes

N/A

Backwards-incompatible changes

N/A

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@trungleduc trungleduc force-pushed the update-completer branch 2 times, most recently from fe206ed to ec887dc Compare April 4, 2022 18:15
@trungleduc trungleduc closed this Apr 4, 2022
@trungleduc trungleduc reopened this Apr 4, 2022
@trungleduc
Copy link
Member Author

Restart CI

);
}

get providersActivated(): ISignal<ICompletionProviderManager, void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design question, are you interested by listening to the changes in the active providers or in the readiness of the manager at load time?

If this is the second, I would use a ready attribute:

get ready(): Promise<void>

If not, the signal is good, I would use anoterh name like activeProvidersChanged to better reflect when the signal is emitted (as it will be emitted on settings update too).

Look for example at jupyterlab/jupyterlab/packages/apputils/src/sessioncontext.tsx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed I need to call the slot on the provider changed event since I need to update existing completer handlers with the new provider setting.
I'll rename the signal to reflect its intent.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a question about the intent.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions on the doc and it is good to go for me.

packages/completer/src/manager.ts Outdated Show resolved Hide resolved
packages/completer/src/manager.ts Show resolved Hide resolved
packages/completer/src/tokens.ts Show resolved Hide resolved
packages/completer/src/tokens.ts Outdated Show resolved Hide resolved
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
@trungleduc
Copy link
Member Author

trungleduc commented Apr 5, 2022

Thanks @fcollonval for the review and suggestion!

@trungleduc trungleduc closed this Apr 5, 2022
@trungleduc trungleduc reopened this Apr 5, 2022
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @trungleduc

@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table
Test file large_code_notebook large_md_notebook
open
chromium
actual 3453 <- [3762 - 3875 - 3963] -> 4398 2443 <- [2743 - 2793 - 2865] -> 3120
expected 3412 <- [3899 - 3960 - 4071] -> 5041 2390 <- [2681 - 2728 - 2800] -> 3060
Mean relative change -3.1% ± 1.4% 2.3% ± 1.2%
switch-from
chromium
actual 620 <- [674 - 695 - 753] -> 938 479 <- [523 - 544 - 935] -> 1131
expected 624 <- [704 - 731 - 760] -> 948 486 <- [531 - 890 - 941] -> 1206
Mean relative change -2.5% ± 2.9% -8.9% ± 7.5%
switch-to
chromium
actual 303 <- [372 - 379 - 390] -> 428 246 <- [280 - 288 - 296] -> 682
expected 309 <- [381 - 390 - 399] -> 426 229 <- [276 - 282 - 291] -> 633
Mean relative change -2.3% ± 1.5% 1.9% ± 3.9%
close
chromium
actual 648 <- [901 - 921 - 937] -> 1018 450 <- [474 - 480 - 492] -> 527
expected 576 <- [912 - 936 - 956] -> 1004 437 <- [470 - 477 - 490] -> 833
Mean relative change -1.3% ± 1.7% 0.0% ± 1.7%

Changes are computed with expected as reference.

@fcollonval fcollonval merged commit c847493 into jupyterlab:master Apr 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants