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

Support Superscript, Subscript and Fractional positioning #520

Merged
merged 10 commits into from
Sep 8, 2022

Conversation

gmischler
Copy link
Collaborator

Fixes #298 & #514 about subscript/superscript
Fixes #261 about text overlap after HTML headings

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.
  • A unit test is covering the code added / modified by this PR
  • This PR is ready to be merged
  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder
  • A mention of the change is present in CHANGELOG.md

This was even more straightforward to implement than anticipated, since we can do all the size and height calculations already in the Fragment().
While researching the topics and related typographical conventions, I discovered that the nominator and denominators of fractional numbers around a "/" are another use case for the functionality. So now we have four sets of defaults for the scale and lift values of "SUP", "SUB", "NOM", and "DENOM".

Implementing support in HTML was pretty much trivial. Until I discovered that we apparently still haven't fixed #261. This PR would have fixed the problem that the nl() after a heading is not done with the height of the heading itself, so that the next and typically much smaller font line will overlap with the heading. The PR was closed last year because the submitter apparently didn't get around to update the affected test files.

I now went a step further, and gave each heading an extra space of 20% the font height above and below. This is good typography for headings, and also what the web browsers do (didn't check the exact numbers, which probably vary).
There's no extra regression test for this, as it reflects in several other tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Merging #520 (0936c0b) into master (356de1a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   93.97%   94.09%   +0.12%     
==========================================
  Files          22       22              
  Lines        6093     6200     +107     
  Branches     1249     1263      +14     
==========================================
+ Hits         5726     5834     +108     
  Misses        191      191              
+ Partials      176      175       -1     
Impacted Files Coverage Δ
fpdf/enums.py 100.00% <100.00%> (ø)
fpdf/fpdf.py 92.56% <100.00%> (+0.01%) ⬆️
fpdf/graphics_state.py 99.34% <100.00%> (+0.35%) ⬆️
fpdf/html.py 96.52% <100.00%> (+0.08%) ⬆️
fpdf/line_break.py 99.56% <100.00%> (+0.54%) ⬆️

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

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

Good job @gmischler!
Nice addition really :)

@@ -23,6 +23,7 @@ This can also be enabled programmatically with `warnings.simplefilter('default',
- the `CreationDate` of PDFs & embedded files now includes the system timezone

### Added
- Added support for subscript, superscript, nominator and denominator char positioning as well as \<sub\> and \<sup\> HTML tags, thanks to @gmischler
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Added support for subscript, superscript, nominator and denominator char positioning as well as \<sub\> and \<sup\> HTML tags, thanks to @gmischler
- Added support for subscript, superscript, nominator and denominator char positioning as well as \<sub\> and \<sup\> HTML tags, thanks to @gmischler: [link to documentation](https://pyfpdf.github.io/fpdf2/TextStyling.html#subscript-superscript-and-fractional-numbers)

docs/TextStyling.md Show resolved Hide resolved
Practical use is essentially limited to `.write()` and `html_write()`.
The feature does technically work with `.cell()` and `.multi_cell`, but is of limited usefulness there, since you can't change font properties in the middle of a line (there is no markdown support). It currently gets completely ignored by `.text()`.

The example shows the most common use cases:
Copy link
Member

Choose a reason for hiding this comment

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

Will this feature work in in a .local_context()?

with pdf.local_context(char_vpos="SUP"):
    pdf.write(...)

I don't think it will currently, but do you think it could be useful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the values are on the context stack, it will of course work within a local context as expected. It should also work when the values are supplied as arguments like in your example (covered by the **kwargs catch-all).
I'll check that to make sure and also add them to the docstring of .local_context().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I had to explicitly enable "char_vpos". I think the various lift and scale parameters are too much to handle in local_context(), though. Anyone who changes those will likely do so globally anyway.

fpdf/html.py Outdated Show resolved Hide resolved
fpdf/html.py Outdated Show resolved Hide resolved
test/text/test_write.py Show resolved Hide resolved
test/html/test_html.py Outdated Show resolved Hide resolved
fpdf/graphics_state.py Show resolved Hide resolved
@Lucas-C
Copy link
Member

Lucas-C commented Sep 7, 2022

I now went a step further, and gave each heading an extra space of 20% the font height above and below.

On second thought, this should probably be mentioned in a "Changed" section in CHANGELOG.md, because this margin change could trigger page jumps for users who use the HTMLMixin

Apart from that, is this PR ready to be merged for you? Do you agree with the minor CHANGELOG.md suggestion I made?

@Lucas-C
Copy link
Member

Lucas-C commented Sep 7, 2022

(side note: I think I'll perform a release once this is merged - no hurry though!)

@gmischler
Copy link
Collaborator Author

Apart from that, is this PR ready to be merged for you? Do you agree with the minor CHANGELOG.md suggestion I made?

@Lucas-C Yup, ready from my side.

@Lucas-C Lucas-C merged commit dbc1114 into py-pdf:master Sep 8, 2022
@Lucas-C
Copy link
Member

Lucas-C commented Sep 8, 2022

This has been released in v2.5.7

Comment on lines +248 to +249
self.heading_above = 0.2 # extra space above heading, relative to font size
self.heading_below = 0.2 # extra space below heading, relative to font size
Copy link
Member

@Lucas-C Lucas-C Jun 18, 2024

Choose a reason for hiding this comment

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

I know that this PR has been merged more than 20 months ago,
but I do not think that those properties have been documented so far.

@gmischler: would you agree to either:

  • convert those properties into class properties?
  • allow those properties to be provided to the class constructor?

...and to document those configurations settings in the docs/.

This way, we would gain in terms of consistency of the configuration options available to configure HTML rendering.

I you agree is either option, I can handle the PR to improve this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Your strange use of terminology makes it a bit of a challenge to figure out what you really want to do... I somehow doubt it really has anything to do with properties. 😉

If you are suggesting to make those instance attributes configurable via constructor parameters, then I'm all for it. 👍

I don't really see a point to turn them into class attributes, though.

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.

None yet

2 participants