image-js / image-js-typescript

Temporary repository to work on the migration of image-js to TypeScript
https://image-js.github.io/image-js-typescript/
MIT License
5 stars 5 forks source link

refactor: move automatic conversion from write to encode #466

Closed EscapedGibbon closed 2 months ago

EscapedGibbon commented 3 months ago

close: https://github.com/image-js/image-js-typescript/issues/464

EscapedGibbon commented 2 months ago

Have you checked code coverage to see if there are non-covered lines in the encode functions?

All the code is covered by tests. I removed the check on bit depth for png encoding, because once the mask changes color model, it automatically becomes 8 bit image.

EscapedGibbon commented 2 months ago

I don't know, however, if there is a need for a case for 1 bit in convertBitDepth function. Technically, the user can try to change bit depth to 1. But nothing will happen, if you run img = img.convertBitDepth(1). The image bit depth will not change. Should we add a throw to convertBitDepth if the image is not 8 or 16 bits?

stropitek commented 2 months ago

The image bit depth will not change. Should we add a throw to convertBitDepth if the image is not 8 or 16 bits?

I think in practice only Mask can have a bit depth of 1, not Image

EscapedGibbon commented 2 months ago

The image bit depth will not change. Should we add a throw to convertBitDepth if the image is not 8 or 16 bits?

I think in practice only Mask can have a bit depth of 1, not Image

I understand. But currently user can do something like this:

let image = new Image(4,4,{colorModel:"RGB"});
image = image.convertBitDepth(1);
//bit depth will remain 8

And there will be no warnings or throws. Normally, if a user wants a binary image, one will create a Mask, I get it. And such behavior doesn't break anything. But should we warn the user about it? Or is it an overkill?

targos commented 2 months ago

Good point. I think we should throw an error

stropitek commented 2 months ago

I understand. But currently user can do something like this:

let image = new Image(4,4,{colorModel:"RGB"});
image = image.convertBitDepth(1);
//bit depth will remain 8

I didn't know this was possible. I agree with @targos, this should throw.

EscapedGibbon commented 2 months ago

I understand. But currently user can do something like this:

let image = new Image(4,4,{colorModel:"RGB"});
image = image.convertBitDepth(1);
//bit depth will remain 8

I didn't know this was possible. I agree with @targos, this should throw.

Should I create separate issue or fix it in this PR?

stropitek commented 2 months ago

Should I create separate issue or fix it in this PR?

You can fix this here with a test.