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

Feature request: allow horizontal centering in TextStyle #1282

Open
Lucas-C opened this issue Oct 11, 2024 Discussed in #1280 · 17 comments · May be fixed by #1300
Open

Feature request: allow horizontal centering in TextStyle #1282

Lucas-C opened this issue Oct 11, 2024 Discussed in #1280 · 17 comments · May be fixed by #1300

Comments

@Lucas-C
Copy link
Member

Lucas-C commented Oct 11, 2024

Proposed solution

  1. Allow Align.C to be passed as a value for .l_margin in TextStyle
  2. When this .l_margin is used in FPDF._use_title_style(), if it is Align.C, then horizontally align the text

The PR implementing this should include unit tests using FPDF.start_section()

Discussed in #1280

Originally posted by Benoite142 October 10, 2024:

Hi,

I've been trying to center a TitleStyle for quite some time now. I've been using a TitleStyle for the start_section function used in the TOC placeholder and I am having a lot of trouble trying to center the TitleStyle. I know we can easily center some cell or text with align='C' but I found no way of doing that for the TitleStyle. I know we can put a value to the l_margin to make it look like it is centered, but I can't really use that since all of my titles differs in length.

I also tried aligning everything to the center of the page, but that still doesn't work with TitleStyle and the sections.
Worst case scenario is that I put everything on the left, but I would really want it to be centered.

If anyone knows how I could do this, please let me know.

Thank you for you time 😊

@Lucas-C
Copy link
Member Author

Lucas-C commented Oct 11, 2024

@allcontributors please add @Benoite142 for ideas

Copy link

@Lucas-C

I've put up a pull request to add @Benoite142! 🎉

@Lucas-C Lucas-C changed the title Feature request: allow horizontal centering in TitleStyle Feature request: allow horizontal centering in TextStyle Oct 11, 2024
@visheshdvivedi
Copy link

visheshdvivedi commented Oct 25, 2024

Hi @Lucas-C ,

I am new to this project and would like to contribute for this feature request.

Based on my understanding, here are the required changes

fpdf/fonts.py (allow to accept Align values in l_margin

class TextStyle(FontFace):
"""
Subclass of `FontFace` that allows to specify vertical & horizontal spacing
"""
def __init__(
    self,
    font_family: Optional[str] = None,  # None means "no override"
    #                                     Whereas "" means "no emphasis"
    font_style: Optional[str] = None,
    font_size_pt: Optional[int] = None,
    color: Union[int, tuple] = None,  # grey scale or (red, green, blue),
    fill_color: Union[int, tuple] = None,  # grey scale or (red, green, blue),
    underline: bool = False,
    t_margin: Optional[int] = None, 
    l_margin: Optional[int] | Optional[Align] = None, # allow to pass Align values in l_margin
    b_margin: Optional[int] = None,
    ):

fonts/fpdf.py

@contextmanager
    def _use_title_style(self, title_style: TextStyle):
        if title_style:
            if title_style.t_margin:
                self.ln(title_style.t_margin)
            if title_style.l_margin:
                if title_style.l_margin == Align.C:
                    self.set_x(self.w/2) # write code to center the title_style horizontally
                    pass
                self.set_x(title_style.l_margin)
        with self.use_font_face(title_style):
            yield
        if title_style and title_style.b_margin:
            self.ln(title_style.b_margin)

Please suggest any other changes required or not (I am not counting the test cases here, just the changes required to introduce the feature)

@Lucas-C
Copy link
Member Author

Lucas-C commented Oct 25, 2024

Hi @visheshdvivedi

Yes, this looks like a good start!
You are very welcome to initiate a Pull Request, and create a dedicated unit test 🙂

You will also find more information about fpdf2 development process on this page:
https://py-pdf.github.io/fpdf2/Development.html

There is one thing I would suggest: enums.Align inherits from enums.CoerciveEnum, meaning that the string CENTER or C can be converted to Align.C by simply calling Align.coerce(string_value).
For this reason:

  1. TextStyle constructor should also accept string as values for l_margin
  2. Inside TextStyle constructor, self.l_margin = Align.coerce(l_margin) should be called
  3. We should also consider if we support other Align values, and if not, raise an error if invalid Align values are provided for l_margin

@visheshdvivedi
Copy link

visheshdvivedi commented Oct 26, 2024

Hi @Lucas-C ,

When writing test, something like this

from fpdf import FPDF, Align, TitleStyle

def test_toc_horizontal_alignment(tmp_path):  # issue-1282
    title_styles = [
        "small title style",
        "intentionally large title style",
    ]

    pdf = FPDF()
    pdf.set_font(family="helvetica", size=12)
    pdf.add_page()
    for title in title_styles:
        level0 = TitleStyle("Helvetica", "", 20, (0, 0, 0), l_margin=Align.C)
        pdf.set_section_title_styles(level0)
        pdf.start_section(title)
    pdf.output("test.pdf")

test_toc_horizontal_alignment(tmp_path="")

When I passed the value of Align.C, it gave an error like so

  File "/<path>/fpdf2/fpdf/fpdf.py", line 3818, in multi_cell
    maximum_allowed_width = w = w - padding.right - padding.left
                                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
TypeError: unsupported operand type(s) for -: 'float' and 'Align'

I looked into it and realized that the start_section has this code here.

page_break_triggered = self.multi_cell(
    w=self.epw,
    h=self.font_size,
    text=name,
    new_x=XPos.LMARGIN,
    new_y=YPos.NEXT,
    dry_run=True,  # => does not produce any output
    output=MethodReturnValue.PAGE_BREAK,
    padding=Padding(
        top=title_style.t_margin or 0,
        left=title_style.l_margin or 0,
        bottom=title_style.b_margin or 0,
    ),
)

to check if any page break will be triggered and this takes in l_margin as input. As a result the code breaks (since no code is present here to allow Align or str values in l_margin.

Should I also update the Padding class and multi_cell function to allow Align and str values ?

@Lucas-C
Copy link
Member Author

Lucas-C commented Oct 26, 2024

Should I also update the Padding class and multi_cell function to allow Align and str values ?

No, I think the best would be to pass center=True to FPDF.multi_cell() if TextStyle.l_margin is Align.C

Your test looks good.
As mentioned on https://py-pdf.github.io/fpdf2/Development.html, you can pass generate=True once to assert_pdf_equal() in order to generate a reference PDF file.

@visheshdvivedi
Copy link

Hi @Lucas-C,

Just one issue, I could not find a way to calculate or get the width of the text in section, as shown here

if title_style.l_margin:
  if isinstance(title_style.l_margin, int):
      self.set_x()
  elif title_style.l_margin == Align.C:
      section_width = 0 # get section width
      self.set_x(self.w/2 - (section_width/2))
  elif title_style.l_margin == Align.L:
      self.set_x(0)
  elif title_style.l_margin == Align.R:
      section_width = 0 # get section width
      self.set_x(self.w - section_width)
  else:
      raise ValueError(f"Value {title_style.l_margin} is not a valid l_margin value for TextStyle")

Is it stored in any variable, or is there any way to calculate it ?

@visheshdvivedi
Copy link

I further looked into the center attribute in multi_cell and how it works. Turns out it is not working properly because the value of w is not valid (it is same as epw and thus the formula to calculate value of x makes it incorrect)

self.x = (
    self.w / 2 if align == Align.X else self.l_margin + (self.epw - w) / 2
)

How should I move forward with the same ?

@Lucas-C
Copy link
Member Author

Lucas-C commented Oct 30, 2024

I further looked into the center attribute in multi_cell and how it works. Turns out it is not working

I am really surprised by this, as we have unit tests ensuring that it works properly:

Could you provide a an autonomous code snippet demonstrating a bug?

For reference:

  • pdf.w is the full current page width
  • pdf.epw is the effective page width, meaning its full width minus its horizontal margins

@visheshdvivedi
Copy link

Hi @Lucas-C,

Here is a script I have written to write down some text (and center it horizontally using center attribute of multi_cell function) here.

from fpdf import FPDF

def test_multi_cell_centering():
    pdf = FPDF()
    pdf.add_page()
    pdf.set_font("Helvetica")
    pdf.multi_cell(w=200, text="Multi cell center text", center=True)
    pdf.output("test_multi_cell_centering.pdf")

if __name__ == "__main__":
    test_multi_cell_centering()

As per my understanding, this script should create a pdf with text "Multi cell center text" horizontally center of the page. However, the pdf created looks like this.

image

If this is the expected behaviour, then probably I have some misunderstanding of the multi_cell function. In that case, please educate me on the same.

If this is not the expected behaviour, we would need to resolve this first to make the horizontal centering feature work.

@andersonhc
Copy link
Collaborator

Hi @visheshdvivedi ,

Multi cell will create a cell and render your text inside it. You are specifying your cell width is 200, so your text is centered inside this cell. If you set a cell border it will be easier to see it:

pdf.multi_cell(w=200, text="Multi cell center text", center=True, border=1)

Set the border and do try increasing w a few times and you will understand how it works.

If you want a cell to cover the whole page width, respecting the borders, you can use:

pdf.multi_cell(w=pdf.epw, text="Multi cell center text", center=True)

@Lucas-C
Copy link
Member Author

Lucas-C commented Oct 31, 2024

I agree with @andersonhc detailed answer 🙂👍

This should prove that center=True works fine, if you replace the .multi_cell() call in your script by this one:

pdf.multi_cell(w=40, text="Multi cell center text", center=True, border=1)

image

@visheshdvivedi
Copy link

visheshdvivedi commented Nov 6, 2024

Hi @Lucas-C,

I understood that center=True centers the cell horizontally along the page, but how can we center the text inside the cell. For example, if I increase the width of the cell to 100, the text is no longer in the center.

pdf.multi_cell(w=100, text="Multi cell center text", center=True, border=1)
image

And based on my understanding, we need a way to center text without the need to specify any width (being able to center text for variable sizes of text without the need to specify custom width for each of them)

image

So I just wanted to know how can we achieve that ?

@Lucas-C
Copy link
Member Author

Lucas-C commented Nov 6, 2024

but how can we center the text inside the cell

By passing align="C" 🙂
Or align=fpdf.enums.Align.C - https://py-pdf.github.io/fpdf2/fpdf/enums.html#fpdf.enums.Align

pdf.multi_cell(w=100, text="Multi cell center text", center=True, border=1, align="C")

Earlier I wrote:

No, I think the best would be to pass center=True to FPDF.multi_cell() if TextStyle.l_margin is Align.C

But on second thought, it may be better to pass align="C" instead of center=True to FPDF.multi_cell()

@visheshdvivedi
Copy link

Hi @Lucas-C,

Thanks for your support, I am clear with the functionality now. Here is my final take.

  1. Passing center=True will center the cell horizontally with respect to the page
  2. Passing align='C' will center the text inside the cell horizontally with respect to the cell.

So now for the final implementation (based on the requirements of the above issue), I will pass the value of l_margin (if it is align or string) to align in multi_cell. This would allow users to specify Align.C, Align.R and Align.L as l_margin values and it will accordingly center align, right align or left align text with respect to the cell.

Please share your views or confirmation so that I raise the pull request.

@visheshdvivedi
Copy link

Here is a test case I have created along with its results with the added feature

from fpdf import FPDF, Align, TextStyle

def test_textstyle_l_margin_horizontal_alignment(tmp_path):  # issue-1282

    pdf = FPDF()
    pdf.set_font(family="Helvetica", size=12)
    pdf.add_page()

    # left align
    level0 = TextStyle("Helvetica", "", 20, (0, 0, 0), l_margin=Align.L)
    pdf.set_section_title_styles(level0)
    pdf.start_section("left aligned section")
    
    # center align
    level0 = TextStyle("Helvetica", "", 20, (0, 0, 0), l_margin=Align.C)
    pdf.set_section_title_styles(level0)
    pdf.start_section("center aligned section")

    # right align
    level0 = TextStyle("Helvetica", "", 20, (0, 0, 0), l_margin=Align.R)
    pdf.set_section_title_styles(level0)
    pdf.start_section("right aligned section")

    pdf.output("test.pdf")

test_textstyle_l_margin_horizontal_alignment(tmp_path="")

Output: test.pdf

image

@Lucas-C
Copy link
Member Author

Lucas-C commented Nov 7, 2024

So now for the final implementation (based on the requirements of the above issue), I will pass the value of l_margin (if it is align or string) to align in multi_cell. This would allow users to specify Align.C, Align.R and Align.L as l_margin values and it will accordingly center align, right align or left align text with respect to the cell.

Yes, this seems like a great plan 👍

Here is a test case I have created along with its results with the added feature

Awesome, good job!

You can go on an open a Pull Request 🙂

@visheshdvivedi visheshdvivedi linked a pull request Nov 7, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants