search

kyamagu / skia-python

Python binding to Skia Graphics Library
https://kyamagu.github.io/skia-python/
BSD 3-Clause "New" or "Revised" License
245 stars 43 forks source link

M121 public #222

Closed HinTak closed 9 months ago

HinTak commented 11 months ago

The changes are all straight-forward. The font tests pass, but the change of what Typeface.MakeDefault does (from a normal default to empty) as from upstream may need rethinking / emulate properly, or with a large warning in the release notes. It will likely break somebody's code, if they assume MakeDefault will give them Times/Arial/Noto, instead of a font with no glyphs.

Re-think on that.

HinTak commented 10 months ago

@kyamagu skia m122 should be out in a few days so this is probaby ready to go soon. Still missing a "README.m121" etc, but code-wise this is okay. The number of skipped tests has just gone under 100, so it is only 70-ish worse than m87 now. I'll add a README.m121 soon. As for the default typeface case, it is difficult to tell what was the default before (it differs from system to system), but I think the old behavior is probably the same as skia.Typeface("") (note the empty string, equivalent toskia.Typeface.MakeFromName(""), fonts with any name, since "" matches anything). I think most people should use a non-empty string e.g. skia.Typeface("Roman") (or "Times" or "Sans"). I think telling people that if they really don't care what font is used, they should convert skia.TypeFace() to Skia.Typeface("") isn't too troublesome either. Do you want skia.Typeface() to behave like skia.Typeface("") (to avoid breaking people's code) or align with upstream's recommendation i.e. make people choose a font?

HinTak commented 10 months ago

@kyamagu #148 's example relies on a "non-empty" default font.

kyamagu commented 10 months ago

@HinTak Thanks!

As for skia.Typeface(), I am okay with either case: 1) keeping the old behavior with a deprecation warning, or 2) switching to the upstream behavior with a specific deprecation error message. What do you think?

HinTak commented 10 months ago

@kyamagu it is kind of a difficult choice - upstream obviously does not want people to assume they can get a font/typeface that draws something with null input, but I expect many existing skia-python users do (as in #148 ). Also I suspect very few skia-python users actually want the MakeEmpty behavior (a valid font/typeface that draws nothing). Also slightly complicated is, do people call the default constructor THEN make changes to it, and how would it work if it starts with "match anything"? There is also the 3rd option, which is withdrawing skia.Typeface() altogether to make sure people change their code to skia.Typeface("") if they really don't care. So we really have 3 choices on this (1) make it behave like make Empty (very few users want this), (2) make it behave like "", (3) removing skia.typeface() altogether to force people to change their code to skia.typeface(""). I am leaning towards (2) or (3). (2) is probably least complaints from users. Oh, I probably should add an explicit "makeempty()" just in case there are people who really want that. (Difficult to think why anybody wants a font/typeface which draws nothing...). I think going for (2) but saying we'll eventually do (3) would be a good idea.

kyamagu commented 10 months ago

@HinTak Sure, let's take option 2: keeping the backward compatibility with a deprecation warning. It is possible to explicitly emit python warning (warnings.warn) or add a deprecation note in the docstring. Either is fine.

HinTak commented 10 months ago

@kyamagu cool. I'll look into adding the warning and including it in this pull.

kyamagu commented 10 months ago

@HinTak Thanks! Perhaps calling warnings.warn or logging.warning via pybind11 API is sufficient for visible user notice.

HinTak commented 10 months ago

Okay, the latest does this (4 times, with 4 tests):

tests/test_font.py:430
  ..../tests/test_font.py:430: DeprecationWarning: "Default typeface" is deprecated upstream. Please specify name/file/style choices.
    (skia.Typeface.MakeDefault(), 10),
kyamagu commented 10 months ago

@HinTak Many thanks!

HinTak commented 9 months ago

@kyamagu thanks for the heads-up on the font patch. I am aware of it being flaky and font-related development is an active area in recent skia - that's what brought me to skia-python, actually. Was wanting to have anothe look at #148 before merging. I just checked the logs, apparently I have a few re-enabled tests not yet included. Just pushed, will paste new result when close.

ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.27s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.21s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.11s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 4.08s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 3.69s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 3.77s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 4.50s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 4.11s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 3.91s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 3.94s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 3.70s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1852 passed, 337 skipped, 4 xfailed, 5 warnings in 4.21s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1842 passed, 347 skipped, 4 xfailed, 4 warnings in 4.86s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1842 passed, 347 skipped, 4 xfailed, 4 warnings in 4.72s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 6.31s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 5.07s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 4.31s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2065 passed, 118 skipped, 11 xfailed, 4 warnings in 5.21s 
ubuntu-22.04 (aarch64) for cp3712:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 42.65s 
ubuntu-22.04 (aarch64) for cp3812:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 42.41s 
ubuntu-22.04 (aarch64) for cp3912:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 40.92s 
ubuntu-22.04 (aarch64) for cp31012:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 42.56s 
ubuntu-22.04 (aarch64) for cp31112:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 38.10s 
ubuntu-22.04 (aarch64) for cp31212:  2076 passed, 106 skipped, 11 xfailed, 4 warnings in 38.29s
HinTak commented 9 months ago

looking at the re-enable tests, I realized skia.Font() and skia.Font(None, ....) should trigger deprecation warnings too. The latter is a bit tricky (but #148 is basically its test) so I shall add those two before merging.

HinTak commented 9 months ago

Skipped is below 100 now.

ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.61s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.54s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.47s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.41s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.01s 
ubuntu-22.04 (auto64) for cp3{7,8,9,10,11,12:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 4.15s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 4.49s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 4.07s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 3.86s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 4.10s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 3.78s 
windows-2022 (auto64) for cp3{7,8,9,10,11,12:  1860 passed, 329 skipped, 4 xfailed, 5 warnings in 3.85s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1850 passed, 339 skipped, 4 xfailed, 4 warnings in 15.59s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  1850 passed, 339 skipped, 4 xfailed, 4 warnings in 15.10s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 16.08s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 14.68s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 14.81s 
macos-12 (auto64) for cp3{7,8,9,10,11,12}12:  2073 passed, 110 skipped, 11 xfailed, 4 warnings in 13.50s 
ubuntu-22.04 (aarch64) for cp3712:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 42.18s 
ubuntu-22.04 (aarch64) for cp3812:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 42.62s 
ubuntu-22.04 (aarch64) for cp3912:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 41.84s 
ubuntu-22.04 (aarch64) for cp31012:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 41.24s 
ubuntu-22.04 (aarch64) for cp31112:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 38.27s 
ubuntu-22.04 (aarch64) for cp31212:  2084 passed, 98 skipped, 11 xfailed, 4 warnings in 39.01s

I am adding deprecation warnings to Font() and Font(None,...) too, as upstream don't return a practically useable (i.e. draw something) Font with them lately. This pushes the warnings up to 20+ as skia.Font() is used quite often in the tests...

kyamagu commented 9 months ago

BTW, python 3.7 is already EOL. Feel free to drop the support

HinTak commented 9 months ago

@kyamagu the new release is having a 403 error reaching pypi - any authentication change recently? Maybe actions needs updating (it says "legacy")?

ERROR HTTPError: 403 Forbidden from https://upload.pypi.org/legacy/
Invalid or non-existent authentication information. See
https://pypi.org/help/#invalid-auth for more information.

About python 3.7 - cutting it doesn't really save build time, so probably can leave it alone until something breaks...

kyamagu commented 9 months ago

@HinTak Will look into it