Add bitmap support to nativeImage.createFromBuffer #8175
Conversation
| @@ -137,15 +137,17 @@ let image = nativeImage.createFromPath('/Users/somebody/images/icon.png') | |||
| console.log(image) | |||
| ``` | |||
|
|
|||
| ### `nativeImage.createFromBuffer(buffer[, scaleFactor])` | |||
| ### `nativeImage.createFromBuffer(buffer[width, height, scaleFactor])` | |||
There was a problem hiding this comment.
buffer[width -> buffer[, width
|
|
||
| * `buffer` [Buffer][buffer] | ||
| * `width` Integer (optional) |
There was a problem hiding this comment.
This argument ordering makes this a breaking change. I.e. Users currently using scaleFactor will have to change their function call to make it keep working.
The new argument order should be buffer, scaleFactor, width, height which then means it remains backwards compatible.
There was a problem hiding this comment.
@MarshallOfSound I already wrote this but if you take a look at line 411, it seems to be backwards compatible.
There was a problem hiding this comment.
Hmmm, the length check does appear to make it backwards compatible. Still not too comfortable with the args ordering as without looking at the implementation in a lot of depth it looks breaking 😆
Fine with it though if it isn't actually breaking
There was a problem hiding this comment.
@MarshallOfSound Yeah. I thought that too but then I saw the length check 😄
There was a problem hiding this comment.
@MarshallOfSound yeah it doesn't break, but it does look a bit weird. I thought about putting it at the end, but then the scale factor always has to be specified for bitmap buffers.
There was a problem hiding this comment.
What do you think about making this new version createFromBuffer(buffer, options) and width, height and scaleFactor can be specified in there?
Then it is easier to add new keys/options in the future and we can just try parsing the second argument as an object and if it fails we fall back to a scale factor second argument with a default width/height.
There was a problem hiding this comment.
That way it is a bit easier to read as well:
nativeImage.createFromBuffer(buffer, 3, 2, 1)versus:
nativeImage.createFromBuffer(buffer, {
width: 3,
height: 2,
scaleFactor: 1
})There was a problem hiding this comment.
@kevinsawicki I like it, will fix it tomorrow! 👍
Fine with it as long as it is not breaking
|
As @kevinsawicki suggested, I moved the |
| @@ -25,6 +25,10 @@ describe('nativeImage module', () => { | |||
|
|
|||
| const imageC = nativeImage.createFromBuffer(imageA.toJPEG(100)) | |||
| assert.deepEqual(imageC.getSize(), {width: 538, height: 190}) | |||
|
|
|||
| const imageD = nativeImage.createFromBuffer(imageA.toBitmap(), | |||
| {width: 538, height: 190}) | |||
There was a problem hiding this comment.
Looks like this image is initially 538x190 so could this spec use different values for the specified width/height to ensure a different size is returned?
There was a problem hiding this comment.
Do you mean to test for a different image or to create a falsy assertion with wrong width/height values?
There was a problem hiding this comment.
Yeah, what happens if the specified width/height doesn't match the bitmap? Does it fail to return it? Or return an empty buffer? Or something else?
Would be good to cover this case to know the behavior.
There was a problem hiding this comment.
I added some tests for bitmaps and the other types. PNG and JPEG is not affected by width/height, because the size can be decoded from the buffer, but bitmaps don't have that so they always have the same size, that is specified. That said they won't crash when a wrong size is passed, just the picture will be messed up.
|
Thanks for this @gerhardberger 👍 🚢 |
This PR adds support to
nativeImage.createFromBufferfor creating images from bitmap buffers. #7950We cannot obtain size from the buffer, it has to be supplied as well, so I added two optional parameters to the method.