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

Improvement: Precalculate the cell content #98

Merged

Conversation

edisonhello
Copy link
Contributor

  • Fix a bug that the cell's height is miscalculated when there's a left-right-padding setting and the word is getting separated.

Cause: In print_row_in_cell, the text is wrapped (by word_wrap) and split (by split_lines) too early before checking the padding settings.

  • Precalculate and cache the result of word_wrap and split_lines before calling print_row_in_cell to improve the printing complixity

Detail: In print_row_in_cell, word_wrap and split_lines will be called once for each cell each line they have. When the content of the cell and the number of lines are both large, the computation time will grow massively.

In this PR, the computation is moved before the actual printing procedure, so those functions won't get called multiple times for each cell, and also avoid the bug mentioned by using the correct line height in print_row_in_cell.

Further todo: the code in Row::get_cell_height is also similar to these, maybe we should reuse the code.

The example of the bug (the upper one is before, and the lower one is after):
image

Reproduce code:

Table table;

table.add_row({"abcde"});

table.format().width(10);
table.format().padding_left(4);
table.format().padding_right(4);

cout << table << endl;

Additional check for not breaking another sample:
image

I did a manual check for all the samples and hope there's no accident.

  • Apply clang-format for all the files changed in this PR.

@edisonhello edisonhello changed the title Cache splitted text Improvement: Precalculate the cell content Feb 8, 2023
@p-ranav p-ranav merged commit b5518f4 into p-ranav:master Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants