Fixed handling of NSImage by cocoa Image backend
#2355
Conversation
There was a problem hiding this comment.
If this problem affects Cocoa, it very likely affects iOS as well.
This definitely needs a test of some kind. In this case, the issue is being caused when an image is created with the native image type, and then being converted to data. That's a test you can write in a completely platform independent way by leveraging ._impl.native to get the native object.
testbed/tests/test_images.py
Outdated
| buffer = io.BytesIO() | ||
| pil_image.save(buffer, format="png", compress_level=0) | ||
|
|
||
| # Construct a Toga image from PIL image | ||
| image1 = toga.Image(buffer.getvalue()) |
There was a problem hiding this comment.
This step shouldn't be needed - Toga can accept PIL images directly as input (and this would be a good in-practice test of that).
There was a problem hiding this comment.
Also - more meaningful variable names wouldn't hurt; orig_image instead of image1 in this case.
testbed/tests/test_images.py
Outdated
|
|
||
|
|
||
| async def test_native_image(app): | ||
| # Generate an image using pillow |
There was a problem hiding this comment.
As I've noted in previous reviews, every test should start with a 1 line docstring that describes the purpose of the test.
testbed/tests/test_images.py
Outdated
| assert image_from_native.height == 30 | ||
|
|
||
| # Construct an image from `image_from_native`'s data | ||
| image2 = toga.Image(image_from_native.data) |
There was a problem hiding this comment.
Again; variable naming - image_from_data maybe?
Since, this bug was caused when the data was requested from the image created with the native image, this test validates that the data from an image created from the raw image representation is valid. Both of the tests, But the behaviors tested by |
There was a problem hiding this comment.
As a general guide, there needs to be a very good reason to remove a test - if a test was previously passing, that's a code usage that is known to be good, and needs to stay that way. Unless you can give a good rationale for why a new test makes an old test unnecessary, you shouldn't drop tests.
In this case, the test that has been deleted is a different use case - and, more importantly, it's the fact that the use case is different that was the problem. Under Cocoa, creating an image from a PNG source is apparently different to creating it from a buffer/native source - so we need to test that discrepancy.
testbed/tests/test_images.py
Outdated
| @@ -22,16 +22,6 @@ async def test_local_image(app): | |||
| assert image.height == 72 | |||
|
|
|||
|
|
|||
| async def test_raw_image(app): | |||
There was a problem hiding this comment.
Presumably we still need to test this case - it was valid previously, and clearly has different semantics on some platforms.
There was a problem hiding this comment.
I have added it back.
On second thought, the test does seems to test a different thing. But it felt very similar to |
The
toga.Image()class should be able to handle aNSImage. But the current implementation is non functional and would pass aNoneTypeobject tonsdata_to_bytes, when aNSImageis passed.This PR fixes this bug and ensures that a valid
NSDatais passed tonsdata_to_bytesThis bug was discovered during the implementation of #1930 and since that PR would be undergoing more reviews before getting merged, I figured this bugfix should be a separate PR.
The latest commit on #1930 is the validity of the correct working of this PR. Locally tested on MacOS.
PR Checklist: