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

added repeatable header for overflow tables #166

Merged

Conversation

Chemmic
Copy link

@Chemmic Chemmic commented Apr 14, 2024

Hey,

I've added a new class called "OverflowOnSamePageRepeatableHeaderTableDrawer". The name is kinda long, maybe it should be changed.

I kinda mixed @vandeseer's "RepeatedHeaderTableDrawer" and "OverflowOnSamePageTableDrawer" together.
It allows tables which overflows on the same page to have repeatable headers as well.

PS: the table starting at a different y position was my mistake and no bug. I misunderstood how the offset worked :)

@vandeseer vandeseer changed the base branch from master to develop April 16, 2024 19:31
Copy link
Owner

@vandeseer vandeseer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! However, I think it needs a little ❤️ still. As soon as the test passes (and the PDF itself is readable correctly), I'm happy to merge. (We can also check then if we can easily refactor the code to reduce the duplication, but I think it's best to start like this and improve only afterwards.)

import de.redsix.pdfcompare.CompareResult;
import de.redsix.pdfcompare.PdfComparator;

public class OverflowOnSamePageRepeatableHeaderTableDrawerTest {
Copy link
Owner

Choose a reason for hiding this comment

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

This class doesn't test anything yet.
Interestingly I also can't open the new PDF file.

Essentially it's missing the comparing part, like this:

final CompareResult compareResult = new PdfComparator<>(
                getExpectedPdfFor(OVERFLOW_ON_SAME_PAGE_PDF),
                getActualPdfFor(OVERFLOW_ON_SAME_PAGE_PDF)
        ).compare();

        assertTrue(compareResult.isEqual());

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if you mean something else, but I'm comparing the pdfs at line 45 :)

Copy link
Owner

Choose a reason for hiding this comment

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

Indeed! My bad, I simply overlooked it. Sorry!

@Chemmic
Copy link
Author

Chemmic commented Apr 16, 2024

Hey,
Thank you for the review!
I don't know what happened exactly, but I couldn't open the PDF as well, it was corrupted somehow.
However, I uploaded a new PDF (working for me now at least)

  • Matteo

@vandeseer
Copy link
Owner

Hey, Thank you for the review! I don't know what happened exactly, but I couldn't open the PDF as well, it was corrupted somehow. However, I uploaded a new PDF (working for me now at least)

  • Matteo

Hey Matteo,

the new PDF didn't work for me either, but that's not a big thing: I could produce it by running the test and that one worked correctly. I will simply replace it after I merged this PR.

Thanks again for your contribution!
Let me know if you're interested in helping me maintaining this library – I'm open for it, since I don't want the project to die, but I really lack time to properly maintain it myself ...

Best,
Stefan

Copy link
Owner

@vandeseer vandeseer left a comment

Choose a reason for hiding this comment

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

I will release after the merge (I hope the release process still works as in the past!)

@vandeseer vandeseer merged commit 33384df into vandeseer:develop Apr 17, 2024
@Chemmic
Copy link
Author

Chemmic commented Apr 18, 2024

Hey, Thank you for the review! I don't know what happened exactly, but I couldn't open the PDF as well, it was corrupted somehow. However, I uploaded a new PDF (working for me now at least)

  • Matteo

Hey Matteo,

the new PDF didn't work for me either, but that's not a big thing: I could produce it by running the test and that one worked correctly. I will simply replace it after I merged this PR.

Thanks again for your contribution! Let me know if you're interested in helping me maintaining this library – I'm open for it, since I don't want the project to die, but I really lack time to properly maintain it myself ...

Best, Stefan

Hey Stefan,

Sure, I'd be glad to help if I can!
I'm currently working on my computer science bachelor's thesis, so I'll also be somewhat limited in time over the next few months as well.
Feel free to contact me at Matteo.staar@gmx.de if there's anything that needs clarification or if I need to know something, like how the release process works or something like that :)

Best, Matteo

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.

2 participants