Closed homosapien-lcy closed 1 year ago
I think the sys.meta_path
stuff is overkill for this situation, unless there is a lot of code that is directly importing enable.savage.trait_defs.ui.qt4
in 3rd-party libraries. A simpler solution is to just have some logic that if ETSConfig.toolkit
is qt4
then the toolkit should import from enable.savage.trait_defs.ui.qt
instead: ie. replace this:
https://github.com/enthought/enable/blob/e9f97e30d78c032c19ae194ab6ee16a6aaa00a65/enable/savage/trait_defs/ui/toolkit.py#L28
with
if ETSConfig.toolkit == "qt4":
backend = "enable.savage.trait_defs.ui.qt"
else:
backend = "enable.savage.trait_defs.ui.%s" % ETSConfig.toolkit
and that should likely fix everything.
The online tests seem failed due to installing pyface7.4.4 for the tests.
enable.savage.trait_defs.ui
I think the
sys.meta_path
stuff is overkill for this situation, unless there is a lot of code that is directly importingenable.savage.trait_defs.ui.qt4
in 3rd-party libraries. A simpler solution is to just have some logic that ifETSConfig.toolkit
isqt4
then the toolkit should import fromenable.savage.trait_defs.ui.qt
instead: ie. replace this:with
if ETSConfig.toolkit == "qt4": backend = "enable.savage.trait_defs.ui.qt" else: backend = "enable.savage.trait_defs.ui.%s" % ETSConfig.toolkit
and that should likely fix everything.
This solution actually may make sense, since there seems not to be other places where we need to use the qt import (for instance, "enable.trait_defs.ui.qt doesn't have the same problem). Also, this can avoid the git test failure problem due to pyface version requirement. What do you think @dpinte
In the latest commits, unneccessary spaces are removed, backward compatibility is added back
What's the reason for the existence of both enable.savage.trait_defs.ui.qt.svg_editor
and enable.savage.trait_defs.qt.svg_editor
?
I have the same question for svg_button_editor
: at this point, enable/savage/trait_defs/qt/svg_button_editor.py
and enable/savage/trait_defs/ui/qt/svg_button_editor.py
seem to be near duplicates of one another. Should the qt/
versions have been removed, or are both needed? If we need both modules, is there a way to fix the duplication?
Good question, @mdickinson ! I'll let @homosapien-lcy investigate.
I have the same question for
svg_button_editor
: at this point,enable/savage/trait_defs/qt/svg_button_editor.py
andenable/savage/trait_defs/ui/qt/svg_button_editor.py
seem to be near duplicates of one another. Should theqt/
versions have been removed, or are both needed? If we need both modules, is there a way to fix the duplication?
Hi Mark:
They are actually different, enable.savage.trait_defs.ui.svg_editor and enable/savage/trait_defs/ui/svg_button_editor.py also inherits BasicEditorFactory (https://github.com/enthought/traitsui/blob/main/traitsui/basic_editor_factory.py) from traitsui class, which allows them to create editors of different styles with the same class as foundation.
If the duplicated names can cause confusion, there are two ways we can solve it:
1 make these class doubly inherited both Editor and BasicEditorFactory and merge into 1 class (For instance SVGEditor(Editor) -> SVGEditor(Editor, BasicEditorFactory)). But the caveat is this will make the class content very long and less readable
2 change the class name of the qt version to **Foundation to avoid confusion
what do you think @mdickinson @dpinte
@homosapien-lcy As of commit 5f076f3b29f9a3f5a757c6be080faa332faacbad in this PR, we have:
These two files are almost identical; one of them already existed in the main branch prior to this PR, and one of them was introduced in this PR. I'm asking why it was necessary to introduce a new module that's almost identical to an existing one.
I'm not asking about the editor factory file at https://github.com/enthought/enable/blob/5f076f3b29f9a3f5a757c6be080faa332faacbad/enable/savage/trait_defs/ui/svg_editor.py. As you correctly say, that's something different.
A similar situation exists for svg_button_editor
. I was hoping that since you introduced these near-duplicate modules in this PR, you could explain why the duplication was necessary.
@mdickinson @homosapien-lcy looking at the enable.savage.traits_def.ui.toolkit
, the toolkit directories should in the enable/savage/traits_def/ui
. This where the old qt4
directory was. I think the issues come from PR https://github.com/enthought/enable/pull/1043 that moved the qt4
directory to enable/savage/traits_def
. The solution is to remove enable/savage/traits_def/qt
and keep enable/savage/traits_def/ui/qt
.
@dpinte Yep, that sounds plausible.
@homosapien-lcy It seems that the file qt_simple.py
introduced in this PR is also essentially a duplicate of qpainter_simple.py
in the same directory - only the exception message has been changed, in a way that doesn't make sense to me.
Should qt_simple.py
be deleted? If not, what's the reason for introducing it?
I have push a change to remove the pyface 8.0.0 requirement and remove the qt_simple.py example
Recently a decision to move pyface.ui.qt4 to pyface.ui.qt has been made in pyface (https://github.com/enthought/pyface/pull/1223) since versions newer than qt4 have become increasingly popular. However, this move can lead to import errors in other packages that are utilizing pyface when they try to import from qt4. For instance, in issue #1026 , we found that the example cannot find the enable.savage.trait_defs.ui.qt module since the default ETS_TOOLKIT is qt while the folder for qt is still enable.savage.trait_defs.ui.qt.
The first way I proposed to solve this is by adapting the similar ShadowedModuleFinder approach in pyface (https://github.com/enthought/pyface/blob/main/pyface/ui/qt4/__init__.py).
Now, Corran also proposed a simpler solution in the comment which is to use a simple check on import. This may be a simpler solution than using ShadowedModuleFinder. Closes issue #1026