fpdf2 icon indicating copy to clipboard operation
fpdf2 copied to clipboard

Allow for using wrapmode='CHAR' in templates

Open carlhiggs opened this issue 1 year ago • 2 comments

Fixes #1159

Checklist:

  • [ ] 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.

  • [x] A unit test is covering the code added / modified by this PR

  • [x] This PR is ready to be merged

  • [x] In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • [x] A mention of the change is present in CHANGELOG.md

In the flextemplate test for the wrapmode functionality, I tested a range of possibilities described in text of each template element. The first three elements test that the new code doesn't do anything strange to existing funcitonality; the last one specifically tests that wrapmode='CHAR' wraps accordingly:

elements = [
    {
        "name": "multi",
        "type": "T",
        "x1": 80,
        "y1": 10,
        "x2": 170,
        "y2": 15,
        "text": "If multiline is False, the text should still not wrap even if wrapmode is specified.",
        "background": 0xEEFFFF,
        "multiline": False,
        "wrapmode": "WORD",
    },
    {
        "name": "multi",
        "type": "T",
        "x1": 80,
        "y1": 40,
        "x2": 170,
        "y2": 45,
        "text": "If multiline is True, and wrapmode is omitted, the text should wrap by word and not "
                "cause an error due to omission of the wrapmode argument.",
        "background": 0xEEFFFF,
        "multiline": True,
    },
    {
        "name": "multi",
        "type": "T",
        "x1": 80,
        "y1": 70,
        "x2": 170,
        "y2": 75,
        "text": "If multiline is True and the wrapmode argument is provided, it should not cause a "
            "problem even if using the default wrapmode of 'WORD'.",
        "background": 0xEEFFFF,
        "multiline": True,
        "wrapmode": "WORD",
    },
    {
        "name": "multi",
        "type": "T",
        "x1": 80,
        "y1": 100,
        "x2": 170,
        "y2": 105,
        "text": "If multiline is True and the wrapmode argument is provided, it should result in "
            "wrapping based on characters instead of words, regardless of language (i.e. even though "
            "this is designed to support scripts like Chinese and Japanese, wrapping long sentences "
            "like this in English still demonstrates functionality.)",
        "background": 0xEEFFFF,
        "multiline": True,
        "wrapmode": "CHAR",
    },
]
pdf = FPDF()
pdf.add_page()
pdf.set_font("courier", "", 10)
templ = FlexTemplate(pdf, elements)
templ.render()
assert_pdf_equal(pdf, HERE / "flextemplate_wrapmode.pdf", tmp_path)

The output of the above looks correct to me: image

I ran black and checked pylint in vscode; I believe the changes I made should be all fine for these requirements.

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

carlhiggs avatar May 18 '24 12:05 carlhiggs

@allcontributors please add @carlhiggs for code

gmischler avatar May 20 '24 08:05 gmischler

@gmischler

I've put up a pull request to add @carlhiggs! :tada:

allcontributors[bot] avatar May 20 '24 08:05 allcontributors[bot]

I've just addressed latest round of review requests: more accurately referring to when 'wrapmode is "CHAR"' in one specific test's self-descriptive text (and updated test pdfs used to validate this to have the changed text too), and externalising duplicate text to a shared python file containing elements now used in both template and flextemplate tests of the optional wrapmode functionality. Black and pylint were run using CLI, and through pre-commit hook. The pylint warnings that remained did not relate to my tests or modifications, but to various "pylint.extensions" that apparently were impossible to load ("bad_plugin-value"?). I believe that is not an issue with this pull request, but something upstream. If this passes checks, do you think this could be could to merge @gmischler ?

carlhiggs avatar May 27 '24 02:05 carlhiggs

Thanks for the contribution, @carlhiggs ! Merging now.

gmischler avatar May 27 '24 14:05 gmischler