Fix visual gap after {nb} page alias when text shaping is enabled (#1090) - #1894
Conversation
andersonhc
left a comment
There was a problem hiding this comment.
Using "0" * len(self.str_alias_nb_pages) for shaped layout seems safer than page_no(), because layout should not depend on the current page number when output later substitutes pages_count.
Could we also add a regression test with >= 10 pages and {nb} in the middle of shaped text? The current new test is one page, so it does not catch page 1 reserving "1" and later rendering "10" or "12".
Separately, the new top-level split on the alias seems to regress markdown parsing across {nb}. For example, **A {nb} B** parses with bold only before the alias. It may be better to keep alias handling inside the existing parser loop so markdown state is preserved.
|
@andersonhc I tested with a 12-page document. Using "0" * len(self.str_alias_nb_pages) (which is "0000" for "{nb}") still did not work as expected because actual page count "12" is shorter. Since we cannot know the total page count at the start, I propose two solutions:
|
The alias itself is already a not-so-explicit expected digits. For non-shaping we just save the space it would take to render I will do some more tests soon and get back to you on a viable solution. It should be a uniform spacing in all pages because it's going to be replaced by the same number on all pages - that's why I don't want to use current page number on the spacing. |
|
Hi @prateek-dagar , I did the following test: from pathlib import Path
from fpdf import FPDF
HERE = Path(__file__).resolve().parent
pdf = FPDF()
pdf.add_font(family="DejaVu", fname=HERE / ".." /"fonts"/"DejaVuSans.ttf")
pdf.set_text_shaping(True)
pdf.set_font("DejaVu", "", 20)
for _ in range(100):
pdf.add_page()
pdf.write(text="Hello {nb} world")
pdf.output(HERE / "test-pr1894.pdf")I still believe we should have the alias to reserve a constant space for all pages because all of them will be replaced by the same number. It's a matter of tweaking what dummy_width_string value should be, and:
I found There is one big improvement that we could do - and I totally understand if you think it's out of scope for this PR: Change TotalPagesSubstitutionFragment to save the reserved width (basically the result of
|
|
Hi @andersonhc , Thanks for the detailed feedback and suggestions! I really like the idea of saving the reserved width at layout time to auto-center the replacement text and issuing a warning if the final page count exceeds that width. I will try to implement this centering, warning, and fix the markdown regression will let you as soon as i am done with it or face any challenge implementing it. Regarding the To prevent this, my plan is to, apply the 3-character default ( What you think about this.? |
I agree with your approach for {nb} vs custom aliases. I'm excited to see the progress here. This is shaping up to a much better feature for users. Let me know if you run into any questions. |
|
Hi @andersonhc , I have updated the implementation and pushed the latest commits. Here is a summary of the fixes:
You can test the word-splitting case directly with this snippet: from fpdf import FPDF
pdf = FPDF()
# Add a shaped font (e.g. Quicksand)
pdf.add_font("Quicksand", style="", fname="test/fonts/Quicksand-Regular.otf")
pdf.set_font("Quicksand", size=24)
pdf.set_text_shaping(True)
for _ in range(12):
pdf.add_page()
# Prior to this fix, this would render split first letters: "12s haping"
pdf.write(text="Pages {nb} with {nb}shaping")
pdf.ln()
pdf.output("test_output.pdf")Please review the changes when you have a moment. Let me know if you see any issues or have further feedback! |
53aa7a3 to
bb54e52
Compare
andersonhc
left a comment
There was a problem hiding this comment.
I really like the centering. It looks much better already.


Fixes #1090
This PR resolves the issue where the special
{nb}(or a custom) page number alias in the middle of a line causes a visual gap/overlap when text shaping is enabled.To fix this, the layout width of the
TotalPagesSubstitutionFragmentis calculated using an estimated page number length (str(self.page_no())) during the layout phase only when text shaping is enabled, and falls back to the alias string under standard rendering to maintain 100% backwards compatibility.Checklist:
docs/folderCHANGELOG.mdBy submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.