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

Add language, font metrics font #55

Merged
merged 11 commits into from
Jun 19, 2024

Conversation

georgeharker
Copy link
Contributor

Updates CFFI generation to work with more recent pango.

unfortunately the glyph tier test is broken - but appears so with my local pango at HEAD also. I'm investigating that.

Mostly I needed access to font metrics

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 68.62745% with 16 lines in your changes missing coverage. Please review.

Project coverage is 97.82%. Comparing base (82ebc95) to head (e291661).

Current head e291661 differs from pull request most recent head 68437dd

Please upload reports for the commit 68437dd to get more accurate results.

Files Patch % Lines
pangocffi/font_metrics.py 59.09% 9 Missing ⚠️
pangocffi/language.py 70.58% 5 Missing ⚠️
pangocffi/context.py 66.66% 1 Missing ⚠️
pangocffi/font.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
- Coverage   99.30%   97.82%   -1.48%     
==========================================
  Files          20       23       +3     
  Lines        1008     1058      +50     
==========================================
+ Hits         1001     1035      +34     
- Misses          7       23      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leifgehrmann
Copy link
Owner

Hi George, apologies for the late reply.

Your changes look great!

You'll notice I ran the GitHub actions, so it'll have checked code coverage and linting.

Understandably, CodeCov is warning about missing tests for the new classes you've added, but that's okay, tests can be added later after a new release of pangocffi.

There are a couple of simple fixes for linting (which can be run locally using make lint or flake8 pangocffi tests --exclude pangocffi/_generated/ffi.py). I can fix them in a later commit after merging this in, or you can fix it yourself in this PR, up to you.

You mentioned "the glyph tier test is broken". Can you elaborate? I did have to fix some issues for the GitHub actions for macOS in #57 , but perhaps you've found something else.

@georgeharker
Copy link
Contributor Author

Thanks, I'll take another look at lint - my flake8 is throwing a load of out of order import issues - possibly due to local flake8 settings, fixed the ones mentioned here.

In terms of the test, it was tests/functional/test_glyph_item_iter_with_context.py:31, but this appears to be broken for me at HEAD also - the text is always blank though all other fields work.

pangocffi/context.py Outdated Show resolved Hide resolved
@georgeharker
Copy link
Contributor Author

I really don't understand the missing glyphIterText. I'll try again on HEAD but it's just blank for me with libpango from homebrew (I'm on a Mac for testing but can also test Linux)

@georgeharker
Copy link
Contributor Author

I can confirm this works as expected on linux - so there's something about the mac setup that's odd, same for HEAD

@leifgehrmann leifgehrmann merged commit 3b4c35d into leifgehrmann:master Jun 19, 2024
@leifgehrmann
Copy link
Owner

Hi, I’ve merged these changes, but after writing some tests I discovered a few issues, which I’ve fixed in this PR: #58

  • I’ve changed FontMetrics to be pythonic: All the getter methods have been changed to properties rather than methods. e.g. my_font_metric.approximate_char_width() is now my_font_metric.approximate_char_width.
  • I discovered a few issues with some of the return types:
    • pango_context_load_font() in some environments can return a null pointer, so I’ve changed Context.load_font() to return Optional[Font].
    • pango_language_from_string() is ambiguous when a null pointer can be returned, so I’ve changed Language.from_string() to return Optional[Language].
    • pango_language_get_preferred() returns an array of languages (or even a null-pointer), not a single language, so I’ve changed Language.preferred() to return Optional[List['Language’]].
  • I also fixed a few internal errors:
    • Wrong parameter name for Language.from_string().
    • The parameter for Language.from_string(lang) needed to be cast to char[].
    • The parameter for Language.matches(range_list) needed to be cast to char[].
    • The return value for Language.to_string() needed to be decoded to utf-8.

Hopefully these changes don’t cause any major issues for you, but please let me know if something else needs changing, or if I've made a mistake.

Regarding your issue with test_glyph_item_iter_with_context.py, I am running the tests through GitHub actions, which tests Ubuntu, Windows, and macOS. I’m also running a mac (albeit an outdated Intel mac), and the tests pass fine. I’ll see if I can test this on a more modern version of macOS at some point, but in the meantime I hope this issue with the test doesn’t affect your usage!

I’ll be preparing a new version of pangocffi later this week.

Once again, thanks a lot for your contribution!

@georgeharker
Copy link
Contributor Author

georgeharker commented Jun 19, 2024 via email

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.

2 participants