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.

Sign up for GitHub

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 SkFontArguments::Palette #297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+117 −0
Open

Support SkFontArguments::Palette #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions src/skia/Font.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ sk_sp<SkFontMgr> SkFontMgr_RefDefault() {

using Axis = SkFontParameters::Variation::Axis;
using Coordinate = SkFontArguments::VariationPosition::Coordinate;
using Override = SkFontArguments::Palette::Override;
PYBIND11_MAKE_OPAQUE(std::vector<Coordinate>);
PYBIND11_MAKE_OPAQUE(std::vector<Override>);

namespace {

Expand All @@ -65,6 +67,12 @@ void SetVariationPositionCoordinates(
vp.coordinateCount = coords.size();
}

void SetPaletteOverrides(
SkFontArguments::Palette& palette,
const std::vector<Override>& overrides) {
palette.overrides = overrides.empty() ? nullptr : &overrides[0];
palette.overrideCount = overrides.size();
}

py::tuple SkFontStyleSet_getStyle(SkFontStyleSet* self, int index) {
SkFontStyle style;
Expand Down Expand Up @@ -342,6 +350,59 @@ variationposition
&SkFontArguments::VariationPosition::coordinateCount)
;

py::class_<SkFontArguments::Palette> palette(
fontarguments, "Palette",
HinTak marked this conversation as resolved.
Show resolved Hide resolved
R"docstring(
A a palette to use and overrides for palette entries.
)docstring");

py::class_<Override>(palette, "Override")
HinTak marked this conversation as resolved.
Show resolved Hide resolved
.def(py::init(
[] (uint16_t index, SkColor color) {
return Override({index, color});
}),
py::arg("index"), py::arg("color"))
.def("__repr__",
[] (const Override& self) {
return py::str("Override(index={}, color={})").format(
self.index, self.color);
})
.def_readwrite("index", &Override::index)
.def_readwrite("color", &Override::color)
;

py::bind_vector<std::vector<Override>>(palette, "Overrides");
HinTak marked this conversation as resolved.
Show resolved Hide resolved

palette
.def(py::init(
[] (int index, const std::vector<Override>& overrides) {
SkFontArguments::Palette palette;
palette.index = index;
SetPaletteOverrides(palette, overrides);
return palette;
}),
py::keep_alive<1, 3>(),
py::arg("index"), py::arg("overrides"))
.def(py::init(
[] (int index) {
return SkFontArguments::Palette({index, nullptr, 0});
}),
py::arg("index"))
.def("__repr__",
[] (const SkFontArguments::Palette& self) {
return py::str("Palette(index={}, overrideCount={})").format(
self.index, self.overrideCount);
})
.def_property("overrides",
[] (const SkFontArguments::Palette& self) {
return std::vector<Override>(
self.overrides, self.overrides + self.overrideCount);
},
&SetPaletteOverrides)
Copy link
Collaborator

@HinTak HinTak Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Don't you need a keep_Alive policy here too? On the set method.

.def_readwrite("index", &SkFontArguments::Palette::index)
.def_readonly("overrideCount", &SkFontArguments::Palette::overrideCount)
;

fontarguments
.def(py::init<>())
.def("setCollectionIndex", &SkFontArguments::setCollectionIndex,
Expand All @@ -352,6 +413,19 @@ fontarguments
actually be indexed collections of fonts.
)docstring",
py::arg("collectionIndex"))
.def("setPalette", &SkFontArguments::setPalette,
R"docstring(
Specify a palette to use and overrides for palette entries.
)docstring",
py::arg("palette"))
.def("setPalette",
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are some setPaletteIndex/getPaletteIndex methods in other classes. Perhaps do both?

[](SkFontArguments& self, int index) {
return self.setPalette(SkFontArguments::Palette({index, nullptr, 0}));
},
R"docstring(
Specify a palette index to use.
)docstring",
py::arg("index"))
.def("setVariationDesignPosition",
&SkFontArguments::setVariationDesignPosition,
R"docstring(
Expand All @@ -365,6 +439,7 @@ fontarguments
)docstring",
py::arg("position"))
.def("getCollectionIndex", &SkFontArguments::getCollectionIndex)
.def("getPalette", &SkFontArguments::getPalette)
.def("getVariationDesignPosition",
&SkFontArguments::getVariationDesignPosition)
;
Expand Down
42 changes: 42 additions & 0 deletions tests/test_font.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,44 @@ def test_FontArguments_setCollectionIndex(fontarguments):
fontarguments.setCollectionIndex(0)


Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there needs to be a dozen more tests for each of the new properties/methods of the new classes added.

@pytest.mark.parametrize(
"index, overrides",
[
(0, [(0, skia.ColorBLACK), (1, skia.ColorRED), (5, skia.ColorBLUE)]),
(1, []),
(2, None),
],
)
def test_FontArguments_setPalette(fontarguments, index, overrides):
if overrides is None:
palette = skia.FontArguments.Palette(index)
else:
palette = skia.FontArguments.Palette(
index,
skia.FontArguments.Palette.Overrides(
[
skia.FontArguments.Palette.Override(*override)
for override in overrides
]
),
)

len_overrides = len(overrides) if overrides is not None else 0
assert palette.index == index
assert len(palette.overrides) == len_overrides
assert palette.overrideCount == len_overrides
assert repr(palette) == f"Palette(index={index}, overrideCount={len_overrides})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably not have a repr test - since the __repr__ method itself is somewhat a personal choice and subjected to changes later too.

for i, override in enumerate(palette.overrides):
assert repr(override) == f"Override(index={override.index}, color={override.color})"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto on testing the precise form of repr outcome.

assert override.index == overrides[i][0]
assert override.color == overrides[i][1]
fontarguments.setPalette(palette)


def test_FontArguments_setPalette_Index(fontarguments):
fontarguments.setPalette(1)


def test_FontArguments_setVariationDesignPosition(fontarguments):
coordinates = skia.FontArguments.VariationPosition.Coordinates([
skia.FontArguments.VariationPosition.Coordinate(0x00, 0.),
Expand All @@ -58,6 +96,10 @@ def test_FontArguments_getCollectionIndex(fontarguments):
assert isinstance(fontarguments.getCollectionIndex(), int)


def test_FontArguments_getPalette(fontarguments):
assert isinstance(fontarguments.getPalette(), skia.FontArguments.Palette)


def test_FontArguments_getVariationDesignPosition(fontarguments):
assert isinstance(
fontarguments.getVariationDesignPosition(),
Expand Down
Loading