Move `pyface.ui.qt4` to `pyface.ui.qt`

corranwebster commented 1 year ago

This moves pyface.ui.qt4.* to pyface.ui.qt and helps with gradually deprecating all uses of "qt4". The current state is internal consistency, optional hooks for backwards compatibility for applications that depend on Pyface.

Changes for downstream libraries and applications should be fairly simple: replacing qt with qt4 in appropriate import statements.

But there are some circumstances where this may not be feasible:

where the end-user doesn't control the code-base (eg. someone who pip-installs Mayavi and gets the new version of Pyface automatically)
where an application developer needs to make a newer version of pyface work with code they don't control that imports from pyface.ui.qt4.*
where a library developer wants to be compatible with newer and older versions of pyface

This PR solves two of these problems by providing opt-in import hooks that replace imports of pyface.ui.qt4.* with corresponding imports of pyface.ui.qt.* so that sys.modules points to the new location (avoiding issues with duplicated objects and modules which would happen with some other strategies). These hooks are not available by default, but can be accessed in a number of ways:

end-users can set environment variables to turn it on: ETS_QT4_IMPORTS to directly turn on the import hooks, and ETS_TOOLKIT=qt4 (as opposed to ETS_TOOLKIT=qt) with the assumption that this is an older environment
application developers can either set ETSConfig.toolkit = "qt4" (as opposed to ETSConfig.toolkit = "qt") with the assumption that this is flagging that this is an application made with older assumptions about import locations; or if they need more control, they can install the import hooks into sys.meta_path in a way that works best for them and any other import hooks that they might be using.

The third case of a library developer who wants to support newer and older versions should be done through the usual mechanisms (eg. try importing pyface.ui.qt.foo and if it fails import pyface.ui.qt4.foo; or looking at pyface version numbers). An alternative which will work in almost all cases is to instead use toolkit_object() to get the thing that they want, which will automatically get it from the right place.

Done:

[x] check about pickle compatibility with mementos: Tasks and Workbench mementos are independent of toolkit
[x] migration documentation (added to toolkits section)

Note:

This ended up taking about a day longer than expected because the original working version of the code was using a deprecated API that would be removed in Python 3.12, so it needed re-implementation after digging in to understand what the new importlib API was doing. In doing so it has been generalized a bit, since we expect to also do this in TraitsUI at a minimum.

Fixes #560

mdickinson commented 1 year ago

And I believe neither create_module nor exec_module should be causing changes to sys.modules

Okay - that's nonsense, of course, since exec_module could be executing imports for other modules. It still seems odd to have create_module potentially altering sys.modules.

mdickinson commented 1 year ago

I'm not sure that this is right [...]

Please ignore this. I was operating under the wrong mental model of how this was working. Let me go away and think and play a bit before I come back and embarrass myself further.

mdickinson commented 1 year ago

Okay, back. I was falsely imagining that we'd be creating new modules for the qt4 imports that were shallow copies of the existing modules, but instead we're making those qt4 imports point to the existing qt modules (and caching in sys.modules).

In which case, this all seems to work.

There does seem to be a minor oddity with packages (as opposed to modules), which as far as I can tell aren't shared, though I'm not sure why.

(pyface) mdickinson@mirzakhani pyface % export ETS_TOOLKIT="qt4"
(pyface) mdickinson@mirzakhani pyface % python
Python 3.11.2 (main, Feb 10 2023, 08:20:03) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyface.ui.qt4.data_view as dv1
<stdin>:1: DeprecationWarning: The pyface.ui.qt4.* modules have moved to pyface.ui.qt.*

Backward compatibility import hooks have been automatically applied.
They will be removed in a future release of Pyface.

>>> import pyface.ui.qt.data_view as dv2
>>> dv1 is dv2  # expecting `True` here
False

I guess this would be problematic if we were in the habit of putting classes or functions in our __init__.py files, but we're not.

corranwebster commented 1 year ago

There does seem to be a minor oddity with packages (as opposed to modules), which as far as I can tell aren't shared, though I'm not sure why.

Yes, this is probably safe because of our coding practices; no, I have no idea why it's happening - for a time I had the problem that modules immediately under pyface.ui.qt4 were shallow copies, but deeper ones were not. I might look at it briefly, but this is probably close enough for what we need as an optional, temporary thing.

corranwebster commented 1 year ago

This latest set of changes seems to resolve the issue with packages being duplicated: the solution was to tell the ModuleSpec that it is in fact a package by adding:

is_package=(new_spec.submodule_search_locations is not None),

enthought / pyface

Move `pyface.ui.qt4` to `pyface.ui.qt` #1223