I have let CI started, but I wish you have got in touch earlier, as I have already got some of this overlapping in my hard disk already. See #259

@khaledhosny ci failed on mac os (which has python 3.13.1). You need to pull in the fix for #295 , which is #296 , to pass ci.

FWIW, even namespacing these classes under FontArguments is troublesome - there is aSkFontArguments class upstream in core, and a skia::textlayout::FontArguments class in the paragraph module.

I’m not sure I get the namespacing comments. The classes are already nested in Python, skia.FontArguments.Palette(), skia.FontArguments.Palette.Overrides(), and skia.FontArguments.Palette.Override(), so I’m not sure what should I do other than that.

This #298 isn't how it is on my hard disk (I have neighbouring code with does something else, so there are collisions while cherry-picking), nor ready for used or tested, but I think if it works, together with your pull, should mostly just be the other half of allowing one to address #259 , loading COLRv1 fonts with custom / non-default palettes.

With the two together, you should be able to do something like this to load a colrv1 font with custom palette:

# Read font file into fontdata
scanner = skia.FontScanner()
# do stuff with FontArguments.Palette
newface = scanner.MakeFromStream(fontdata, fontargs)

With this PR I can already select non-default palettes and override their colors in COLR (v0 and v1) fonts, but I’m testing only with FreeType fonts:

fontMgr = skia.FontMgr.New_Custom_Empty()

face1 = fontMgr.makeFromFile(fontpath)
font1 = skia.Font(face, face.getUnitsPerEm())

fontArgs2 = skia.FontArguments()
fontArgs2.setPalette(1)
face2 = face1.makeClone(fontArgs2)
font2 = skia.Font(face2, size)

fontArgs3 = skia.FontArguments()
palette3 = skia.FontArguments.Palette(
    1,
    skia.FontArguments.Palette.Overrides(
        [
            skia.FontArguments.Palette.Override(0, skia.ColorMAGENTA),
            skia.FontArguments.Palette.Override(6, skia.ColorBLUE),
            skia.FontArguments.Palette.Override(7, skia.ColorGREEN),
            skia.FontArguments.Palette.Override(8, skia.ColorCYAN),
        ]
    ),
)
fontArgs3.setPalette(palette3)
face3 = face1.makeClone(fontArgs3)
font3 = skia.Font(face3, size)

string = "\u06DD"
text1 = skia.TextBlob.MakeFromShapedText(string, font1)
text2 = skia.TextBlob.MakeFromShapedText(string, font2)
text3 = skia.TextBlob.MakeFromShapedText(string, font3)

bounds = text.bounds()

width = math.floor(bounds.width() * 3)
height = math.floor(bounds.height())

surface = skia.Surface(width, height)
with surface as canvas:
    canvas.drawColor(skia.ColorWHITE)
    paint = skia.Paint()
    canvas.drawTextBlob(text1, -bounds.x(), -bounds.y(), paint)
    canvas.drawTextBlob(text2, bounds.width() - bounds.x(), -bounds.y(), paint)
    canvas.drawTextBlob(text3, bounds.width() * 2 - bounds.x(), -bounds.y(), paint)

Interesting. Thanks for the code - some of it can go into the tests directory. (skia's resources/fonts/ is available for tests).

FWIW, I have some incomplete code which supposedly let you do face.getFontDescriptor().setPaletteIndex(N) (and short-circuiting to face.setPaletteIndex(N)) to switch. Does not work (yet).

If the asserts are not logically independent I.e. you just want an exact match of a composite structure, it might be clearer to do assert x and y and z a single mulit-line assert, any mis-match is considered a failure. If there are multiple independent/stages ways of failure, you do a fixture then multiple tests.

I think face.getFontDescriptor().setPaletteIndex(N) not working is an upstream bug but I have not gotten round to file it yet. Basically the paletteindex is stored in an intermediate structure but never passed onto the font renderer. In the case of colrv1, freetype never gets it set. But similar comments applies to the colrv0 path in the directwrite rendering path too.

assert isinstance... is typically one per test, and the object itself done as a re-used fixture. See the other tests.

See for example the fixture at https://github.com/kyamagu/skia-python/blob/db7d7fcc84dc5e59ea8a925457f2d319a57052df/tests/test_paragraph.py#L12 and the init test below.

And all the fxtures finally get passed to the usage test at https://github.com/kyamagu/skia-python/blob/db7d7fcc84dc5e59ea8a925457f2d319a57052df/tests/test_paragraph.py#L45

Interesting. Thanks for the code - some of it can go into the tests directory. (skia's resources/fonts/ is available for tests).

That would be nice, but testing palette overrides will requiring checking the rendered output (possibly against a reference image), but I don’t see any tests doing something like this.

That would be nice, but testing palette overrides will requiring checking the rendered output (possibly against a reference image), but I don’t see any tests doing something like this.

Well, setting and getting it back should work. It is probably possible to render to a surface and detect it is "mostly red", say, just by count the red pixels. I have a test in the paragraph part, which used to have trouble with rendering "\n" as a new line rather than .notdef. The test is simply that the rendered area should be tall than wide, whichever the default font is.

I am starting to see the variationposition code from which this modelled from a bit buggy about life times and zero length arguments...

You added 3 classes, so there should be 3 init tests, in addition to the class method tests...

I have had a better look at the example code. I think this pull is substantially incomplete (in addition to all the detailed comments so far) - to be of any use to somebody who isn't the font author, it really needs the face.getFontDescriptor().get*Palette*Override* method to find out what is settable (or use another tool to examine the font to find out what is settable). Hence this is of no use without those.

So this is not going to be merged without substantially additional work.

I see your example must have used one of your own authored arabic fonts (Amiri Quran?). That's fair enough, but an example with one of the English fonts would be preferred; as well as some tests with the font files in skia/resources/fonts/ .

I"d have preferred that this stays open until you or somebody else improves on it. Nevermind.

Well, I added bindings for existing Skia APIs, but you are asking for palette discoverability APIs that Skia does not currently have as a prerequisite of merging this PR, so I don’t know what I was supposed to do.

I wrote earlier that palette discovery looks to be possible binding around face.getFontDescriptor().get*Palette*Override* (the equvalent c++ code behind).

I have the python bits for face.getFontDescriptor()... but I haven't got time to finish it yet.

I wrote earlier that palette discovery looks to be possible binding around face.getFontDescriptor().get*Palette*Override* (the equvalent c++ code behind).

This does not query the font, it returns whatever the user have set.

Yes, I believe that is an upstream bug. Skia-python is not against carry interim fixes for upstream bugs, before/during they are being accepted.

Skia has no code whatsoever to query font palettes, so again you blocked this PR on developing code that upstream does not have. If Skia is fine for its users with its API as it is, why can’t this Python bindings.

Example - #290 - it is an upstream bug for which a proposed fix was posted a while ago by somebody else, and I intend to take it on here at some point, even if upstream hasn't accepted it yet. That fix helps avoiding people filing bug reports with spurious and misleading information here, so we should have it even if upstream hasn't accepted it.

Note I am not "blocking" this PR. I am merely saying it is incomplete / need more work, because as it is, it requires special input font files and example uses of the additional code requires intimate knowledge of the special test font files. Until it becomes more generally useful to other input font files, the new code is only useful to you, the supplier/author of the special input font file.

IMHO, PR (in general, not specifically for this project) are just NOT merged by default, and nobody (including me) should assume any pull submitted will be merged. It just isn't, and shouldn't.

Also, FWIW, other pulls have stayed opened for months / years without a demand to merge. Typically mine get merged here after after 2 months when I say it is ready. You are demanding a merge or withdrawal in 10 days?

So this PR is incomplete because Skia, the upstream, is missing an API you think it important to use the existing Skia API that this PR exposes to Python users?

CSS allow selecting/overriding font palettes (just like the API this PR binds), but browsers lack any API to query font palettes, so CSS support for font palettes is unusable, too?

Skia/CSS allow setting OpenType font features, but lack APIs to query fonts for supported features, so Skia/CSS support for font features is unusable, too?

To be frank, it feels like you are just making things up, and while no one should expect their code contribution to be accepted, you should have stated your (made up) objection up front, instead of wasting my time with minute details while then later stating this PR can never be merged until Skia develop some API you think are a prerequisite for using this existing API.

FWIW, #181 stayed open for 18 months, before it was superceded and closed. Personally I think I would prefer to spend my time (and you spend yours) actually coding, than continuing this exchange. Sorry.

I have previously commented this:

You added 3 classes, so there should be 3 init tests, in addition to the class method tests...

The new issue #310 is a good illustration why we insist on this. This pull is, while perhaps useful to some, substantially incomplete in its current form. Skia-python currently binds ~1000 symbols in skia and have a test suite with ~2400 tests, I.e. about 2.4 tests per symbols. This pull added 3 classes and about a dozen symbols with only two tests with parametrics. That is very low.

Also a side-issue: we try not to merge pulls which fails CI (for the same reason). While the reason for this pull failing CI is known; you could have cherry-picked the fix; we may do it ourselves - e.g. merge this with another pull which contains the fix, typically at a merge window I.e. just before a new release updating the skia version - before merging into HEAD. That would be about once every two months currently. CI at the moment takes over 5 hours to run - and c.f. your demand to merge or withdraw in 10 days. We don't merge large changes at that timescale.

Cc. @kyamagu