Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SG-30534: First step to supporting PySide6 #898

Merged
merged 2 commits into from
Jul 6, 2023
Merged

SG-30534: First step to supporting PySide6 #898

merged 2 commits into from
Jul 6, 2023

Conversation

staceyoue
Copy link
Contributor

  • Provide a patch for PySide6 to work using the PySide (Qt4) interface
  • Create a new qt6 module to import PySide6 directly (unpatched)
  • Next step, deprecate PySide/Qt4 and make PySide6/Qt6 the default base

* Provide a patch for PySide6 to work using the PySide (Qt4) interface
* Create a new qt6 module to import PySide6 directly (unpatched)
* Next step, deprecate PySide/Qt4 and make PySide6/Qt6 the default base
python/tank/util/pyside6_patcher.py Show resolved Hide resolved
python/tank/util/pyside6_patcher.py Show resolved Hide resolved
python/tank/util/pyside6_patcher.py Show resolved Hide resolved
python/tank/util/pyside6_patcher.py Show resolved Hide resolved
python/tank/util/pyside6_patcher.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #898 (c9a5749) into master (e8e1a5f) will decrease coverage by 0.27%.
The diff coverage is 10.72%.

@@            Coverage Diff             @@
##           master     #898      +/-   ##
==========================================
- Coverage   79.13%   78.86%   -0.27%     
==========================================
  Files         177      179       +2     
  Lines       19495    19828     +333     
==========================================
+ Hits        15428    15638     +210     
- Misses       4067     4190     +123     
Impacted Files Coverage Δ
python/tank/util/pyside6_patcher.py 0.00% <0.00%> (ø)
python/tank/util/qt_importer.py 69.69% <54.28%> (-0.23%) ⬇️
python/tank/platform/engine.py 63.49% <81.81%> (+0.22%) ⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rob-aitchison rob-aitchison marked this pull request as ready for review June 29, 2023 16:20
Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

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

Look good to me. Just 2 minor comments.

@@ -0,0 +1,12 @@
# Copyright (c) 2016 Shotgun Software Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2016 -> 2023
Shotgun Software Inc -> Autodesk

To apply on each new file of this PR

@@ -372,6 +446,10 @@ def _import_modules(self, interface_version_requested):
return pyside2
except ImportError:
pass
elif interface_version_requested == self.QT6:
# TODO migrate qt base from Qt4 interface to Qt6 will require patching Qt5 as Qt6
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this TODO be taken care of? In this PR? In the next pyside6 PR? When we deprecate QT4?

logger.debug("Requesting %s-like interface", interface)

# First try PySide6.
if interface_version_requested == self.QT4:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo? (QT4 instead of QT6)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is. It troubled me too but it makes sense when you read the full file.

Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

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

Fine with me. The left overs comments will have to be addressed when dropping QT4.

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.

4 participants