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

Implement FPDF.table() - close #701 & #723 #703

Merged
merged 17 commits into from
Mar 27, 2023
Merged

Implement FPDF.table() - close #701 & #723 #703

merged 17 commits into from
Mar 27, 2023

Conversation

Lucas-C
Copy link
Member

@Lucas-C Lucas-C commented Feb 20, 2023

Features
Work-in-progress implementation status:

  • support cells with content wrapping over several lines
  • control over column & row sizes, or by default let them be automatically computed
  • control table width
  • allow to set table headings, styled differently, but make this optional
  • handle splitting a table over page breaks, with headings repeated
  • control over cell background, through a callback function to allow maximum customization
  • control over text alignment in cells, with rules by column or row
  • allow to embed images in cells
  • control over borders: color, width & where they are drawn (e.g. allow to not draw the surrounding square, allow to only draw the horizontal line above the headings, etc.) Also: control thickness of border below headings
  • honor the initial X / Y current position to render the table, and allow to easily center it in the page
  • allow for several cells to be merged horizontally (aka colspan)
  • replace the table-building logic in fpdf/html.py by a call to this new FPDF.table() method

Also, do not forget:

@Lucas-C Lucas-C marked this pull request as draft February 20, 2023 22:15
@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Patch coverage: 94.02% and project coverage change: +0.01 🎉

Comparison is base (f371035) 93.18% compared to head (fbf4c39) 93.20%.

❗ Current head fbf4c39 differs from pull request most recent head 6b2d152. Consider uploading reports for the commit 6b2d152 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
+ Coverage   93.18%   93.20%   +0.01%     
==========================================
  Files          26       27       +1     
  Lines        7120     7252     +132     
  Branches     1272     1315      +43     
==========================================
+ Hits         6635     6759     +124     
- Misses        301      309       +8     
  Partials      184      184              
Impacted Files Coverage Δ
fpdf/image_parsing.py 83.62% <ø> (ø)
fpdf/html.py 92.92% <91.25%> (-1.82%) ⬇️
fpdf/table.py 91.75% <91.75%> (ø)
fpdf/enums.py 97.89% <100.00%> (+2.37%) ⬆️
fpdf/fonts.py 100.00% <100.00%> (ø)
fpdf/fpdf.py 92.40% <100.00%> (+0.56%) ⬆️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lucas-C Lucas-C self-assigned this Feb 20, 2023
@Lucas-C Lucas-C force-pushed the table branch 10 times, most recently from d21efd0 to bafe5c1 Compare February 24, 2023 11:21
@Lucas-C
Copy link
Member Author

Lucas-C commented Feb 24, 2023

Documentation preview: https://github.com/PyFPDF/fpdf2/blob/table/docs/Tables.md

@Lucas-C
Copy link
Member Author

Lucas-C commented Feb 24, 2023

This PR is almost ready.

Ping @gmischler, @eroux, @erap129, @Bubbu0129, @RedShy, @andersonhc, @NikhilExactSpace, @eabase, @JianDk, @RubendeBruin : you are all regular fpdf2 contributors, or fpdf2 users that expressed an interest for tables in the past.

I'd be very happy to have your feedback on this new method: are you OK with the behaviour of the FPDF.table() method added in this PR? Do you see any missing features? cf. https://github.com/PyFPDF/fpdf2/blob/table/docs/Tables.md

You can test the code by installing fpdf2 from the table branch of this repo:

pip install git+https://github.com/PyFPDF/fpdf2.git@table

I'm mainly looking for feedback on the API, but code reviews are also very welcome 😊

@eroux
Copy link

eroux commented Feb 24, 2023

thanks, impressive PR! I must say I'm almost only focused on images for my project (as you probably noticed!) and I've never really used that kind of API (except in TeX an eternity ago... for this project), so I don't feel I have very insightful or relevant comments

@gmischler
Copy link
Collaborator

That's what I get for letting the topic sit for so long and not turn it into a draft PR from the start...

In TextRegion, I have started to implement my ideas as outlined in #339.
This is a much more general solution for organizing flowing text, which is planned to support tables as well as other columnar or arbitrarily shaped outlines. In addition to all the features listed above, it will also allow for text fragments of different fonts/sizes to be combined and align all of them together in one text block.

As far as I have seen yet, your approach is not fundamentally different from mine, at least on the surface, so it might be possible to integrate the two with reasonable effort.

Sorry to barge in so large in your progress, but I think a more general approach to text layout is much more useful in the long run. So I'd recommend to keep this on hold for the moment. To the very least we should try to come up with a common API strategy and naming convention, so that integrating the two approaches later doesn't unnecessarily break backwards compatibility.

I'll turn my own code into a PR asap, so we can compare the concepts next to each other and others can chime in as well.

@Lucas-C
Copy link
Member Author

Lucas-C commented Feb 24, 2023

thanks, impressive PR! I must say I'm almost only focused on images for my project (as you probably noticed!) and I've never really used that kind of API (except in TeX an eternity ago... for this project), so I don't feel I have very insightful or relevant comments

Alright, no worries @eroux, thanks for taking the time to post this comment 😊

@Lucas-C
Copy link
Member Author

Lucas-C commented Feb 24, 2023

I'll turn my own code into a PR asap, so we can compare the concepts next to each other and others can chime in as well.

Alright @gmischler, I can wait for your PR for a few weeks before merging this 😊

The table() implementation in this PR relies mainly on the following FPDF methods:

  • multi_cell() and almost all of its parameters: w h, txt, border, align, fill, max_line_height, new_x, new_y, split_only
  • use_font_style()
  • preload_image()

In what way does your work in TextRegion would have an impact?
I read https://github.com/gmischler/fpdf2/blob/TextRegion/docs/TextRegion.md, but I don't seen how uing TextRegion would be better than multi_cell(). Or do you plan for multi_cell() to be based on TextRegion?

Also, I think your work does not question the addition of a FPDF.table() method, so I still would be curious to know your opinion on https://github.com/PyFPDF/fpdf2/blob/table/docs/Tables.md @gmischler

@RubendeBruin
Copy link

Thanks for the ping. Impressive piece of work! The current API completely covers my current use and I really like the possibility of adding images.

@Lucas-C Lucas-C marked this pull request as ready for review February 24, 2023 21:28
@Lucas-C Lucas-C force-pushed the table branch 15 times, most recently from 720cecc to 6b2d152 Compare March 27, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants