Move .trait_defs.ui.qt4 to .trait_defs.ui.qt #1028

homosapien-lcy · 2023-04-11T06:19:13Z

Recently a decision to move pyface.ui.qt4 to pyface.ui.qt has been made in pyface (enthought/pyface#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

dpinte · 2023-04-11T09:54:17Z

enable/savage/trait_defs/ui/__init__.py

@@ -0,0 +1,93 @@
+# Import hooks for loading enable.savage.trait_defs.ui.qt.* in place of 


@homosapien-lcy this feels like a lot of duplicated code. enable depends on pyface, no? I suspect we could make sure it depends on pyface 8.x and just import the code from there.

Yeah, currently the issue is, if the pyface version the user need is not 8.x, we would still need the ShadowedModuleFinder in Enable since we cannot import it (This folder renaming does not happen only in pyface, but in other packages as well). Do you want the code to first try to import from the ShadowModuleFinder like this?:

try: from pyface.ui import ShadowedModuleFinder except ImportError: from enable.savage.trait_defs.ui import ShadowedModuleFinder

It's good to think of backward compatibility. The current enable version has a requirement of pyface >= 7.2 (see https://github.com/enthought/enable/blob/main/enable/__init__.py#L19). You can bump the requirement for this release to be >= 8.0.

Yes, we did this for the upcoming TraitsUI release - simply require Pyface >= 8

corranwebster · 2023-04-12T10:28:28Z

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:

enable/enable/savage/trait_defs/ui/toolkit.py

Line 28 in e9f97e3

backend = "enable.savage.trait_defs.ui.%s" % ETSConfig.toolkit

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.

…inder in favor of the one in pyface

homosapien-lcy · 2023-04-14T04:37:11Z

The online tests seem failed due to installing pyface7.4.4 for the tests.

homosapien-lcy · 2023-04-14T06:09:43Z

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 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:

enable/enable/savage/trait_defs/ui/toolkit.py

Line 28 in e9f97e3

backend = "enable.savage.trait_defs.ui.%s" % ETSConfig.toolkit

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

…nce the import for enable.savage.trait_defs.ui.qt is not widely imported in third party package

Rename *.qt4.* modules as *.qt.* and adapt imports to account for changes in Pyface 8.0 and TraitsUI 8.0 while keeping backwards compatibility. It also removes some lingering PyQt4 support code. This has some overlap with #1028 but doesn't fix everything that that PR does, so we still need that, or need to merge it into this. As best I can tell, this change will break one test in an internal downstream project so we don't need to do any magic backwards compatibility mapping like we did in Pyface or TraitsUI.

dpinte · 2023-04-26T07:27:55Z

enable/qt/base_window.py

@@ -42,7 +42,6 @@
    DRAG_RESULTS_MAP,
 )

-


this should be removed

dpinte · 2023-04-26T07:28:06Z

enable/tests/qt/test_scrollbar.py

@@ -26,6 +26,7 @@
 class ScrollBarTest(unittest.TestCase):
    def setUp(self):
        from pyface.qt.QtGui import QApplication
+


this can be removed

dpinte · 2023-04-26T07:30:42Z

setup.py

@@ -422,7 +422,6 @@ def macos_extensions():
            'enable.toolkits': [
                'null = enable.null.toolkit:toolkit',
                'qt = enable.qt.toolkit:toolkit',
-                'qt4 = enable.qt.toolkit:toolkit',


this needs to stay for backward compatibility!

… backward compatibility

homosapien-lcy · 2023-04-26T08:19:22Z

In the latest commits, unneccessary spaces are removed, backward compatibility is added back

mdickinson · 2023-04-27T07:36:46Z

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?

mdickinson · 2023-04-27T07:50:24Z

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?

dpinte · 2023-04-27T08:01:19Z

Good question, @mdickinson ! I'll let @homosapien-lcy investigate.

homosapien-lcy · 2023-04-28T04:17:45Z

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?

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

mdickinson · 2023-04-28T06:55:05Z

@homosapien-lcy As of commit 5f076f3 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.

dpinte · 2023-04-28T07:28:21Z

@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 #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.

mdickinson · 2023-04-28T07:29:57Z

@dpinte Yep, that sounds plausible.

mdickinson · 2023-04-28T07:42:15Z

enable/__init__.py

@@ -16,7 +16,7 @@
    __version__ = "not-built"

 __requires__ = [
-    "numpy", "pillow", "traits>=6.2.0", "traitsui", "pyface>=7.2.0", "fonttools"


Do we still need this change to the Pyface requirement?

Let me check on that, whether any issues will be raised in pyface < 8

Since many of the changes are depending on pyface 8.00 which renamed all qt4 to qt, I think we need pyface 8.00

Please could you point to some examples? I'm not seeing anything in this PR which would require us to use Pyface 8.0.

(Note that EDM doesn't have Pyface 8.0 yet.)

@homosapien-lcy Ah, you edited your comment after I replied to it. Please don't do that! (Now my replies don't make sense out of context.)

mdickinson · 2023-04-28T07:44:18Z

kiva/examples/kiva/qt_simple.py

+try:
+    from pyface.qt import QtGui
+except ImportError:
+    raise Exception("PyQt4 needs to be installed to run this example")


Is this accurate? We stopped supporting PyQt4 some time ago, so I'm not sure there's much value in introducing a new example that requires PyQt4.

Ah, I see; this isn't a new example - it's a duplicate of kiva/examples/kiva/qpainter_simple.py, with only this one line changed. I suggest removing this addition to the PR.

mdickinson · 2023-04-28T07:48:22Z

@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?

…t_simple example

homosapien-lcy · 2023-05-01T07:29:23Z

I have push a change to remove the pyface 8.0.0 requirement and remove the qt_simple.py example

mdickinson

Thanks for the changes. LGTM.

Chengyu Liu added 3 commits April 11, 2023 14:57

FEAT: add ShadowedModuleFinder

4caed24

MAINT: renaming QT directories to fit standard

9a0c944

fix typo

605a020

homosapien-lcy changed the title ~~[WIP] FEAT: add ShadowedModuleFinder~~ FEAT: add ShadowedModuleFinder Apr 11, 2023

Merge branch 'main' into fix_qt_import

0f5b8d8

dpinte requested changes Apr 11, 2023

View reviewed changes

dpinte mentioned this pull request Apr 11, 2023

ModuleNotFoundError: No module named 'pyface.ui.qt4.color' when running demos related to enable/enable/trait_defs/ui/qt4/rgba_color_editor.py enthought/chaco#886

Closed

Chengyu Liu added 3 commits April 14, 2023 13:18

1 increase pyface version requirement 2 replace local ShadowedModuleF…

c36536b

…inder in favor of the one in pyface

change pyface version

f62adfc

reverse test env change

dfe79cf

1 also add shadowmodulefinder for enable.trait_defs.ui 2 fix typos

e7e9e9e

homosapien-lcy changed the title ~~FEAT: add ShadowedModuleFinder~~ Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt Apr 14, 2023

Chengyu Liu added 3 commits April 19, 2023 15:29

MAINT: replace shadowModuleFiner with a simple import modification si…

53933d1

…nce the import for enable.savage.trait_defs.ui.qt is not widely imported in third party package

cancel rgba editting

75e22e4

FORMAT: fix typo of qt

0bcf8a4

homosapien-lcy changed the title ~~Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt~~ [WIP] Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt Apr 19, 2023

Chengyu Liu added 7 commits April 19, 2023 15:59

Nit: fix typo in error message

06d9b69

MAINT: Replacing some of the qt4 for qt (more changes on the way)

176699a

MAINT: resolve conflict with main

db10f9d

Merge branch 'main' into fix_qt_import

83985bd

MAINT 1 rename enable.qt4 -> enable.qt 2 add import path changinvg

992a32f

MAINT: qt importing changes for ci and kiva example

5cab91f

MAINT: kiva qt example renaming

d4e411b

homosapien-lcy changed the title ~~[WIP] Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt~~ Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt Apr 20, 2023

corranwebster mentioned this pull request Apr 20, 2023

Rename "qt4" to "qt" #1043

Merged

Merge branch 'main' into fix_qt_import

3a3fda9

dpinte requested changes Apr 26, 2023

View reviewed changes

dpinte reviewed Apr 26, 2023

View reviewed changes

MAINT: 1 remove unneccessary space changes 2 add back qt4 toolkit for…

5f076f3

… backward compatibility

dpinte approved these changes Apr 27, 2023

View reviewed changes

MAINT: remove wrongly moved qt.** modules

4e9627d

mdickinson reviewed Apr 28, 2023

View reviewed changes

MAINT: 1 reverse pyface 8.0 version requirement 2 remove duplicated q…

bba795e

…t_simple example

mdickinson approved these changes May 1, 2023

View reviewed changes

Merge branch 'main' into fix_qt_import

f81e2a5

corranwebster merged commit 92196ef into main May 2, 2023
35 checks passed

corranwebster deleted the fix_qt_import branch May 2, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move .trait_defs.ui.qt4 to .trait_defs.ui.qt #1028

Move .trait_defs.ui.qt4 to .trait_defs.ui.qt #1028

homosapien-lcy commented Apr 11, 2023 •

edited

dpinte Apr 11, 2023

homosapien-lcy Apr 12, 2023 •

edited

dpinte Apr 12, 2023

corranwebster Apr 12, 2023

corranwebster commented Apr 12, 2023

homosapien-lcy commented Apr 14, 2023

homosapien-lcy commented Apr 14, 2023

dpinte Apr 26, 2023

dpinte Apr 26, 2023

dpinte Apr 26, 2023

homosapien-lcy commented Apr 26, 2023

mdickinson commented Apr 27, 2023

mdickinson commented Apr 27, 2023 •

edited

dpinte commented Apr 27, 2023

homosapien-lcy commented Apr 28, 2023

mdickinson commented Apr 28, 2023

dpinte commented Apr 28, 2023 •

edited

mdickinson commented Apr 28, 2023

mdickinson Apr 28, 2023

homosapien-lcy Apr 28, 2023

mdickinson Apr 28, 2023 •

edited

mdickinson Apr 28, 2023

mdickinson Apr 28, 2023

mdickinson Apr 28, 2023

mdickinson Apr 28, 2023

mdickinson commented Apr 28, 2023 •

edited

homosapien-lcy commented May 1, 2023

mdickinson left a comment

		@@ -0,0 +1,93 @@
		# Import hooks for loading enable.savage.trait_defs.ui.qt.* in place of

Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt #1028

Move *.trait_defs.ui.qt4 to *.trait_defs.ui.qt #1028

Conversation

homosapien-lcy commented Apr 11, 2023 • edited

Choose a reason for hiding this comment

homosapien-lcy Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corranwebster commented Apr 12, 2023

homosapien-lcy commented Apr 14, 2023

homosapien-lcy commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homosapien-lcy commented Apr 26, 2023

mdickinson commented Apr 27, 2023

mdickinson commented Apr 27, 2023 • edited

dpinte commented Apr 27, 2023

homosapien-lcy commented Apr 28, 2023

mdickinson commented Apr 28, 2023

dpinte commented Apr 28, 2023 • edited

mdickinson commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson Apr 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdickinson commented Apr 28, 2023 • edited

homosapien-lcy commented May 1, 2023

mdickinson left a comment

Choose a reason for hiding this comment

Move .trait_defs.ui.qt4 to .trait_defs.ui.qt #1028

Move .trait_defs.ui.qt4 to .trait_defs.ui.qt #1028

homosapien-lcy commented Apr 11, 2023 •

edited

homosapien-lcy Apr 12, 2023 •

edited

mdickinson commented Apr 27, 2023 •

edited

dpinte commented Apr 28, 2023 •

edited

mdickinson Apr 28, 2023 •

edited

mdickinson commented Apr 28, 2023 •

edited