Skip to content

Conversation

@CharlieS1103
Copy link

Note: I'm not that experienced in rust (this is essentially my first real project in the language) so let me know if I did anything wrong.
Also, I believe that the test case is in the jp2000, however, it is different than all the other test cases, I simply have no idea how to create my own .icns file containing a jp2k checkmark.

Copy link
Owner

@mdsteele mdsteele left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like a good start! I left a few initial comments for now.

@0rvar
Copy link

0rvar commented Mar 14, 2024

Any way I can help land this PR?

@CharlieS1103
Copy link
Author

@0rvar For sure, I think everything i've added so far has some major conceptual errors, and I'd still like to get this running smoothly so my current project can function without hiccups, if you have any ideas please feel free to throw them in!

@0rvar
Copy link

0rvar commented Mar 19, 2024

I've gotten jpeg2000 support working for my own test cases in my own fork, I have opened a chained PR to this branch in the fork:

CharlieS1103#1

@CharlieS1103 CharlieS1103 requested a review from mdsteele August 29, 2024 17:54
let width = icon_type.pixel_width();
let height = icon_type.pixel_width();
match icon_type.encoding() {
#[cfg(feature = "pngio")]
Copy link
Owner

Choose a reason for hiding this comment

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

This line needs to be restored so that the crate can still build with the pngio feature disabled.

decoder_test("loom.icns", IconType::RGBA32_512x512, "loom-512x512.png");
}


Copy link
Owner

Choose a reason for hiding this comment

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

Minor style nitpick: Please remove the extra lines here (just one blank line between functions).

}

#[test]
fn decode_ic08(){
Copy link
Owner

Choose a reason for hiding this comment

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

This test seems to be failing for me when I run it locally. (Hmm, looks like I need to set up CI for this repo.)

Copy link
Author

Choose a reason for hiding this comment

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

I'm not totally sure why, given the JP2000 decoding is working (which I'm not totally sure of as I tested it with a jp2000 file which I created of type ic10 and it didn't seem to function)
I believe that the test should've passed but I also have not spent much time in this repo overall.

Copy link
Owner

Choose a reason for hiding this comment

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

I just now enabled CI testing for this repo. If you rebase, we should be able to see the results of that.

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