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
Remove deregisterProvider from SearchProviderRegistry interface #6282
Remove deregisterProvider from SearchProviderRegistry interface #6282
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple docs items. Thanks!
@@ -85,6 +80,10 @@ export class SearchProviderRegistry implements ISearchProviderRegistry { | |||
); | |||
} | |||
|
|||
get changed(): ISignal<this, void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring?
* @returns true if removed, false if key did not exist in map. | ||
*/ | ||
deregisterProvider(key: string): boolean; | ||
register(key: string, provider: ISearchProviderConstructor): IDisposable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing doc for the return value?
Thanks, good catches! |
References
Addresses an issue brought up in #6217.
Code changes
This removes the
deregisterProvider
function onISearchProviderRegistry
, renamesregisterProvider
to simplyregister
and returns aDisposableDelegate
, and adds achanged
signal toISearchProviderRegistry
that emits when a provider has been registered or disposed (and effectively deregistered). The goal here was to allow extensions to only 'deregister' the providers that they provide and no others.Also, with the new signal, we can ensure that the
jp-mod-searchable
class that controls the ctrl + f and other search-related keybindings is added to and removed from active widgets only when they have a registered search provider.User-facing changes
There should be no user-facing changes here, other than a slightly tighter control on when the search keybindings are routed to the browser vs a search provider when providers are registered/deregistered.
Backwards-incompatible changes
This makes three breaking changes to the
ISearchProviderRegistry
interface, as described above:registerProvider
->register
and it now returns anIDisposable
deregisterProvider
changed
signal