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

Allow tuples with one item to give single color value in getink #4927

Merged
merged 3 commits into from Oct 12, 2020

Conversation

radarhere
Copy link
Member

Resolves #4925

Normally, a user would run Image.new('F', (100,100), 255) and Image.new('I', (100,100), 255), providing a single value for color for those modes.

This PR allows users to also provide that single value inside a tuple, Image.new('F', (200,100), (255,)) and Image.new('I', (200,100), (255,)), which would have previously thrown an error.

src/_imaging.c Outdated Show resolved Hide resolved
radarhere and others added 2 commits September 22, 2020 08:36
Co-authored-by: nulano <nulano@nulano.eu>
Copy link
Contributor

@nulano nulano left a comment

Choose a reason for hiding this comment

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

I think the current version will accept a tuple-in-tuple for L and RGB modes, LGTM otherwise.

@@ -330,21 +330,22 @@ def test_p_putpixel_rgb_rgba(self):
class TestImagePutPixelError(AccessTest):
IMAGE_MODES1 = ["L", "LA", "RGB", "RGBA"]
IMAGE_MODES2 = ["I", "I;16", "BGR;15"]
INVALID_TYPES1 = ["foo", 1.0, None]
INVALID_TYPES2 = [*INVALID_TYPES1, (10,)]
INVALID_TYPES = ["foo", 1.0, None]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
INVALID_TYPES = ["foo", 1.0, None]
INVALID_TYPES = ["foo", 1.0, None, ((10,),)]

Tuple can be accepted, but probably not tuple-in-tuple. Maybe test for that as well? Or is that too much of an edge-case?

Comment on lines +521 to +523
if (PyTuple_Check(color) && PyTuple_Size(color) == 1) {
color = PyTuple_GetItem(color, 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this currently makes L mode accept tuple(tuple(10)) and RGB mode accept tuple(tuple(255,255,255)), which is probably not the intended behaviour.

Suggested change
if (PyTuple_Check(color) && PyTuple_Size(color) == 1) {
color = PyTuple_GetItem(color, 0);
}
/* IMAGING_TYPE_UINT8 unpacks tuples later */
if (im->type != IMAGING_TYPE_UINT8 && PyTuple_Check(color) && PyTuple_Size(color) == 1) {
color = PyTuple_GetItem(color, 0);
}

@hugovk hugovk merged commit 309cb9e into python-pillow:master Oct 12, 2020
@radarhere radarhere deleted the tuple branch October 12, 2020 11:41
@NightMachinery
Copy link

NightMachinery commented Aug 18, 2021

My code broke, probably because of this PR:

image = PIL.ImageOps.pad(
	image,
	(max(image.width, min_width), max(image.height, min_height)),
	color=(255, 255, 255),
)

How do you now provide an RGB tuple?

@radarhere
Copy link
Member Author

I don't think this PR should have broken your code. What mode is your image? What error are you receiving?

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.

SystemError on Image.new / fill for 32-bit mode with color tuple
4 participants