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: PdfPTable missing line using setSplitLate #524

Closed
wants to merge 3 commits into from
Closed

fix: PdfPTable missing line using setSplitLate #524

wants to merge 3 commits into from

Conversation

whexy
Copy link

@whexy whexy commented Apr 23, 2021

Due to the fix for issue #258 on pull request #260, these lines of code in com.lowagie.text.pdf.PdfRow::splitRow have been changed from

float bottom = cell.getTop() + cell.getEffectivePaddingBottom() - newHeight;
float top = cell.getTop() - cell.getEffectivePaddingTop();

to

float top = cell.getTop() - cell.getEffectivePaddingTop();
float bottom = top + cell.getEffectivePaddingBottom() - newHeight;

However, the change is actually wrong, which introduced a bug found in issue #345.

I draw a figure to show the calculation process. To calculate the correct value of bottom, the original lines of code is correct.

To test the code, please use the code mentioned in issue #345.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sixdouglas
Copy link
Contributor

I agree with you. According to the drawing you made, the original lines are the good ones.
My question is, if we go back to the original lines, will the issue #258 reappear?
With your actual knowledge of this cell width/height calculation, do you have any idea how to fix both issues?
Thanks for your help

@asturio
Copy link
Member

asturio commented Apr 25, 2021

This should be correct, if newHeight is correct. Wouldn't something like this be better?:

float bottom = cell.getBottom() + cell.getEffectivePaddingBottom();

I don't know how to reproduce #258, any ideas?

@asturio
Copy link
Member

asturio commented May 2, 2021

I'm really reluctant with this PR. But I think it is better to write a line twice than having a line disappear at all. The PR is actually a revert of #260.
What do you think? @sixdouglas @PalAditya

@Wugengxian
Copy link

Wugengxian commented May 2, 2021

@asturio I reproduce #258 by the code:

void writePDF() throws FileNotFoundException, DocumentException {
        File file = new File("output.pdf");
        Document document = new Document(PageSize.A4);
        PdfWriter.getInstance(document, new FileOutputStream(file));

        document.open();

        PdfPTable table = new PdfPTable(1);
        table.setSplitLate(false);

        String a = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Morbi eu cursus purus, id efficitur arcu" +
                ". Pellentesque eu arcu elit. Phasellus in mattis diam. Donec in augue eget ligula consectetur" +
                " pretium. Maecenas scelerisque non massa ut faucibus.Nam sit amet malesuada nisl. Duis varius " +
                "vitae leo venenatis ullamcorper. Morbi at vestibulum tortor, vel blandit erat.";
        StringBuilder stringBuffer = new StringBuilder();
        for (int i = 0; i < 38; i++){
            stringBuffer.append(a).append("count").append(" ").append(i).append(" ");
        };
        table.addCell(stringBuffer.toString());
        document.add(table);
        document.close();
    }

the output:

image

version of OpenPDF:

1.3.8

if we accept this PR in newest version of OpenPDF, this issue does not happen. it seems that it is fixed by other PR. I think this PR should be merged in newest version to fix #345.

@whexy
Copy link
Author

whexy commented May 3, 2021

I got confused that I never successfully reproduced #258 (the duplication problem) by changing these two lines.

Thanks to @Wugengxian, it turns out that #258 is not related to this calculation mistake at all.

By the way, if you look at the comments in that issue, you will be confused, too. There was never a code test for the problem. We depended on someone who told us everything is fine.

I think the PR is safe now.

@sixdouglas
Copy link
Contributor

@asturio I'll take some time to play with this PR and this sample code to see if we can loose or double any line, or if these issues are both closed.
I'll let you know

@asturio asturio linked an issue May 13, 2021 that may be closed by this pull request
@whexy whexy closed this Feb 28, 2022
@bk1991
Copy link

bk1991 commented Apr 15, 2022

@whexy I am facing similar issue on OpenPDF1.3.26, after doing the change you mentioned the issue doesn't occur.
In which release your changes will be available ?

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.

Lines get lost on page break
5 participants