Skip to content

Add a unit test for drawing images#572

Merged
jwiggins merged 2 commits into
masterfrom
test/draw-image
Feb 5, 2021
Merged

Add a unit test for drawing images#572
jwiggins merged 2 commits into
masterfrom
test/draw-image

Conversation

@jwiggins

@jwiggins jwiggins commented Feb 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

NOTE: I expect this to fail until #569 is merged. This is due to broken implementations of draw_image in the PS and SVG backends.

======================================================================
ERROR: test_image (kiva.tests.test_ps_drawing.TestPSDrawing)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/.edm/envs/enable-test-3.6-null/lib/python3.6/site-packages/kiva/tests/drawing_tester.py", line 33, in test_image
    self.gc.draw_image(img, (100, 100, 20, 20))
  File "/home/runner/.edm/envs/enable-test-3.6-null/lib/python3.6/site-packages/kiva/basecore2d.py", line 856, in draw_image
    self.device_draw_image(img, rect)
  File "/home/runner/.edm/envs/enable-test-3.6-null/lib/python3.6/site-packages/kiva/ps.py", line 231, in device_draw_image
    if width != img.width() or height != img.height():
AttributeError: 'numpy.ndarray' object has no attribute 'width'

======================================================================
ERROR: test_image (kiva.tests.test_svg_drawing.TestSVGDrawing)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/.edm/envs/enable-test-3.6-null/lib/python3.6/site-packages/kiva/tests/drawing_tester.py", line 33, in test_image
    self.gc.draw_image(img, (100, 100, 20, 20))
  File "/home/runner/.edm/envs/enable-test-3.6-null/lib/python3.6/site-packages/kiva/basecore2d.py", line 856, in draw_image
    self.device_draw_image(img, rect)
  File "/home/runner/.edm/envs/enable-test-3.6-null/lib/python3.6/site-packages/kiva/svg.py", line 284, in device_draw_image
    if width != img.width() or height != img.height():
AttributeError: 'numpy.ndarray' object has no attribute 'width'

@jwiggins jwiggins mentioned this pull request Feb 5, 2021
Merged
rahulporuri
rahulporuri approved these changes Feb 5, 2021
View reviewed changes

@rahulporuri rahulporuri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with a few questions.

Comment thread kiva/pdf.py

if rect is None:
rect = (0, 0, img.width(), img.height())
rect = (0, 0, pil_img.width, pil_img.height)

@rahulporuri rahulporuri Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sigh. I should've caught this in the earlier PR.

@jwiggins jwiggins Feb 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No worries. That's what test coverage is for.

Comment thread kiva/tests/test_ps_drawing.py
if not any((line.endswith("fill"),
line.endswith("stroke"),
line.endswith("cliprestore"),
line.endswith("grestore"),

@rahulporuri rahulporuri Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure what this special incantation is either.

@jwiggins jwiggins Feb 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is what shows up in the postscript output when you draw an image.

Comment thread kiva/tests/test_pdf_drawing.py
# been drawn.
line = content.getData().splitlines()[-2]
if not any((line.endswith(b'f'),
line.endswith(b'Q'),

@rahulporuri rahulporuri Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure what this special incantation is.

@jwiggins jwiggins Feb 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is what it in the PDF when you draw an image.

Comment thread kiva/ps.py
Comment on lines +243 to +245
fp = BytesIO()
pil_img.save(fp, "eps", eps=0)
self.contents.write(fp.getvalue().decode('utf8'))

@rahulporuri rahulporuri Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure why we are saving the image to a BytesIO and then decoding it + writing it to self.contents - instead of saving it directly to self.contents.

Is this just best practice or is this preventing any possible issues?

@jwiggins jwiggins Feb 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

self.contents is a StringIO and Image.save will fail if you pass it in because this isn't Python 2 where you can write bytes into it. So we create a temporary BytesIO and then write it to self.contents after converting it to unicode.

@rahulporuri rahulporuri Feb 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ohh damn. So this was just straight up broken earlier and we just didn't know. Thanks!

@jwiggins jwiggins Feb 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's right

@jwiggins jwiggins merged commit c6f711b into master Feb 5, 2021
@jwiggins jwiggins deleted the test/draw-image branch February 5, 2021 14:42
@jwiggins

jwiggins commented Feb 5, 2021

Copy link
Copy Markdown
Member Author

Thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@rahulporuri rahulporuri rahulporuri approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jwiggins @rahulporuri