Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

CSV fragment#5727

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ian-r-rose merged 8 commits intojupyterlab:masterfromlheagy:csv-fragment
Jan 11, 2019
Merged

CSV fragment#5727

ian-r-rose merged 8 commits intojupyterlab:masterfromlheagy:csv-fragment
Jan 11, 2019

Conversation

@lheagy
Copy link
Contributor

Fixes#5720

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

sfqbu3vik9

Currently only supportsrow requests (notcol orcell,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

It looks to me like thegotoLine 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

Choose a reason for hiding this comment

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

I thinkthis.update() probably belongs inside yourthen(...) function since that is done asynchronously.

Copy link
ContributorAuthor

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

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 theonUpdateRequest handler.

lheagy reacted with thumbs up emoji
Copy link
Member

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

Choose a reason for hiding this comment

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

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 ingoToLine, which was not your fault, but does hamper this PR.

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

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 theonUpdateRequest handler.

lheagy reacted with thumbs up emoji
@ian-r-rose
Copy link
Member

goToLine currently reads as

goToLine(lineNumber: number){letscrollY=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(leti=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 unlessthis._grid.scrollY === 0.

Instead, I think it should read something like

goToLine(lineNumber: number){letscrollY=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(leti=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.

lheagy reacted with thumbs up emoji

@ian-r-rose
Copy link
Member

Pinging@jasongrout again for comment.

@lheagy
Copy link
ContributorAuthor

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

@lheagy what do you think about updatinggoToLine?

lheagy reacted with thumbs up emoji

@lheagy
Copy link
ContributorAuthor

Sure, I can take a crack at it :)

@ian-r-rose
Copy link
Member

Thanks!

@lheagy
Copy link
ContributorAuthor

goToLine currently reads as

goToLine(lineNumber: number){letscrollY=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(leti=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 unlessthis._grid.scrollY === 0.

Instead, I think it should read something like

goToLine(lineNumber: number){letscrollY=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(leti=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

Awesome, thanks@lheagy!

Copy link
Member

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

Choose a reason for hiding this comment

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

Looks great to me, thanks@lheagy

lheagy reacted with hooray emoji
@ian-r-roseian-r-rose added this to the1.0 milestoneJan 11, 2019
@ian-r-roseian-r-rose merged commit1f1e63a intojupyterlab:masterJan 11, 2019
@lheagylheagy deleted the csv-fragment branchJanuary 11, 2019 20:11
@locklockbot added the status:resolved-lockedClosed issues are locked after 30 days inactivity. Please open a new issue for related discussion. labelAug 8, 2019
@locklockbot locked asresolvedand limited conversation to collaboratorsAug 8, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@afshinafshinafshin left review comments

+1 more reviewer

@ian-r-roseian-r-roseian-r-rose approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancementpkg:csvviewerstatus:resolved-lockedClosed issues are locked after 30 days inactivity. Please open a new issue for related discussion.

Projects

None yet

Milestone

1.0

Development

Successfully merging this pull request may close these issues.

Add fragment handler for CSVs

3 participants

@lheagy@ian-r-rose@afshin
[8]ページ先頭
©2009-2025 Movatter.jp