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.

Sign up for GitHub

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

Jump to bottom

Ensure file is closed if it is opened by ImageQt.ImageQt #5260

Merged
merged 1 commit into from Mar 8, 2021
Merged

Ensure file is closed if it is opened by ImageQt.ImageQt #5260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions Tests/test_imageqt.py
View file
Open in desktop
Expand Up @@ -4,11 +4,14 @@

from .helper import hopper

pytestmark = pytest.mark.skipif(
not ImageQt.qt_is_installed, reason="Qt bindings are not installed"
)

if ImageQt.qt_is_installed:
from PIL.ImageQt import qRgba


@pytest.mark.skipif(not ImageQt.qt_is_installed, reason="Qt bindings are not installed")
def test_rgb():
# from https://doc.qt.io/archives/qt-4.8/qcolor.html
# typedef QRgb
Expand Down Expand Up @@ -38,7 +41,13 @@ def checkrgb(r, g, b):
checkrgb(0, 0, 255)


@pytest.mark.skipif(not ImageQt.qt_is_installed, reason="Qt bindings are not installed")
def test_image():
for mode in ("1", "RGB", "RGBA", "L", "P"):
ImageQt.ImageQt(hopper(mode))


def test_closed_file():
with pytest.warns(None) as record:
ImageQt.ImageQt("Tests/images/hopper.gif")

assert not record
Copy link
Member

@hugovk hugovk Mar 7, 2021 •
edited

Choose a reason for hiding this comment

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

Do you know why this is missing coverage? We should have some Qt at least on some CI?

(Similarly for #5260)

Copy link
Contributor

@nulano nulano Mar 7, 2021 •
edited

Choose a reason for hiding this comment

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

It looks like Codecov only received AppVeyor coverage for this PR: https://codecov.io/gh/python-pillow/Pillow/commit/254973f8a035275c342e408e6ec883e5b2d2ee47/build

There is PyQt5 in the Arch container (among others), its coverage is reported here: https://codecov.io/github/python-pillow/Pillow/commit/d5f5ae68a8f758e53188903a01f43f693bff3220

There are some errors reported:

https://github.com/python-pillow/Pillow/pull/5260/checks?check_run_id=1933751967#step:8:21

==> GitHub Actions detected.
->  Issue detecting commit SHA. Please run actions/checkout with fetch-depth > 1 or set to 0
    project root: .
    Yaml found at: codecov.yml

https://github.com/python-pillow/Pillow/pull/5260/checks?check_run_id=1933751943#step:15:763

->  Pinging Codecov
https://codecov.io/upload/v4?package=bash-20210129-7c25fce&token=secret&branch=imageqt_exclusive_fp&commit=d5f5ae68a8f758e53188903a01f43f693bff3220&build=580803277&build_url=http%3A%2F%2Fgithub.com%2Fpython-pillow%2FPillow%2Factions%2Fruns%2F580803277&name=ubuntu-latest%20Python%203.9&tag=&slug=python-pillow%2FPillow&service=github-actions&flags=GHA_Ubuntu&pr=5260&job=Test&cmd_args=F
{'detail': ErrorDetail(string='Actions workflow run is stale', code='not_found')}
404

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 tried rebasing in hopes of getting more reports, but I only got one more.

Copy link
Member

@hugovk hugovk Mar 8, 2021 •
edited

Choose a reason for hiding this comment

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

Well we get some coverage here now: https://codecov.io/gh/python-pillow/Pillow/src/dc0db43d35b081ad5f0f6266be79c0355d284f3b/Tests/test_imageqt.py

and https://codecov.io/gh/python-pillow/Pillow/commit/dc0db43d35b081ad5f0f6266be79c0355d284f3b/build shows a load of GHA builds too. Good enough for this PR, let's keep an eye on Codecov... Thanks!

19 changes: 13 additions & 6 deletions src/PIL/ImageQt.py
View file
Open in desktop
Expand Up @@ -128,13 +128,15 @@ def align8to32(bytes, width, mode):
def _toqclass_helper(im):
data = None
colortable = None
exclusive_fp = False

# handle filename, if given instead of image name
if hasattr(im, "toUtf8"):
# FIXME - is this really the best way to do this?
im = str(im.toUtf8(), "utf-8")
if isPath(im):
im = Image.open(im)
exclusive_fp = True

qt_format = QImage.Format if qt_version == "6" else QImage
if im.mode == "1":
Expand All @@ -157,10 +159,15 @@ def _toqclass_helper(im):
data = im.tobytes("raw", "BGRA")
format = qt_format.Format_ARGB32
else:
if exclusive_fp:
im.close()
raise ValueError(f"unsupported image mode {repr(im.mode)}")

__data = data or align8to32(im.tobytes(), im.size[0], im.mode)
return {"data": __data, "im": im, "format": format, "colortable": colortable}
size = im.size
__data = data or align8to32(im.tobytes(), size[0], im.mode)
if exclusive_fp:
im.close()
return {"data": __data, "size": size, "format": format, "colortable": colortable}


if qt_is_installed:
Expand All @@ -182,8 +189,8 @@ def __init__(self, im):
self.__data = im_data["data"]
super().__init__(
self.__data,
im_data["im"].size[0],
im_data["im"].size[1],
im_data["size"][0],
im_data["size"][1],
im_data["format"],
)
if im_data["colortable"]:
Expand All @@ -197,8 +204,8 @@ def toqimage(im):
def toqpixmap(im):
# # This doesn't work. For now using a dumb approach.
# im_data = _toqclass_helper(im)
# result = QPixmap(im_data['im'].size[0], im_data['im'].size[1])
# result.loadFromData(im_data['data'])
# result = QPixmap(im_data["size"][0], im_data["size"][1])
# result.loadFromData(im_data["data"])
# Fix some strange bug that causes
if im.mode == "RGB":
im = im.convert("RGBA")
Expand Down