-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Raise a TypeError when Image() is called #7461
base: main
Are you sure you want to change the base?
Conversation
src/PIL/Image.py
Outdated
@@ -980,7 +991,7 @@ def convert_transparency(m, v): | |||
else: | |||
# get the new transparency color. | |||
# use existing conversions | |||
trns_im = Image()._new(core.new(self.mode, (1, 1))) | |||
trns_im = Image._init()._new(core.new(self.mode, (1, 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.
Image._init()._new()
is quite common and very ugly. In this case we don't want to create a new image second time just to copy empty info and palette properties.
I suggest following:
im._new(core.new())
— creates image like im, copying info and palette. I'm not sure how often this method is used this way.
Image._init(core.new())
— creates empty image using only core image object.
The only place I see where we need to create empty Image object (without core object) is new()
function, but it could be easily avoided.
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 cherry-picked #7460 here, since that helps.
I've pushed a commit with a version of your suggestion. It's not exact, because it turns out that _new()
doesn't just copy existing palettes, it also creates new ones. It's not enough to just attach a palette afterwards in new()
, because something like Image.linear_gradient("P")
also needs a palette.
src/PIL/Image.py
Outdated
im = Image._init() | ||
if mode == "P" and isinstance(color, (list, tuple)) and len(color) in [3, 4]: | ||
# RGB or RGBA value for a P image | ||
from . import ImagePalette |
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.
palette = None
if mode == "P" and isinstance(color, (list, tuple)) and len(color) in [3, 4]:
# RGB or RGBA value for a P image
from . import ImagePalette
palette = ImagePalette.ImagePalette()
color = palette.getcolor(color)
im = Image._init(core.fill(mode, size, color))
im.palette = palette
return im
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.
Per my last comment, _init(im)
needs to create a palette for P mode images, which might lead to two ImagePalettes being created here.
However, we don't need getcolor()
to know what the color index should be - it is always going to be zero, as this is the first color in the palette. Taking advantage of that, we can just re-use the palette from _init()
.
src/PIL/Image.py
Outdated
im.palette = ImagePalette.ImagePalette() | ||
color = im.palette.getcolor(color) | ||
return im._new(core.fill(mode, size, color)) | ||
# This will be the first color allocated to the palette |
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.
Why we need to make this assumption rather than just using existing API?
We can rewrite palette if it is exist:
im = Image._init(core.fill(mode, size, color))
if palette:
im.palette = palette
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.
Hmm, I understand the desire to use APIs. My original thought was to try and avoid constructing a second imagePalette, but this version of the code seems simpler to me. I don't think it should ever change that a palette starts out empty, and that the first color is at the zero index.
Are you sure using getcolor()
is better?
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.
@homm did my argument persuade you, or no? I can change to your suggestion if you like, I'd rather progress this than let it sit unresolved.
@hugovk is there any particular problem with this? My guess would be that it is perhaps too complex of a change for an edge case, and the user in question should just use our API as intended? |
I was waiting to see if @homm had more to say (re: #7461 (comment)). But it does seem like a edge case (they're getting an error calling the API in the wrong way, and this changes it to another error) and I don't think this has come up before, so I'm leaning towards the status quo. |
8f1157a
to
1b92e78
Compare
Resolves #7455
When running
this PR will raise a TypeError