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

Fix the word column width which is larger like 22in (#1867) #1869

Merged

Conversation

speckyspooky
Copy link
Contributor

No description provided.

@speckyspooky speckyspooky added the BugFix Change to correct issues label Aug 21, 2024
@speckyspooky speckyspooky added this to the 4.17 milestone Aug 21, 2024
@speckyspooky speckyspooky self-assigned this Aug 21, 2024
Copy link
Contributor

@hvbtup hvbtup left a comment

Choose a reason for hiding this comment

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

Two suggestions:

When clipping the column widths, you use the Elvis operator ?:. When clipping the table width, you use Math.min. I think using Math.min in both places looks a bit cleaner.

My gut feeling is that the for loop with the comment "validate the maximum of column width", which clips the calculated column widths, is not needed, because the column widths and the table width have already been clipped before.
But I'm just looking at the code, I did not test it.
Maybe you could explain why the for loop is necessary in the comment.

@speckyspooky
Copy link
Contributor Author

The Elvis operator is my standard comparer for such situation for all data types therefore I used it.

The loop is necessary because if no column width is set the calculation will be done before the loop.
If the column width is set the first check will bei used but the return is earlier.
The reason for the 2 checks at the end is the 2 returns and the different calculation methods.

@speckyspooky speckyspooky merged commit cad5146 into eclipse-birt:master Aug 22, 2024
3 checks passed
@hvbtup
Copy link
Contributor

hvbtup commented Aug 23, 2024

OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugFix Change to correct issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants