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

Add "Go to Line" and "Find" capabilities to csvviewer #5523

Merged

Conversation

@perrinjerome
Copy link
Member

@perrinjerome perrinjerome commented Oct 23, 2018

Go to Line

jupyterlab-csvviewer-go-to-line

Find

jupyterlab-csvviewer-find

Some notes about find implementation:

  • So that we can configure the colours for "cell matching search" and "current cell matching search", I had to refactor a bit to replace the TextRenders created for the theme by some TextRenderConfig that would allow the search service to build a TextRenderer which style the matching cells accordingly.
  • added a unit test faking the cell renderer service to check that the cell matching search text are highlighted.

At the same time, I add a fixup commit for a mistake in #5377

find method does not exist for GoToLine
- introduce a `GridSearchService` to keep the state of incremental
search and provide a function to highlight cells matching search text.
- replace light / dark theme TextRenderer by a new `TextRenderConfig`
class holding the config of cell rendering for different states.
@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 23, 2018

Thanks for this, @perrinjerome! I took an initial pass and had a couple of questions:

  • Do you mind making a Dialog.prompt() static method that uses your prompt widget? I can see using that a lot!
  • It looks like the scroll position logic might not work if the rows are not all uniform height, or am I mistaken?

@perrinjerome
Copy link
Member Author

@perrinjerome perrinjerome commented Oct 24, 2018

Thanks for feedback @blink1073

  • Do you mind making a Dialog.prompt() static method that uses your prompt widget? I can see using that a lot!

Good idea. I also felt that such a prompt widget would be a nice to have utility. I'll try to take care of this in the next few days.

  • It looks like the scroll position logic might not work if the rows are not all uniform height, or am I mistaken?

Ah no, it would not work ... but as far as I see csvviewer cells currently always have all same height.

I tried everything I could think of to get cells of different heights and came up with this csv:

Column A,Colum B
"CELL WITH LOTS OF DATA
CELL WITH LOTS OF DATA
CELL WITH LOTS OF DATA 
CELL WITH LOTS OF DATA",b
a,b
a,b

but it still looks like this:

image

So I'm wondering if there a way to have cells with variable height with csvviewer.

Anyway, we can already write this in a way that would still work in this case. Do you think it means we have to take into account the sectionSize of each row ? something like:

  /**
   * Go to line
   */
  goToLine(lineNumber: number) {
    let scrollY = this._grid.scrollY;
    for (let i = scrollY; i < lineNumber - 1; i++) {
      scrollY += this._grid.sectionSize('row', i);
    }
    this._grid.scrollTo(this._grid.scrollX, scrollY);
  }

This makes me feel datagrid widget needs an API to do this, also because if this implementation was on DataGrid, DataGrid could take decisions based on the knowledge it has on the data and cell shapes ( for example it should know if all cells have same height and use the same this._grid.baseRowSize * (lineNumber - 1) approach as this first implementation ). I noticed this was suggested in phosphorjs/phosphor#285 (comment) .

As a first step, I can update this implementation using this sectionSize approach.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 24, 2018

Ah, good point regarding the line location. Mind adding an inline TODO comment pointing to that comment?

@blink1073 blink1073 added this to the 1.0 milestone Oct 26, 2018
@perrinjerome
Copy link
Member Author

@perrinjerome perrinjerome commented Oct 30, 2018

@blink1073 I think I implemented the changes as we discussed. Thanks again for comments.

I made a Dialog.prompt and simplified a bit, the signature is simple Dialog.prompt( prompt: string, defaultValue: string | number | boolean) and the input widget depend on the type. There's no longer the possibility to use different button text or dialog title, but I guess it's not needed in this context, when more control is needed, it's always possible to use showDialog .

image

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 30, 2018

Looks great, thanks again! Linter caught a few things:

ERROR: packages/apputils/src/dialog.ts[730, 24]: == should be ===
ERROR: packages/apputils/src/dialog.ts[736, 24]: == should be ===
ERROR: packages/apputils/src/dialog.ts[765, 31]: == should be ===
ERROR: packages/csvviewer/src/widget.ts[277, 105]: trailing whitespace

@perrinjerome
Copy link
Member Author

@perrinjerome perrinjerome commented Oct 30, 2018

Oh sorry for that, I'll update this soon.

@perrinjerome perrinjerome force-pushed the feat/csv-viewer-find-go-to-line branch from 6ed4def to b7e6bb7 Oct 30, 2018
@perrinjerome
Copy link
Member Author

@perrinjerome perrinjerome commented Oct 30, 2018

I updated (by amending the commits - I hope this way is OK), this time linter should be OK. Thanks !

@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 30, 2018

LGTM, thanks!

@blink1073 blink1073 merged commit a64f619 into jupyterlab:master Oct 30, 2018
1 of 2 checks passed
@perrinjerome perrinjerome deleted the feat/csv-viewer-find-go-to-line branch Oct 30, 2018
@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