-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Added support for Windows 1.0 icons #7427
Conversation
Check out the samples as device dependent and independent in #7320 |
Looking at "DEVDEP.ICO", it's actually not device-dependent, but in "both" formats. I presume that it is also able to be added to our test suite, and distributed under the Pillow license? I've added that here as a test image for the "both" format, and manually changed the image I already had to register as device-dependent. So this PR now reads device-independent, device-dependent, "both", and saves in device-independent format. |
Yes, you can add those icons of course to the test suite under any license. They are just randomly generated pictures trying to include all bitmaps, white, black and transparent. Not sure if the format could be generated more complicated or complete that that to include all use cases. I encourage anybody to try with |
Ah, this duplicates some of the work I'd done in #6965... :( Anyway, if this eventually gets merged, I suppose I'll rebase my work on this. |
src/PIL/IcoImagePlugin.py
Outdated
bitmapLength = self.state.xsize * self.state.ysize // 8 | ||
firstBitmap = self.fd.read(bitmapLength) | ||
secondBitmap = self.fd.read(bitmapLength) | ||
for i, byte in enumerate(firstBitmap): | ||
secondByte = byte ^ secondBitmap[i] | ||
for j in reversed(range(8)): | ||
first = byte >> j & 1 | ||
second = secondByte >> j & 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snake_case
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit for this.
src/PIL/IcoImagePlugin.py
Outdated
firstBitmap = bytearray() | ||
secondBitmap = bytearray() | ||
w, h = self.im.size | ||
for y in range(h): | ||
for x in range(w): | ||
l, a = self.im.getpixel((x, y)) | ||
if x % 8 == 0: | ||
firstBitmap += b"\x00" | ||
secondBitmap += b"\xff" | ||
if not a: | ||
firstBitmap[-1] ^= 1 << (7 - x % 8) | ||
if not l: | ||
secondBitmap[-1] ^= 1 << (7 - x % 8) | ||
data = firstBitmap + secondBitmap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
snake_case
? Also, l
and a
are somewhat opaque to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed commits to update the variable names. It is an LA image being saved, so l
is the L channel, and a
the A channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the master branch is already patched, or we must wait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand you correctly, this PR has not been merged into Pillow's main branch, no. It is on hold because there have been concerns raised about whether this really needs to be part of Pillow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible to make it a plugin? Are plugins easy to install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you just want to add it your own code, here is a separate plugin derived from this PR - Ico1ImagePlugin.py.zip
Unzip it, and then you can
from PIL import Image
import Ico1ImagePlugin
im = Image.open("ico1both.ico")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be useful in Pillow – people aren't going to find external plugins when they need them, so they'll end up writing the same code again, and that's no fun for anyone...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've merged new features for less good reasons so I'd like to see this go in. Worst case it comes out again in the future? 🤷
ImageMagick apparently added support for it but doesn't work: ImageMagick/ImageMagick#6866 |
Closing due to lack of interest. |
//TODO |
@superbonaci if that is a note to yourself, ok, but the intention from our side is that this is not something that will be done. |
Resolves #7320 by adding support for reading and writing device-independent Windows 1.0 icons.
http://fileformats.archiveteam.org/wiki/Windows_1.0_Icon was used as a reference for the specification.
Because these images contains white, black and transparent pixels, I chose LA for the mode.
The test image was created by the user in the issue.
Because .ico is already taken, the format needs to be passed in as an argument when saving.