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.

Sign up for GitHub

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

Jump to bottom

No consistent way to execute a callback function upon acceptance of a Completer item #16294

Closed
ajbozarth opened this issue May 6, 2024 · 5 comments · Fixed by #16312
Closed

No consistent way to execute a callback function upon acceptance of a Completer item #16294

ajbozarth opened this issue May 6, 2024 · 5 comments · Fixed by #16312
Assignees
@ajbozarth
Labels
enhancement pkg:completer

Comments

@ajbozarth
Copy link
Contributor

ajbozarth commented May 6, 2024 •
edited

Problem

When accepting a item in the InlineCompleter you can connect a callback function to the inline-completer:accept command, but in the case of the Completer, the various equivalent commands completer:select-notebook, completer:select-file, and completer:select-console are not always executed when accepting an item.

The Completer class has a private Signal _selected that is triggered in the code in four places:

  1. The selectActive() function which is connected to the various commands listed above
  2. When the Completer only has one item it is automatically accepted
  3. When the Completer is run with a query and only has one item it is automatically accepted
  4. When the user selects the item by click on it with their mouse

This means that there is no way to connect a callback function to cases 2-4 since the command is not executed and the signal triggered is private.

Proposed Solution

The most straight forward solution I see is finding a way to surface the private _selected Signal, this is already possible to do where the Completer class is accessible via the selected() function, but there is no current access of a Completer class to extensions. All calls to Completer instances are done via the private _panelHandlers Map in the CompletionProviderManager which is not surfaced.

If we create a accessor function to _panelHandlers, possibly a lookup method that with a widget id arg, to the CompletionProviderManager then any extension can get the Completer for a given widget and add a connection to the selected signal.

Additional context

This is important to my use case because our Completer extension wants to track the metrics when users accept completion items.

This could be related to #15026, which would also solve this problem, but with a more complicated solution for this use case, trying the callback into the actual implementation of the insertText
@ajbozarth ajbozarth added enhancement status:Needs Triage Applied to new issues that need triage labels May 6, 2024
@krassowski
Copy link
Member

because our Completer extension wants to track the metrics when users accept completion items.

Do you want to track acceptance of suggestions from your Provider (I'm assuming that you have one), or from all providers?

Currently I think panelHandlers should remain implementation detail, but there could be a high level signal which would proxy Selected from all widgets.

@ajbozarth
Copy link
Contributor Author

I only need it for my own provider, I'm fine with any command or signal that I can connect to, there just isn't one at the moment, so I figured surfacing the existing one was the easiest.

@ajbozarth
Copy link
Contributor Author

@krassowski If we don't surface panelHandlers (or it's contents, as I was suggesting), how would you suggest we surface the selected Signal? Is there a global Signals registry to add it to that I'm unaware of?

@krassowski
Copy link
Member

I only need it for my own provider

ICompletionProviderManager.registerProvider() currently returns void. One idea would be to return an object following a new interface which could include the selected signal. It could also include dispose() method to implement IDisposable and calling that method would de-register the provider.

What do you think?

@ajbozarth
Copy link
Contributor Author

ICompletionProviderManager.registerProvider() currently returns void. One idea would be to return an object following a new interface which could include the selected signal. It could also include dispose() method to implement IDisposable and calling that method would de-register the provider.

I'm not against the idea, but while digging into the idea I did think of a middle ground. An accessor on CompletionProviderManager to get the selected Signal for a given widget id, that function would then cycle through panelHandlers and create a signal connected to the select signal on all the handlers for that id, this would work like your earlier idea:

a high level signal which would proxy Selected from all widgets.

This would also give a generic signal, rather than specific to a provider, but I think that's a better implementation. If you don't like this idea I can work on the registerProvider return idea, it felt like a lot of code would need to change to surface the signal to that interface and keep it up to date.

I have a general idea of an implementation for the registerProvider return and a pretty detailed one for the signal proxy implementation. I could most likely have a PR for the latter this week, the former would have to wait a bit for my bandwidth to free up to work on it.

@JasonWeill JasonWeill removed the status:Needs Triage Applied to new issues that need triage label May 7, 2024
@JasonWeill JasonWeill mentioned this issue May 7, 2024
Open
@ajbozarth ajbozarth mentioned this issue May 9, 2024
Merged
@ajbozarth ajbozarth self-assigned this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ajbozarth ajbozarth

Labels
enhancement pkg:completer
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add a signal to proxy all Completer.selected signals ajbozarth/jupyterlab
3 participants
@krassowski @ajbozarth @JasonWeill