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

Added PySide6 Support (squash) #394

Merged
merged 39 commits into from
May 2, 2024
Merged

Added PySide6 Support (squash) #394

merged 39 commits into from
May 2, 2024

Conversation

zoshua
Copy link
Contributor

@zoshua zoshua commented Feb 22, 2024

New branch for the "squashing"

@zoshua zoshua changed the title Added PySide6 Support Added PySide6 Support (squash) Feb 22, 2024
@mottosso
Copy link
Owner

Excellent, nice and clean. But, somehow, tests are still not running. 😭 @martin-chatterjee can you spot what we could do to make tests run on PRs?

I expect tests will pass, as they appear to have done locally. So it's likely we can merge this, without officially announcing support for PySide6, and work our way to introducing tests for it in a separate PR. Although ideally, we'd do that here as well.

One of the reasons for using such a complex Dockerfile as we have today is because the world was young, and PyQt and PySide was just starting to live side by side, so it wasn't clear whether there were differences between official PySide/PyQt on PyPI versus PySide embedded in Maya. At this point however, I expect it would be very safe to test against what exists on PyPI. Especially considering this project no longer runs exclusively on Maya.

I remember seeing someone post their fork with an alternative test suite, but I'm struggling to find it :S It would be sensible to keep a separate GitHub action script or runner per Python version and PySide/PyQt combination.

Would you be up for this @zoshua or @martin-chatterjee?

@martin-chatterjee
Copy link
Contributor

"Excellent, nice and clean. But, somehow, tests are still not running. 😭 @martin-chatterjee can you spot what we could do to make tests run on PRs?"

@mottosso, I just found the time to have a quick look, and I can not reproduce the CI issues on my side – which is good news, I guess? 🙂

I've created a PR for further debugging – so maybe you could try to merge my PR into some temp branch and push on your end?
This should show us if the CI still fails on mottosso/Qt.py.


For reference, here's what I did:

1. Add @zoshua's Qt.py fork as a remote, and fetch:

git remote add zoshua git@github.com:zoshua/Qt.py.git
git remote set-url zoshua --push no-push
git fetch zoshua

2. Create new branch review-zoshua-pyside6 off of master, and push:

git checkout master
git checkout -b review-zoshua-pyside6
git push origin review-zoshua-pyside6

→ This establishes my base line, proofing that CI is (still) running as expected. ✅

3. Bring in @zoshua's work out of branch squash, and un-stage it:

git merge --squash zoshua/squash
git restore --staged .

4. Commit changes bit by bit, and push on every commit:

CI completes without errors on every commit. ✅

@mottosso
Copy link
Owner

so maybe you could try to merge my PR into some temp branch and push on your end?

But actions should run without merging. A big part of the actions is to make sure merging is safe to begin with, but if we have to merge before running the tests..? Am I missing something? 🤔 Your PR also is not triggering any tests. I expect there must be a configuration option to make GitHub Actions run on PRs too?

@martin-chatterjee
Copy link
Contributor

@mottosso: Ah I see, I might have misunderstood the issue in the first place.

I was under the impression that the CI test jobs do not succeed on your end (→ just like this last failed GH action).

But what you would like to have is automatic CI test runs being executed on mottosso/Qt.py for every new PR, right?

So I might have barked up the wrong tree here for while... 🙂🤦‍♂️

@martin-chatterjee
Copy link
Contributor

Off the top of my head, I'm not sure if running CI on the targeted repo of a PR is possible on Github – I've never thought about that before...

However, before any contributor is able to open a PR, they need to push to their fork – and this will already trigger and execute the CI test jobs over there.

So in a way, every PR already has executed CI test jobs associated with it – just not on mottosso/Qt.py, but on the source fork.

@mottosso, is this what you're after?

@mottosso
Copy link
Owner

But what you would like to have is automatic CI test runs being executed on mottosso/Qt.py for every new PR, right?

That's right, yeah. Sorry for not being clear about this! I appreciate you spent time putting together this alternative PR and painstakingly ran through each commit.

@mottosso
Copy link
Owner

@mottosso, is this what you're after?

No. In every other project I maintain on GitHub, CI is always run on PRs. There should be no need to visit a fork in order to see the results of tests.

For example:

@martin-chatterjee
Copy link
Contributor

Ah nice, thanks for the example. So this actually is possible within Github. I'll take a look. 👍

@martin-chatterjee
Copy link
Contributor

Well, that should be easy... 🙂👍

According to the example repo you provided:

name: cmdx-workflow

on:
  push:
  pull_request:
    branches: [ master ]

That seems to be all that's needed. 🙂

I'll take a look and test this.

@martin-chatterjee
Copy link
Contributor

@mottosso:

PR #399

@mottosso
Copy link
Owner

Excellent, thanks @martin-chatterjee. At this point, I expect any updates to this PR to also run all tests. @zoshua would you mind merging with latest master, so we can see this thing in action?

@mottosso
Copy link
Owner

Great work. Now all we need is a GitHub workflow to test the PySide6-specifics of it. I expect we can start by adding a pyside6.yml here with the steps (1) install Python, (2) install PySide6 and (3) run the tests. Would you like to give it a spin @zoshua?

run: |
docker run --rm \
-v $(pwd):/Qt.py \
-e PYTHON=${{ matrix.PYTHON }} \
Copy link
Owner

Choose a reason for hiding this comment

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

This line runs a Docker container, which we would no longer need in this case. Instead, call the ./entrypoint.sh file directly, along with the Python version used.

run: PYTHON=3.11 ./entrypoint.sh

Set PYTHON=3.10 or 3.11, depending on which version of Python ubuntu-latest has. You should probably set it to a specific version of Ubuntu, so we know and that it won't change in the future.

Copy link
Owner

Choose a reason for hiding this comment

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

From here, it looks like Ubuntu 22.

It would be safe to set runs-on to always use Ubuntu 22. That way, we will be made aware of when it is no longer available, a few years from now, and can update accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I just had a look: Ubuntu 22 seems to ship with Python 3.10.
(→ source)

Comment on lines 25 to 27
- name: pip install PySide6
run: |
pip install PySide6
Copy link
Contributor

Choose a reason for hiding this comment

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

Choose pip explicitly, install needed dependencies.

As @mottosso mentioned, these tests will not be run within the Docker container anymore. So you probably also need to install a few more dependency packages here.

You'll probably need to installnose and nosepipe, maybe a few more. (→ You can have a look at the Dockerfile, to see what's needed.)


Also, it might be good to be explicit about the pip/python version that you are using.

So please replace:
(Using Python 3.10 in my example...)

pip install PySide6

with:

python3.10 -m pip install PySide6

Comment on lines 14 to 19
strategy:
matrix:
include:
- os: ubuntu-latest
VFXPLATFORM: "2018"
PYTHON: "3.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the strategy: block can go away.

A strategy: matrix: block is used to execute tests within the Docker container multiples times in a row in run-tests.yml, using different parameters.

→ I think this does not have any use in here, and can be removed.

run: |
docker run --rm \
-v $(pwd):/Qt.py \
-e PYTHON=${{ matrix.PYTHON }} \
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I just had a look: Ubuntu 22 seems to ship with Python 3.10.
(→ source)

removed strategy block

updated pip install step

updated the run tests step
added step Set entrypoint.sh script permissions
changed nosetests${PYTHON} to python${PYTHON} -m nose
entrypoint.sh Outdated
@@ -22,13 +22,13 @@ printf "#\n# Testing implementation..\n"
python${PYTHON} -u run_tests.py
printf "#\n# Testing caveats..\n"
python${PYTHON} build_caveats.py
nosetests${PYTHON} \
python${PYTHON} -m nose \
Copy link
Owner

Choose a reason for hiding this comment

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

nose is oldschool, need to use nose2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rgr that

updated pip install step to use nose2
updated to use nose2
removed incompatible nose2 command line arguments
more nose2 usage updates
@mottosso
Copy link
Owner

mottosso commented Apr 6, 2024

https://github.com/mottosso/Qt.py/pull/394/checks#step:7:94

Only one missing member! That's great.

This seems to me like it's been renamed/slightly edited. We could either (1) wrap the Qt 6 class in our own to emulate the one from Qt 4/5. Or we could deprecate it entirely. Wrapping it seems straightforward, but I haven't looked into whether it's used in the same way. And it is useful, I expect we will need it.

Thoughts?

@mottosso
Copy link
Owner

mottosso commented Apr 6, 2024

"AttributeError: module 'PySide2.QtCore' has no attribute 'QStringListModel'"

Did you overcome this? Because it does exist in Qt 4/5/6.

@zoshua
Copy link
Contributor Author

zoshua commented Apr 6, 2024

"AttributeError: module 'PySide2.QtCore' has no attribute 'QStringListModel'"

Did you overcome this? Because it does exist in Qt 4/5/6.

I just reverted to what previously existed/worked here in the github Run Tests.

Do you think its related to running our tests in the highly customized docker container?

added QRegularExpressionValidator remap
@zoshua
Copy link
Contributor Author

zoshua commented Apr 6, 2024

Just checked and it appears that the function signatures match..

https://doc.qt.io/qtforpython-5/PySide2/QtGui/QRegExpValidator.html#PySide2.QtGui.PySide2.QtGui.QRegExpValidator

https://doc.qt.io/qtforpython-6/PySide6/QtGui/QRegularExpressionValidator.html#PySide6.QtGui.PySide6.QtGui.QRegularExpressionValidator

So we should be good to map PySide6 to QRegExpValidator with _misplaced_members

@zoshua
Copy link
Contributor Author

zoshua commented Apr 6, 2024

@mottosso
This one appears to be PyQt4 Centric
File "/home/runner/work/Qt.py/Qt.py/examples/loadUi/baseinstance2.py", line 7, in

You think I am good to remove from Pyside6.yml?

@mottosso
Copy link
Owner

mottosso commented Apr 7, 2024

Yes, definitely. Based on explicitly making this only run for PyQt4, https://github.com/mottosso/Qt.py/blob/master/examples/loadUi/baseinstance2.py that seems safe.

Removed 
python3.10 -m nose2 --verbose examples.loadUi.baseinstance2
@mottosso
Copy link
Owner

Had a look at this, and we've got a bit more work before we can call this drop-in ready for Qt 4 and Qt 5.

  1. QFont.setWeight doesn't take a number anymore, but an enum
  2. QtCore.Qt.MidButton no longer exists for mousePressEvent

Here's one way to solve (1).

diff --git a/Qt.py b/Qt.py
index 08b5c03..00d3571 100644
--- a/Qt.py
+++ b/Qt.py
@@ -1195,6 +1195,9 @@ _compatibility_members = {
             "getOpenFileNames": "QtWidgets.QFileDialog.getOpenFileNames",
             "getSaveFileName": "QtWidgets.QFileDialog.getSaveFileName",
         },
+        "QFont": {
+            "setWeight": "QtGui.QFont.setWeight",
+        },
     },
     "PySide2": {
         "QWidget": {
@@ -1514,8 +1517,38 @@ def _pyside6():
         Qt.QtCompat.setSectionResizeMode = \
             Qt._QtWidgets.QHeaderView.setSectionResizeMode

+    def setWeight(func):
+        def wrapper(self, weight):
+            weight = {
+                100: Qt._QtGui.QFont.Thin,
+                200: Qt._QtGui.QFont.ExtraLight,
+                300: Qt._QtGui.QFont.Light,
+                400: Qt._QtGui.QFont.Normal,
+                500: Qt._QtGui.QFont.Medium,
+                600: Qt._QtGui.QFont.DemiBold,
+                700: Qt._QtGui.QFont.Bold,
+                800: Qt._QtGui.QFont.ExtraBold,
+                900: Qt._QtGui.QFont.Black,
+            }.get(weight, Qt._QtGui.QFont.Normal)
+
+            return func(self, weight)
+
+        wrapper.__doc__ = func.__doc__
+        wrapper.__name__ = func.__name__
+
+        return wrapper
+
+    decorators = {
+        "QFont": {
+            "setWeight": setWeight,
+        }
+    }
+
     _reassign_misplaced_members("PySide6")
-    _build_compatibility_members("PySide6")
+    _build_compatibility_members("PySide6", decorators)


 def _pyside2():

Namely, we provide a new member via QtCompat which requires updates to legacy software.

font = self._window.font()
font.setFamily("Open Sans")
font.setPointSize(8)
Qt.QtCompat.QFont.setWeight(font, 400)

Not ideal, the ideal would be for existing code to require 0 changes. But, we cannot do:

Qt.QtGui.QFont.setWeight = some_wrapper_function

As this would modify the original binding, which is one of the things that sets Qt.py apart from other wrappers. We cannot risk the stability of code that does not use Qt.py.

What options do we have? If we go with the above, then we must provide this member for all bindings, and we should also provide the new enums, e.g. QtCompat.QtGui.QFont.Bold, such that new code can run on old bindings.

Here's one project you can try with.

python -m pip install pyblish-lite

It bundles Qt.py inside of vendor/Qt.py. Replace that with this version and you'll run into the above issues. Can we test with a few project projects to find what else there is? We'll need an upgrade guide in the README with what users are expected to do in order to make their projects compatible with Qt 6. If indeed we cannot find a way to make it transparent.

@zoshua
Copy link
Contributor Author

zoshua commented Apr 26, 2024

Thank you for the feedback and insight.

Will be picking this up soon 👍

@zoshua
Copy link
Contributor Author

zoshua commented Apr 27, 2024

Investigations...

  1. I made the "Here's one way to solve (1)." tweaks locally to Qt.py and ran our current PySide6 tests without issue.
  2. I created a new space/venv to test pyblish-lite w/ PySide6 and the latest tweaks made from step 1, updating the local vendor/Qt.py.
  3. I encounter and fix some PySide6 _misplaced_members issues
  4. I confirm the QtCore.Qt.MidButton button issue
  5. I hacked a bit with _misplaced_members to see if we could reroute "QtCore.Qt.MiddleButton": "QtCore.Qt.MidButton" with no luck
  6. I hacked a bit further directly tweaking _pyside6() by adding Qt._QtCore.Qt.MidButton = module.QtCore.Qt.MiddleButton, which seems to have made the error go away, however I am sure this is not the appropriate solution. Also, I have not used pyblish before so I am not familiar w/ how it is supposed to function.

Thoughts on next steps?

@mottosso
Copy link
Owner

I hacked a bit further directly tweaking _pyside6() by adding Qt._QtCore.Qt.MidButton = module.QtCore.Qt.MiddleButton, which seems to have made the error go away

This would be the easiest route, but we can't risk affecting applications that use PyQt/PySide without Qt.py, and this approach would affect those.

Thoughts on next steps?

Let's stick with the first approach for now and put it in a "Qt 6 Transition Guide". We need the find a similar solution to MidButton and put that in the guide too. Our goal then should be to make this guide as small as possible.

Qt 6 Transition Guide

  • Replace font.setWeight(some number) with Qt.QtCompat.QFont.setWeight(font, some number)
  • Replace event.type == QtCore.MidButton with ...?
  • And so forth

updated _pyside6() - added QtCompat.QFont.setWeight functionality

updated _misplaced_members PySide2 to restore parity with current release.

updated _misplaced_members PySide6 to be consistent with PySide2.
@zoshua
Copy link
Contributor Author

zoshua commented May 1, 2024

Made the suggested updates and added a possible solution for the Mid/Middle Button change.

Confirmed error removal in pyblish-lite by making the following changes in
view.py
from .vendor.Qt import QtCore, QtWidgets, QtCompat

view.py - Item.mousePressEvent()
if event.button() == QtCompat.Qt.MidButton:

@mottosso
Copy link
Owner

mottosso commented May 2, 2024

Great, thanks for this.

Considering that this PR does not affect existing use of Qt.py, I'll merge this now despite suspecting that there will be a number of bullet points needed for an upgrade guide. I'll add a section to the README now with what we now, then we can continue adding and updating as we continue our discovery. A new release will follow shortly.

@mottosso mottosso merged commit 5a88175 into mottosso:master May 2, 2024
9 checks passed
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.

3 participants