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

Replace perfectbound logic with sigsize = 1 #85

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

acestronautical
Copy link
Collaborator

It seems like we might be able to remove all the perfect bound logic, and instead treat perfectbound as signature size of 1.

This would simply the codebase if true.

Copy link

github-actions bot commented Feb 5, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-02-12 05:47 UTC

@acestronautical
Copy link
Collaborator Author

Testing this against main the PDFs generating for perfectbound seem identical.

@acestronautical acestronautical changed the title WIP: replace perfectbound logic with sigsize = 1 Replace perfectbound logic with sigsize = 1 Feb 6, 2024
@acestronautical
Copy link
Collaborator Author

acestronautical commented Feb 9, 2024

I rebased this and fixed the problem where it would allow generating signature files. if perfectbound or booklet is selected then only the aggregate file is generated.

@momijizukamori
Copy link
Owner

Two relatively small things:

  1. when the setting is 'perfectbound', we should probably not display signature breakdown info, particularly because for longer docs that's going to get messy fast (current main branch implementation displays 'N/A' for perfectbound)
  2. Can we move and rename the tests, rather than deleting them? May need one or two other tweaks to work with the new flow, but I don't think that should be hard.

@acestronautical
Copy link
Collaborator Author

I can rewrite the test but I would prefer to do that in a different PR since they will basically be new tests.
Making the signature breakdown not display is a little tricky, even for very long books the list doesn't become unusably long, it's just a little ugly. I would prefer to push this as is and do the cleanup later if that's ok since this whole thing is about improving the codebase. Just not sure I have the time at the moment to do everything.

@momijizukamori
Copy link
Owner

Yeah that's fair - could you make placeholder issues for those two things, and I will approve this? I am a bit scatterbrained this weekend unfortunately, lol

@acestronautical
Copy link
Collaborator Author

Filed:
#91
#92

😄 📖

Copy link
Owner

@momijizukamori momijizukamori left a comment

Choose a reason for hiding this comment

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

Thanks!

@acestronautical acestronautical merged commit fee1159 into main Feb 12, 2024
3 checks passed
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