Skip to content

Fix footer height calculation#252

Merged
philprime merged 2 commits into
techprimate:developfrom
lpeancovschi:fix/footer_calculation
Dec 5, 2020
Merged

Fix footer height calculation#252
philprime merged 2 commits into
techprimate:developfrom
lpeancovschi:fix/footer_calculation

Conversation

@lpeancovschi
Copy link
Copy Markdown
Contributor

Fixes footer height calculation.

Without this fix footer height is equal to header height. That means that if you don't have a header but do have a footer your content will overlap the footer because header height is zero.

@philprime
Copy link
Copy Markdown
Member

Hi, thanks for your contribution.

After a quick look I need you to provide an example to reproduce the issue, as your fix seems wrong:

The method you changed calculates the distance from the top edge, therefore it is paper_height - footer_height.

I could be wrong, that's why an example would be great

@lpeancovschi
Copy link
Copy Markdown
Contributor Author

lpeancovschi commented Dec 1, 2020

Hello @philprime,

Thanks for looking at this! The issue is reproducible when you have a table that is rendered over a footer. To reproduce it you can paste this method to the HeaderFooterExampleFactory.swift:

func generateDocument() -> [PDFDocument] {
    let document = PDFDocument(format: .a4)

    // footer will be offset by the height that is equal to the header height
    document.add(.headerRight, textObject: PDFSimpleText(text: "Header right 1"))

    document.add(.footerLeft, textObject: PDFSimpleText(text: "Footer Left 1"))
    document.add(.footerLeft, textObject: PDFSimpleText(text: "Footer Left 2"))
    document.add(.footerLeft, textObject: PDFSimpleText(text: "Footer Left 3"))
    document.add(.footerLeft, textObject: PDFSimpleText(text: "Footer Left 4"))

    var items = [[String?]]()

    for _ in 0..<200 {
        items.append(["Lorem Ipsum is simply dummy text of the printing and typesetting industry."])
    }

    let table = PDFTable(rows: items.count, columns: 1)

    table.content = items
    table.rows.allCellsStyle = PDFTableCellStyle(colors: (fill: .clear, text: .black))
    table.widths = [1]

    table.padding = 10.0

    document.add(table: table)

    return [document]
}

Here's how it looks without the fix:

no_fix

And this is with the fix:

Fix

@philprime
Copy link
Copy Markdown
Member

Thanks a lot for the example! Helps massively.
I will look into it a little further as I am still confused where my thought is wrong.

@philprime philprime self-assigned this Dec 2, 2020
@philprime philprime self-requested a review December 2, 2020 16:07
@philprime
Copy link
Copy Markdown
Member

I took a closer look, and you were right. The calculation itself doesn't make any sense and seems to be a copy paste error. Anyways I added a suggestion so the method includes all paddings, spacings etc.

If you approve my recommendation we can merge it into develop (I will change the branch)

@philprime philprime changed the base branch from master to develop December 4, 2020 18:22
@philprime philprime changed the base branch from develop to master December 4, 2020 18:26
@philprime philprime changed the base branch from master to develop December 4, 2020 18:26
Comment thread Source/Internal/Utils/PDFCalculations.swift
@philprime
Copy link
Copy Markdown
Member

Can you please apply my suggested change and update the PR? I will merge it afterwards and release an update

@philprime
Copy link
Copy Markdown
Member

Relates to #249

@lpeancovschi
Copy link
Copy Markdown
Contributor Author

Amazing, thanks for reviewing this PR!

@lpeancovschi
Copy link
Copy Markdown
Contributor Author

Don't know why the code analysis is failing. Codacy doesn't provide much context.

@lpeancovschi
Copy link
Copy Markdown
Contributor Author

lpeancovschi commented Dec 5, 2020

@philprime are you planning to release #223 too?

@philprime
Copy link
Copy Markdown
Member

Don't worrry about Codacy, looks like some project linking issue. They changed their GitHub integration a little while ago so it might be broken.

Yes I am planning to release #223 too

@philprime philprime merged commit b93b642 into techprimate:develop Dec 5, 2020
@lpeancovschi lpeancovschi deleted the fix/footer_calculation branch December 5, 2020 17:37
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.

3 participants