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 png chunk reader, expose more of codec and bitmap #88

Open
wants to merge 13 commits into
base: xamarin-mobile-bindings
Choose a base branch
from

Conversation

mgood7123
Copy link

No description provided.

Copy link

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Very nice PR. All this looks good. I do have some suggestions and also a couple of nitpicks.

Mainly some properties are just named differently or return a higher level object in C#, so either we can add a new property to do this Size => Info.Size or we can use the existing property.

Then the last nit is the underscores in the class names. My generator is not very smart. It often gets things wrong.

BUILD.gn Outdated Show resolved Hide resolved
src/c/sk_bitmap.cpp Show resolved Hide resolved
src/c/sk_bitmap.cpp Outdated Show resolved Hide resolved
include/c/sk_codec.h Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
src/c/sk_codec.cpp Outdated Show resolved Hide resolved
src/xamarin/SkManaged_Png_Chunk_Reader.cpp Outdated Show resolved Hide resolved
src/xamarin/sk_managed_png_chunk_reader.cpp Outdated Show resolved Hide resolved
@mgood7123
Copy link
Author

Very nice PR. All this looks good. I do have some suggestions and also a couple of nitpicks.

Mainly some properties are just named differently or return a higher level object in C#, so either we can add a new property to do this Size => Info.Size or we can use the existing property.

Then the last nit is the underscores in the class names. My generator is not very smart. It often gets things wrong.

alright :)

My generator is not very smart. It often gets things wrong.

haha :)

@mgood7123 mgood7123 force-pushed the png_chunk_reader_codec_bitmap branch from 38534bb to 56e7c42 Compare July 1, 2022 04:09
include/c/sk_types.h Outdated Show resolved Hide resolved
BUILD.gn Outdated Show resolved Hide resolved
src/c/sk_bitmap.cpp Show resolved Hide resolved
src/xamarin/SkiaKeeper.c Outdated Show resolved Hide resolved
Copy link

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I think the find/replace may have been too powerful, so the google code needs to be reverted a bit.

Also, we need a enum mapping like this to protect against the Google Unnecessary Reorder ™️ that will take place when we update: https://github.com/mono/skia/blob/xamarin-mobile-bindings/src/c/sk_enums.cpp#L244-L246

include/codec/SkCodec.h Outdated Show resolved Hide resolved
src/codec/SkCodec.cpp Outdated Show resolved Hide resolved
@mgood7123
Copy link
Author

I think the find/replace may have been too powerful, so the google code needs to be reverted a bit.

Also, we need a enum mapping like this to protect against the Google Unnecessary Reorder ™️ that will take place when we update: https://github.com/mono/skia/blob/xamarin-mobile-bindings/src/c/sk_enums.cpp#L244-L246

fixed for now :)

@mattleibow
Copy link

The only thing left is the enum added to sk_enums.cpp

@mgood7123
Copy link
Author

The only thing left is the enum added to sk_enums.cpp

which one?

@mgood7123
Copy link
Author

mgood7123 commented Jul 12, 2022

The only thing left is the enum added to sk_enums.cpp

like this?

// sk_codec_selection_policy_t
static_assert ((int)SkCodec::SelectionPolicy::kPreferStillImage   == (int)PREFER_STILL_IMAGE_SK_CODEC_SELECTION_POLICY,   ASSERT_MSG(SkCodec::SelectionPolicy, sk_codec_selection_policy_t));
static_assert ((int)SkCodec::SelectionPolicy::kPreferAnimation    == (int)PREFER_ANIMATION_SK_CODEC_SELECTION_POLICY,     ASSERT_MSG(SkCodec::SelectionPolicy, sk_codec_selection_policy_t));

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