search

chinedufn / psd

A Rust API for parsing and working with PSD files.
https://chinedufn.github.io/psd
Apache License 2.0
265 stars 40 forks source link

Add Image resources section decoder #12

Closed tdakkota closed 4 years ago

tdakkota commented 4 years ago

Adds Image resources section decoder.

tdakkota commented 4 years ago

I think we should use this https://github.com/psd-tools/psd-tools/blob/master/tests/image_resources/slices_0.dat

chinedufn commented 4 years ago

I think that at the very least there should be one end to end test that confirms, with a real PSD file, that we properly extract data from the image resources section.

I also question the maintainability in working with data files over real PSD files.

For example, if in 6 months we realize that our image slice section is being parsed slightly incorrectly - I wouldn't want to be studying that .dat file to see what's wrong.

I would want to open a PSD file in Photoshop and compare what we're parsing to what Photoshop says is should look like.

I think that this is the nature of visual libraries such as a PSD parser.

Debugging pretty much requires that you can open something in Photoshop and visualize it and mess with it.

So, yeah I'm usually all for small unit tests that poke at one specific piece.

But I think that due to the PSD file spec being decades old and in some places under-documented there is no substitute (from a maintainability perspective) for real files when trying to build a reliable PSD parser.

Open to your thoughts on this though. Thanks for all of your effort!

tdakkota commented 4 years ago

I think we should use psd-tools as example to make decoder. There are 800+ tests, it's tested greatly. Some things are not documented in specification, but implemented in this package.

tdakkota commented 4 years ago

Also, I think that Psd should not parse image resources section as default, it should be optional.

chinedufn commented 4 years ago

Also, I think that Psd should not parse image resources section as default, it should be optional.

What's your reasoning for this?

chinedufn commented 4 years ago

I'd like to avoid landing any code that is untested - even if we just move it to a module commented out with a TODO to add unit tests for different descriptors over time.

Ah I'm sorry I misread the code. It looks like the psd file tests you add prove that we successfully export the descriptors.

Sorry - looks good!

tdakkota commented 4 years ago

What's your reasoning for this?

Image resource contains some metadata, which can be useless for some users, so why parse it, when we don't need it? Also, it is big enough part of file.

chinedufn commented 4 years ago

Image resource contains some metadata, which can be useless for some users, so why parse it, when we don't need it?

The one trade-off in my mind would be making the API more complex.

Now you need to know to set that flag if you want that information - and we need to maintain that.

That being said - that doesn't sound bad and if this data is rarely used then ... sure.

Cool! Thanks!

chinedufn commented 4 years ago

Everything is looking great.

Only thing I see remaining is the 0.dat 1.dat file names - mind giving those more descriptive names of what those files contain?

After that LGTM and we can merge!

chinedufn commented 4 years ago

Thanks for your awesome work - I've added you as a collaborator on psd. Feel free to merge when you're ready

image

tdakkota commented 4 years ago

Thanks for your awesome work - I've added you as a collaborator on psd. Feel free to merge when you're ready

Thank you a lot!

chinedufn commented 4 years ago

No problem! Anything blocking you from merging this?