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.

Sign up for GitHub

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

Jump to bottom

Resize column to fit text on double click #546

Merged
merged 1 commit into from
Jul 17, 2023
Merged

Resize column to fit text on double click #546

merged 1 commit into from
Jul 17, 2023

Conversation

vthemelis
Copy link
Contributor

@vthemelis vthemelis commented Feb 24, 2023 •
edited
Loading

Implements: #535 and jupyter-widgets/ipydatagrid#302

Automatically resizes the width of columns by double clicking on the barriers between column headers.

@welcome
Copy link

welcome bot commented Feb 24, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@krassowski krassowski added the enhancement New feature or request label Feb 24, 2023
vthemelis
vthemelis commented Feb 24, 2023
View reviewed changes
packages/datagrid/src/datagrid.ts Outdated
const gc = this.canvasGC;
gc.font = CellRenderer.resolveOption(renderer.font, config);
const textWidth = gc.measureText(cellValue).width;
const textWidth = this._getCellTextWidth(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB that this does not translate to the same code as the original.

Initially, the cellValue was procured from the corner-header, however, the config would refer to the column-header area. I guess that's a bug?

@krassowski
Copy link
Member

krassowski commented Feb 24, 2023 •
edited
Loading

Thanks for starting this!

It's slow --It will kill your browser if you try it on a large column.

We could have a dedicated code path for very large columns (say > 1000 rows) with a heuristic. It could find e.g. 100 cells with longest text (by character count) and then only measureText width of these and take the maximum.

vthemelis
vthemelis commented Feb 24, 2023
View reviewed changes
packages/datagrid/src/datagrid.ts
DataGrid._getConfig(dataModel, idx, index, 'body')
);

// Heuristic: Sort by the length of the text to render and only fully calculate the text width
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can decrease the number of selected rows or try using a heap/quickselect for getting the top-N elements instead if this is not adequate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: MinHeap won't really help with performance. The main issue seems to be that getting the values of the cells is extremely slow. This is possibly accounted by the fact that the data is stored in a row-major form:

lumino/packages/datagrid/src/jsonmodel.ts

Line 83 in 9c4933f

value = this._data[row][field.name];

I think that what I'm doing here is probably good-enough for now but can change it if you think that this is likely not adequate.

@vthemelis
Copy link
Contributor Author

Issues I've looked into:

  • It's inaccurate --I'm reusing existing code to find the length of the text of a cell but it doesn't seem correct.
  • It's slow --It will kill your browser if you try it on a large column.
  • The code is a bit untidy but will fix that once we establish a direction.

@vthemelis vthemelis marked this pull request as ready for review February 25, 2023 14:14
@vthemelis
Copy link
Contributor Author

vthemelis commented Feb 25, 2023 •
edited
Loading

An issue that I'm seeing on the side is that dblclick event seem to get dropped sometimes completely.

I don't think that this is an issue with my code as I observe the same behaviour in main. This prohibits the smooth function of the existing edit functionality that also relies on double-clicking cells.

Opened #547 for this.

@vthemelis vthemelis mentioned this pull request Feb 25, 2023
Closed
@vthemelis vthemelis mentioned this pull request Feb 28, 2023
Closed
@vthemelis
Copy link
Contributor Author

Hi @krassowski,

Would you be able to have another look at this PR?

@vthemelis
Copy link
Contributor Author

The CI seems a bit flakey at the moment. Do you know how I can rerun one of the builds?

@krassowski
Copy link
Member

I kicked the CI, will look at all the PRs by Monday.

krassowski
krassowski reviewed May 12, 2023
View reviewed changes
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Heads-up: we would like to gradually add tests for any APIs changed in datagrid (which will make reviewing easier and merging PR faster). I had started this in #585 and once it is in we could add a simple test for the APIs modified here too - I can help with this.

packages/datagrid/src/textrenderer.ts Outdated Show resolved Hide resolved
packages/datagrid/src/datagrid.ts Outdated Show resolved Hide resolved
@alvinsga
Copy link

Hi, can I get an update on this?

@fcollonval
Copy link
Member

@vthemelis friendly ping - do you have time to finish this one up?

@vthemelis
Copy link
Contributor Author

@vthemelis friendly ping - do you have time to finish this one up?

Yes, will look into it this weekend

@vthemelis
Resize columns to fit on double-click at their boundaries
0f2f087 
Signed-off-by: Vasilis Themelis <vdthemelis@gmail.com>
@vthemelis
Copy link
Contributor Author

Heads-up: we would like to gradually add tests for any APIs changed in datagrid (which will make reviewing easier and merging PR faster). I had started this in #585 and once it is in we could add a simple test for the APIs modified here too - I can help with this.

@krassowski, I made the changes suggested by your comments but I'm not sure how to best test my changes. The setup seems a bit too involved and I'm not very familiar with UI testing.

Would you mind merging without tests for now?

fcollonval
fcollonval approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @vthemelis

@krassowski would you have time to help with testing this or is it ok to move forward?

@krassowski
Copy link
Member

Let's move forward and add test in a follow up PR

@fcollonval fcollonval merged commit c639d9a into jupyterlab:main Jul 17, 2023
@welcome
Copy link

welcome bot commented Jul 17, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@vthemelis vthemelis deleted the resize-col-on-double-click branch July 23, 2023 13:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@krassowski krassowski krassowski left review comments

@fcollonval fcollonval fcollonval approved these changes

Assignees

@vthemelis vthemelis

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vthemelis @krassowski @alvinsga @fcollonval