Fix q svg widget#1022
Conversation
|
@homosapien-lcy This PR appears to include commits from a different PR (#1019). Was that intentional? |
|
|
||
| from io import BytesIO | ||
| from xml.etree.cElementTree import ElementTree | ||
| from PySide6.QtSvgWidgets import QSvgWidget |
There was a problem hiding this comment.
This import will only succeed if PySide6 is installed. What happens if someone wants to use this code with PyQt6 instead of PySide6?
mdickinson
left a comment
mdickinson
left a comment
There was a problem hiding this comment.
We need a fix that works with any Qt backend, not just with PySide6.
Also, please remove the unrelated changes from this PR.
Thanks for the comments. I opened another issue for this in the pyface https://app.zenhub.com/workspaces/ets-queue-priority-64022b3641e35b00166c1272/issues/gh/enthought/pyface/1235. This PR is supposed to be a quick fix |
Accidentally included those... Just removed |
Again, we need a fix that works with any Qt backend, not just with PySide6. It's important to think carefully about the potential impact of code changes. Question: what happens with the Enable main branch if someone with PySide2 installed tries to run the |
I think that won't work since we are using pyside6 here. |
Yes, exactly! The example works before this change, but fails afterwards. So yes, this needs to be fixed in Pyface, but it needs to be done carefully and tested with the various Qt backends there.
That won't work, because for PySide2, |
|
@homosapien-lcy For this PR, I see two possible ways forward: (1) Close the PR and wait for the next Pyface release (assuming that this has been fixed in Pyface) For (2), it's not clear to me whether this would work or not; it would need to be tested with all the currently supported Enable backends. |
Note that there's a separate discussion to be had here on which backends we want to support going forward. It may make sense to drop support for PySide2 and PyQt5 at this point. (I think Pyface has already done this.) |
Got it. I think solution 1 sounds reasonable. But I also push the change for solution 2 if some user needs a quick fix |
|
|
||
| # import widget for different package cases | ||
| try: | ||
| from pyface.qt import QtSvg as QWidget |
There was a problem hiding this comment.
I think QtSvg is a module here? So shouldn't this be from pyface.qt.QtSvg import QSvgWidget?
There was a problem hiding this comment.
Thanks for the comments. That's good point, then we can import QSvgWidget from both packages without the as renaming. Just adapt this change.
|
This issue is now fixed in Pyface, and my understanding is that we're expecting a Pyface release fairly soon. I'd recommend closing this PR. |
No, we'll probably keep PyQt5 and PySide2 around for a while (at least until we support PyQt6, I would think) |
4 participants
A temporary fix for issue #1020. Closes #1020.