Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG #17337

Merged
merged 1 commit into from Mar 14, 2019

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Mar 11, 2019

Description of Change

Required to safely serialize/deserialize NativeImage instances to be sent over IPC in #17200

/cc @nornagon

Checklist

Release Notes

Notes: Added safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 11, 2019
@miniak miniak force-pushed the miniak/native-image-bitmap branch 2 times, most recently from 8c4f6bb to de3e18f Compare March 11, 2019 19:41
@miniak miniak self-assigned this Mar 11, 2019
@miniak miniak changed the title feat: add safer nativeImage.createFromBitmap(), which does not decode PNGs feat: add safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG Mar 11, 2019
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

IMO we should deprecate createFromBuffer, and replace it with createFromBitmap (or maybe createFromRGBA), createFromPNG and createFromJPEG. Just guessing at the image format isn't really a sane way to do it.

docs/api/native-image.md Outdated Show resolved Hide resolved
docs/api/native-image.md Outdated Show resolved Hide resolved
@miniak miniak force-pushed the miniak/native-image-bitmap branch 3 times, most recently from 3e03037 to df45afd Compare March 11, 2019 23:13
@miniak
Copy link
Contributor Author

miniak commented Mar 11, 2019

@nornagon I was just thinking about doing that. I agree that having separate createFromPNG and createFromJPEG is cleaner. I will do that in a follow up PR. What about addRepresentation? That one has the same issues.

@MarshallOfSound
Copy link
Member

I'm thinking we should probably have some kind of "guess what this is" method. I can see it being handy when you think a file is an image but don't know what format it is.

@nornagon
Copy link
Member

I think that falls outside the scope of Electron. See e.g. http://www.darwinsys.com/file/

@MarshallOfSound
Copy link
Member

@nornagon At least pointing people to that / an npm module for file type detection would be enough. I just don't want to say "figure it out have fun" 👍

docs/api/native-image.md Outdated Show resolved Hide resolved
atom/common/api/atom_api_native_image.cc Outdated Show resolved Hide resolved
@miniak miniak force-pushed the miniak/native-image-bitmap branch from db6d05a to ad99925 Compare March 12, 2019 08:14
@miniak miniak requested a review from nornagon March 12, 2019 08:14
@sindresorhus
Copy link
Contributor

Just guessing at the image format isn't really a sane way to do it.

It's not guessing, it's detecting, and I think it should stay. It's a nice convenience. I agree there should also be explicit methods for JPEG and PNG.

@sindresorhus
Copy link
Contributor

At least pointing people to that / an npm module for file type detection would be enough. I just don't want to say "figure it out have fun" 👍

My file-type package is the most popular one.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 12, 2019
@miniak miniak force-pushed the miniak/native-image-bitmap branch from ad99925 to 019fcf8 Compare March 13, 2019 18:50
@nornagon nornagon merged commit 878538f into master Mar 14, 2019
@release-clerk
Copy link

release-clerk bot commented Mar 14, 2019

Release Notes Persisted

Added safer nativeImage.createFromBitmap(), which does not decode PNG/JPEG.

@miniak miniak deleted the miniak/native-image-bitmap branch March 14, 2019 18:20
kiku-jw pushed a commit to kiku-jw/electron that referenced this pull request May 16, 2019
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.

None yet

4 participants