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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSV fragment #5727

Merged
merged 8 commits into from Jan 11, 2019
Merged

CSV fragment #5727

merged 8 commits into from Jan 11, 2019

Conversation

@lheagy
Copy link
Contributor

@lheagy lheagy commented Dec 5, 2018

Fixes #5720

Implement a CSV setFragment function that scrolls to the requested row of a csv file.

sfqbu3vik9

Currently only supports row requests (notcol or cell, https://tools.ietf.org/html/rfc7111#section-3)

TODO / Question:

If the CSV file is already open, this does not update which row is at the top, so I think I am missing a refresh or update step... Any pointers?

nj4nxaznkt

Thanks!

Thanks @ian-r-rose, @jasongrout, @ellisonbg, @AlbertHilb for patiently answering my questions 馃槃

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Dec 10, 2018

It looks to me like the gotoLine functionality in the CSVViewer widget is not correct. @jasongrout can you take a look to confirm when you get the chance?

this.context.ready.then(() => {
this.content.goToLine(Number(topRow));
});
this.update();
Copy link
Member

@afshin afshin Dec 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this.update() probably belongs inside your then(...) function since that is done asynchronously.

Copy link
Contributor Author

@lheagy lheagy Dec 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @afshin: I was actually wondering if it is needed at all?

Copy link
Member

@ian-r-rose ian-r-rose Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is necessary at all, since the scrolling/viewport logic does not happen in the onUpdateRequest handler.

Copy link
Member

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

This looks goods pretty good to me @lheagy, sorry for the slow turnaround (got pretty behind during the holidays).

The main thing we need to do to make this useful is fix the bug in goToLine, which was not your fault, but does hamper this PR.

this.context.ready.then(() => {
this.content.goToLine(Number(topRow));
});
this.update();
Copy link
Member

@ian-r-rose ian-r-rose Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is necessary at all, since the scrolling/viewport logic does not happen in the onUpdateRequest handler.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 8, 2019

goToLine currently reads as

  goToLine(lineNumber: number) {
    let scrollY = this._grid.scrollY;
    /* The lines might not all have uniform height, so we can't just scroll to lineNumber * this._grid.baseRowSize
    see https://github.com/jupyterlab/jupyterlab/pull/5523#issuecomment-432621391 for discussions around
    this. It would be nice if DataGrid had a method to scroll to cell, which could be implemented more efficiently
    because datagrid knows more about the shape of the cells. */
    for (let i = scrollY; i < lineNumber - 1; i++) {
      scrollY += this._grid.sectionSize('row', i);
    }
    this._grid.scrollTo(this._grid.scrollX, scrollY);
}

Unless I am missing something, this is basically guaranteed to be wrong unless this._grid.scrollY === 0.

Instead, I think it should read something like

  goToLine(lineNumber: number) {
    let scrollY = 0;
    /* The lines might not all have uniform height, so we can't just scroll to lineNumber * this._grid.baseRowSize
    see https://github.com/jupyterlab/jupyterlab/pull/5523#issuecomment-432621391 for discussions around
    this. It would be nice if DataGrid had a method to scroll to cell, which could be implemented more efficiently
    because datagrid knows more about the shape of the cells. */
    for (let i = 0; i < lineNumber - 1; i++) {
      scrollY += this._grid.sectionSize('row', i);
    }
    this._grid.scrollTo(this._grid.scrollX, scrollY);
}

The big, honking warning about this method not being very efficient is still correct, but can be fixed another day.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 8, 2019

Pinging @jasongrout again for comment.

@lheagy
Copy link
Contributor Author

@lheagy lheagy commented Jan 9, 2019

Thanks @ian-r-rose, and no worries on the delay - it is important to take a break over the holidays! I pushed a change that removes the unnecessary update.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 10, 2019

@lheagy what do you think about updating goToLine?

@lheagy
Copy link
Contributor Author

@lheagy lheagy commented Jan 10, 2019

Sure, I can take a crack at it :)

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 10, 2019

Thanks!

@lheagy
Copy link
Contributor Author

@lheagy lheagy commented Jan 11, 2019

goToLine currently reads as

  goToLine(lineNumber: number) {
    let scrollY = this._grid.scrollY;
    /* The lines might not all have uniform height, so we can't just scroll to lineNumber * this._grid.baseRowSize
    see https://github.com/jupyterlab/jupyterlab/pull/5523#issuecomment-432621391 for discussions around
    this. It would be nice if DataGrid had a method to scroll to cell, which could be implemented more efficiently
    because datagrid knows more about the shape of the cells. */
    for (let i = scrollY; i < lineNumber - 1; i++) {
      scrollY += this._grid.sectionSize('row', i);
    }
    this._grid.scrollTo(this._grid.scrollX, scrollY);
}

Unless I am missing something, this is basically guaranteed to be wrong unless this._grid.scrollY === 0.

Instead, I think it should read something like

  goToLine(lineNumber: number) {
    let scrollY = 0;
    /* The lines might not all have uniform height, so we can't just scroll to lineNumber * this._grid.baseRowSize
    see https://github.com/jupyterlab/jupyterlab/pull/5523#issuecomment-432621391 for discussions around
    this. It would be nice if DataGrid had a method to scroll to cell, which could be implemented more efficiently
    because datagrid knows more about the shape of the cells. */
    for (let i = 0; i < lineNumber - 1; i++) {
      scrollY += this._grid.sectionSize('row', i);
    }
    this._grid.scrollTo(this._grid.scrollX, scrollY);
}

The big, honking warning about this method not being very efficient is still correct, but can be fixed another day.

Thanks @ian-r-rose: this resolved it!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 11, 2019

Awesome, thanks @lheagy!

Copy link
Member

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

Looks great to me, thanks @lheagy

@ian-r-rose ian-r-rose added this to the 1.0 milestone Jan 11, 2019
@ian-r-rose ian-r-rose merged commit 1f1e63a into jupyterlab:master Jan 11, 2019
2 of 3 checks passed
@lheagy lheagy deleted the csv-fragment branch Jan 11, 2019
@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.

3 participants