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

Raise a TypeError when Image() is called #7461

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Oct 13, 2023

Resolves #7455

When running

from PIL import Image
Image.Image()

this PR will raise a TypeError

Traceback (most recent call last):
  File "test.py", line 2, in <module>
    Image.Image()
  File "PIL/Image.py", line 488, in __init__
TypeError: Images should not be instantiated directly. Use the module new() function instead.

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)))
Copy link
Member

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.

Copy link
Member Author

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
Comment on lines 2941 to 2944
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
Copy link
Member

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

Copy link
Member Author

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().

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
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member Author

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.

@radarhere
Copy link
Member Author

@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?

@hugovk
Copy link
Member

hugovk commented Apr 1, 2024

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.

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.

Image.Image() == Image.Image() fails with AttributeError
3 participants