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

Convert CSV to use document searcher #5937

Merged
merged 12 commits into from Feb 4, 2019
Merged

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 2, 2019

Also, remove the Edit Find menu delegator, preferring instead the document search UX.

See #5523 where the CSV find functionality was originally implemented.

@jasongrout jasongrout added this to the 1.0 milestone Feb 2, 2019
@jasongrout jasongrout requested a review from ian-r-rose Feb 2, 2019
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 2, 2019

Turns out this was a little harder than we thought, because we forgot to actually provide a search provider registry to the system! So I corrected that as well, so this was a really good test case of something extending the search system.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 2, 2019

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @jasongrout. I have a few comments/questions, some about the CSV search, and some about the general API and structuring of the search extension.

Two main structure questions:

  1. Why does this not use the */*-extension pattern? I know it can be a bit annoying to split them out, but it is kind of out-of-step with the rest of the monorepo structure.
  2. In a number of places there are instanceof checks and any typings. Those make me nervous, and I wonder if we could tighten them up a bit.

packages/csvviewer-extension/package.json Show resolved Hide resolved
* @param widget - The widget to search over.
* @returns the search provider, or undefined if none exists.
*/
getProviderForWidget(widget: any): ISearchProvider | undefined;
Copy link
Member

@ian-r-rose ian-r-rose Feb 2, 2019

Why is this typed as any? The name seems to suggest Widget

} else if (domain) {
this._cm = domain.content.editor;
async startQuery(query: RegExp, searchTarget: any): Promise<ISearchMatch[]> {
if (searchTarget instanceof CodeMirrorEditor) {
Copy link
Member

@ian-r-rose ian-r-rose Feb 2, 2019

It's not really the point of this PR, but this logic looks fragile to me.

scrollY += this._grid.sectionSize('row', i);
}
for (let j = scrollX; j < this._column - 1; j++) {
for (let j = scrollX; j < col - 1; j++) {
scrollX += this._grid.sectionSize('column', j);
}
this._grid.scrollTo(scrollX, scrollY);
Copy link
Member

@ian-r-rose ian-r-rose Feb 2, 2019

The scrolling into view doesn't seem to be working in my testing.

Copy link
Contributor Author

@jasongrout jasongrout Feb 2, 2019

Great, thanks for pointing it out.

// check to see if the CMSearchProvider can search on the
// first cell, false indicates another editor is present
return (
domain instanceof DocumentWidget && domain.content instanceof CSVViewer
Copy link
Member

@ian-r-rose ian-r-rose Feb 2, 2019

In other places in the codebase we would do a check on an InstanceTracker, which seems safer and more idiomatic. Is there a reason for the instanceof checks instead?

Copy link
Contributor Author

@jasongrout jasongrout Feb 2, 2019

The tracker won't work here because the csv viewer doesn't expose a tracker. Granted that's a problem with the csv viewer, but I was trying to limit scope here in this PR.

if (cellData.indexOf(searchText) !== -1) {
const increment = reverse ? -1 : 1;
this._column += increment;
for (
Copy link
Member

@ian-r-rose ian-r-rose Feb 2, 2019

How does this perform on a large CSV? Is there any danger of this hanging?

Copy link
Contributor Author

@jasongrout jasongrout Feb 2, 2019

Good question. I left the searching logic basically alone, so it probably performs on par with what it was before (with the exception that now I do a regex match instead of a string lookup).

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 2, 2019

@ian-r-rose - excellent questions. Our premise on this was that it is a first cut of the functionality, and there will be cleanup and iteration of this going forward before 1.0.

To the specific questions: we plan to refactor into two plugins, like we have elsewhere, and I also want to tighten up the typings like you mention.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Feb 3, 2019

WIP - I'm going ahead and splitting the package in this PR.

@jasongrout jasongrout changed the title Convert CSV to use document searcher WIP Convert CSV to use document searcher Feb 3, 2019
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 3, 2019

Sounds good @jasongrout. I think it's reasonable to get this out the door, then do some tightening later. Let me know when you think it's ready.

@jasongrout jasongrout changed the title WIP Convert CSV to use document searcher Convert CSV to use document searcher Feb 4, 2019
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Works great! Thanks for the additional restructuring @jasongrout.

@jasongrout jasongrout merged commit 7dccf49 into jupyterlab:master Feb 4, 2019
8 of 11 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants