Skip to content

Don't test drawing on Qt4#745

Merged
jwiggins merged 1 commit into
masterfrom
fix/qt4-ci
Mar 23, 2021
Merged

Don't test drawing on Qt4#745
jwiggins merged 1 commit into
masterfrom
fix/qt4-ci

Conversation

@jwiggins
Copy link
Copy Markdown
Member

Our Pillow requirement has been upgraded to version 7.2.0 in EDM. This version removed support for Qt4. To get CI working again, we should skip tests of QPainter drawing on Qt4.

@jwiggins jwiggins requested a review from rahulporuri March 23, 2021 11:56
Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Code changes LGTM but I don't know if there are any documentation changes that are necessary. I'm not entirely sure what all backends will continue to support Qt4 and Qt5 once we start depending on pillow >= 7.2.0. I think the agg and celiagg backends will continue supporting both Qt4 and Qt5. Is that correct?

@jwiggins
Copy link
Copy Markdown
Member Author

Yes, the AGG backends will continue to support Qt4 and Qt5. We can also just "vendor" the QImage generation code so that we don't need to get it from Pillow. All backends which paint an offscreen GC into a QWidget would be able to use such code.

@rahulporuri
Copy link
Copy Markdown
Contributor

We can also just "vendor" the QImage generation code so that we don't need to get it from Pillow.

I don't think we need that at the moment. We can announce with the 5.1.0 release that Qt4 is only supported by the agg backends if the edm eggs are used (because itll depend on pillow >= 7.2.0) and if someone complains, we can consider vendoring that code. Otherwise, i think it's an unnecessary maintenance overhead.

@jwiggins jwiggins merged commit 5ce24ec into master Mar 23, 2021
@jwiggins
Copy link
Copy Markdown
Member Author

Thanks for the review

@jwiggins jwiggins deleted the fix/qt4-ci branch March 23, 2021 13:00
jwiggins added a commit that referenced this pull request Mar 24, 2021
jwiggins added a commit that referenced this pull request Mar 25, 2021
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.

2 participants