Document Search Feedback and Updates

[afshin]

JupyterLab now has a new document search architecture which allows for extension authors to create search providers for different types of widgets inside Lab. This is great news. Thank you, @aschlaep! The feature was released in the latest 1.0 pre-release.

Below is some feedback and suggestions for iterating on the API and user experience of the search:

• The Accel F keyboard shortcut that instantiates a search request targets the body selector. This is too broad because in cases where there is no search provider, the command still short-circuits the native browser search functionality. To see this in action, you can open a Markdown file in preview mode and invoke search. A way of dealing with this issue is to have the extension that registers a search provider also listen to the instance tracker for its widget type and add a class to each widget that is added to the tracker. By doing this, we can target that class, e.g. jp-mod-searchable
• The concept of default vs. custom search providers might be unnecessary. I would propose having just a flat set of search providers and moving the registration of the two default search providers into the documentsearch-extension instead of having them added in the constructor of the registry. By doing this, no providers have a "special" status compared to any others and it simplifies the registry a little.
• Currently any extension can deregister an active search provider that it did not add. I would propose modifying this to use the pattern we use elsewhere by having the register() method return an IDisposable (a DisposableDelegate) instance that can be used to remove the provider if the author of the provider wishes, without exposing de-registration to everyone else.
• When a search instance is instantiated, it is passed a brand new instance of the relevant search provider. Is that provider disposed after the search life cycle has ended? There may be a memory leak here.

[aschlaep]

Thanks @afshin! I'll get working on the first and third points first, as they seem like the most important to get in before 1.0, then I'll tackle the other two.

[ellisonbg]

Thanks, this is helpful! Excited to see this work.

[jasongrout]

I'm not sure how to assign this issue to @aschlaep. I tried adding him as a collaborator to the repo, but that didn't put his name in the list of people we can assign issues to. Anyone know how to do this?

[jasongrout]

(and there are lots of people listed in the "assign" list that aren't listed as GitHub collaborators, so I'm not sure how that list gets populated)

[afshin]

@jasongrout I was confused, too.

[afshin]

@aschlaep Is it possible that the email address you commit with is different from the email address that your GitHub account login is? I think I had an issue like that and I needed to add a couple email addresses in my GitHub settings before my account worked correctly. Right now, your commits do not show your user thumbnail, so I think GitHub doesn't associate them correctly to your account. But that's just my guess.

[aschlaep]

@afshin I think that was the issue, I was definitely committing with a different email address than the one I had on my github profile. Just added it and it looks like it might have fixed it - I see my profile image next to my commits now

[blink1073]

Looks good, assigned to @aschlaep!

[aschlaep]

Just to keep track:

• Restrict selector scope of the shortcuts from the body to a searchable class (Change documentsearch shortcut selector to searchable class #6237)
• Remove deregister from ISearchProvider and instead return a DisposableDelegate from register (Remove deregisterProvider from SearchProviderRegistry interface #6282)
• Remove notion of default search providers, instead register the "default" ones in the documentsearch package in documentsearch-extension (Remove notion of default search providers #6301)
• Check on SearchInstance cleanup for potential memory leaks

[aschlaep]

Per @jasongrout's suggestion, I'm going to close this out for now and open a new issue for the SearchInstance memory leak investigation on 1.1. I've done a bit of investigation and haven't found much in the way of a leak yet, but more work is needed to confirm.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.