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 for different charset encodings in PcfFontFile #3907

Merged
merged 5 commits into from
Mar 31, 2020

Conversation

yawor
Copy link
Contributor

@yawor yawor commented Jun 16, 2019

Changes proposed in this pull request:

Currently PcfFontFile class outputs only bitmap pil fonts with iso8859-1 encoding, even though the PCF format supports Unicode. This makes in hard to work with Pillow with bitmap fonts in languages which use different character sets. With my PR it's possible to set different charset encoding in PcfFontFile's class constructor. By default, it generates pil font file with iso8859-1 like before the change.
Generated pil font file still contains up to 256 characters, but the character set is different depending on the selected encoding.
The change works in both Python 2 and 3.
To use such font with ImageDraw.text method, the method must be called with a str (py2)/bytes (py3) object with the same encoding as the font file.

src/PIL/PcfFontFile.py Outdated Show resolved Hide resolved
@radarhere radarhere changed the title Support for different charset encondings in PcfFontFile Support for different charset encodings in PcfFontFile Jun 17, 2019
@radarhere
Copy link
Member

Could you add a test case, something that fails without your change and passes with your change?

@yawor
Copy link
Contributor Author

yawor commented Jun 26, 2019

Should I copy test_font_pcf.py as a new test (for example test_font_pcf_charsets.py or something) and adapt it for different font and charset? Or should I add new tests in the original one?
Looking at the structure of the current test file for pcf, it seems that the former approach may be better.

@radarhere
Copy link
Member

I would say a new test, yes.

@yawor
Copy link
Contributor Author

yawor commented Jun 26, 2019

To do the tests I need to also include another pcf file in the Fonts directory alongside properly generated pil/pbm ones for few charsets. Is there any policy regarding what I can place there? I've been testing this using xos4 Terminus fonts amongst others. It is licensed under SIL OFL 1.1. Is it OK to add it to the repository?

@radarhere
Copy link
Member

Sure. Just add another line to the list in https://github.com/python-pillow/Pillow/blob/master/Tests/fonts/LICENSE.txt

@hugovk hugovk assigned hugovk and unassigned hugovk Jun 27, 2019
@ElectronicElephant
Copy link

Can confirm this bug exists. BTW, I'm wondering when it will be mergerd?

@ElectronicElephant
Copy link

Also, if I'm understanding the code correctly, this line again makes the whole program decodes only 256 chars from pcf file, which is not acceptable.

https://github.com/yawor/Pillow/blob/94b1a88e4983da96132ead4eebc147462f9ce54f/src/PIL/PcfFontFile.py#L222

I'm corrently working with Pillow to print Chinese Bitmap-fonts onto a screen. It's a hard time for me.

@yawor
Copy link
Contributor Author

yawor commented Jan 20, 2020

@EletronicElephant this probably still waits for writing tests for this feature, about which I've totally forgot.
I've just added them to this PR.
Unfortunately this PR won't help with your problem. The issue is not with the PCF to PIL font converter, but with the PIL font format itself, which allows only 256 characters. To allow more than 256 characters, the format itself would need to be extended or changed.

@ElectronicElephant
Copy link

@yawor Bad news for me. I'll have to walk around for a new method... Thank you all the same ~

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Not a full review, just a couple of quick comments.

And please could you fix the lint on CI with?

pip install -U black
black .

Tests/test_font_pcf_charsets.py Outdated Show resolved Hide resolved
Tests/test_font_pcf_charsets.py Outdated Show resolved Hide resolved
Tests/test_font_pcf_charsets.py Outdated Show resolved Hide resolved
@yawor
Copy link
Contributor Author

yawor commented Mar 31, 2020

@hugovk regarding the lint issue, I can remove the u prefix from strings. Somehow I've overlooked that support for Py2.7 has been dropped when rebasing my changes on the latest master at that time.

@hugovk hugovk merged commit 596adb9 into python-pillow:master Mar 31, 2020
@hugovk
Copy link
Member

hugovk commented Mar 31, 2020

Thank you!

@hugovk hugovk mentioned this pull request Mar 31, 2020
23 tasks
hugovk added a commit to hugovk/Pillow that referenced this pull request Apr 1, 2020
hugovk added a commit that referenced this pull request Apr 1, 2020
Add release notes for #3907 PcfFontFile charset encodings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants