Skip to content

Support CPAL table #841

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

Merged
merged 4 commits into from
Feb 28, 2018
Merged

Support CPAL table #841

merged 4 commits into from
Feb 28, 2018

Conversation

ebraminio
Copy link
Collaborator

@ebraminio ebraminio commented Feb 27, 2018

In continuation to and closes #244, addressed some of the review there and (unsure about if hb_ot_color_get_palette_colors is best API we can provide?) however the related test fails. I want to see if my changes are in right direction? (making fields private, doing checks at the related places, ...)

brawer and others added 3 commits June 19, 2016 13:17
The name id 0 is used as Copyright notice. It's quite unlikely that a
font supplies a color palette with the exact same name as the font's
copyright notice, but the API should not prevent this.

Also, try to fix a problem with GObject introspection, where the
auto-generated Python bindings could not return palette colors.
@ebraminio ebraminio force-pushed the color branch 4 times, most recently from 72e726d to 991b716 Compare February 27, 2018 19:36
if (paletteFlags.is_null ())
return HB_OT_COLOR_PALETTE_FLAG_DEFAULT;

const HBUINT32* flags = (const HBUINT32 *) (const void*) &paletteFlags (base);
Copy link
Collaborator Author

@ebraminio ebraminio Feb 27, 2018

Choose a reason for hiding this comment

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

I wish I could use UnsizedArrayOf... not sure if this is correct

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, will move UnsizedArrayOf to hb-open-type.hh some time.

@behdad
Copy link
Member

behdad commented Feb 27, 2018

Ok let me finish the other stuff. If this is clean and adds no API for now, feel free to merge.

@ebraminio ebraminio force-pushed the color branch 2 times, most recently from d675cd0 to c7aac87 Compare February 27, 2018 21:53
@ebraminio
Copy link
Collaborator Author

ebraminio commented Feb 27, 2018

I improved it as far as I could from the things learned from the previous experiences, like sanitizing the arrays, bringing the logic on the right place and making data private, however in the process, I was careful about not breaking the tests so hopefully it won't give you headache when you want use the API or not before disabling the tests altogether. However I didn't refactor hb_ot_color_get_palette_colors as I wasn't sure if that is right abstraction to have, but I can understand its performance benefits of course.

I think Wine implementation is a good place to be inspired about what would be needed for a real implementation to be exposed, https://github.com/wine-mirror/wine/blob/5ec6b8f807f61ee77b9a96d94798c8e3f3db7af4/dlls/dwrite/dwrite_private.h#L220-L238 (and other related places on the repo)

@behdad
Copy link
Member

behdad commented Feb 27, 2018

Thanks. Let me finish BASE (while also keeping eye on subsetter, and redoing fonttools table packing) and aat, then review this.

@ebraminio ebraminio force-pushed the color branch 7 times, most recently from cdc16e2 to 4490d62 Compare February 28, 2018 07:42
@ebraminio
Copy link
Collaborator Author

ebraminio commented Feb 28, 2018

Ok, this adds no API now (except the some enums and structs which I guess are not issue) and is clean (as far as I could, and to idea is to learn from your touch ups :) ), so lets have it as your previous suggestion.

@ebraminio ebraminio merged commit 7722746 into harfbuzz:master Feb 28, 2018
@ebraminio ebraminio deleted the color branch February 28, 2018 08:35
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.

3 participants