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

PR: Improve compatibility for QtWidgets and QtGui modules between Qt5 and Qt6 bindings #410

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

StSav012
Copy link
Contributor

@StSav012 StSav012 commented Mar 3, 2023

Place QAction, QActionGroup, QFileSystemModel, QShortcut, and QUndoCommand both in QtGui and QtWidgets.

The rationale. When developing under PySide6, one can import QAction from qtpy.QtGui. When the code moved to PyQt5, it fails.

This is a part of the insanely large PR I recently proposed. This way, it would be easier to review the changes.

Fixes #389

@StSav012 StSav012 marked this pull request as ready for review March 3, 2023 16:03
@dalthviz dalthviz added this to the v2.3.1 milestone Mar 8, 2023
@StSav012 StSav012 changed the title Provide what has been moved to between QtWidgets and QtGui in Qt6 Provide what has been moved between QtWidgets and QtGui in Qt6 Mar 9, 2023
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @StSav012 for the work here! It LGTM 👍

Also, I think it is okay to include this for 2.3.1, but just in case, what do you think @ccordoba12 and @CAM-Gerlach ?

@dalthviz dalthviz changed the title Provide what has been moved between QtWidgets and QtGui in Qt6 PR: Provide full compatibility for QtWidgets and QtGui modules between Qt5 and Qt6 bindings Mar 16, 2023
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, seems fine to me @dalthviz 👍

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Some stylistic suggestion, otherwise looks good to me.

qtpy/QtGui.py Show resolved Hide resolved
qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/QtGui.py Show resolved Hide resolved
@ccordoba12
Copy link
Member

what do you think @ccordoba12?

Sure, I think that's fine because this is a small enough change.

@StSav012
Copy link
Contributor Author

Thank you all. @ccordoba12 , I'll accept the changes as soon as I'll get to PC.

Unfortunately, the changes don't provide full compatibility, for the modules have changed and are evolving. The change has its roots in mistakes I've made. Hopefully, others won't.

@ccordoba12
Copy link
Member

Unfortunately, the changes don't provide full compatibility, for the modules have changed and are evolving. The change has its roots in mistakes I've made. Hopefully, others won't.

I don't understand what you mean by this @StSav012. Are you talking about the changes you did in this PR? Or to my own suggestions?

@StSav012
Copy link
Contributor Author

Oh, I'm sorry. I meant the PR. The reason I mentioned in the PR description is what I've stumbled upon.

As for your corrections, I'll gladly accept them all when I reach my PC. Tomorrow night, probably.

@dalthviz dalthviz changed the title PR: Provide full compatibility for QtWidgets and QtGui modules between Qt5 and Qt6 bindings PR: Improve compatibility for QtWidgets and QtGui modules between Qt5 and Qt6 bindings Mar 17, 2023
@CAM-Gerlach
Copy link
Member

As for your corrections, I'll gladly accept them all when I reach my PC. Tomorrow night, probably.

(N.B., any of us can apply them for you if you want).

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 19, 2023

Yeah those are unrelated and will be fixed when #414 is merged, hopefully very soon, and then you can rebase on that:

git fetch upstream
git switch moved-modules-in-qt6
git rebase upstream/master
git push --force-with-lease

@dalthviz
Copy link
Member

PR #414 is now merged! 🎉

@CAM-Gerlach
Copy link
Member

I've gone ahead and used GItHub's built-in "Update Branch" button to update this. @StSav012 , if you need to pull the changes locally, you'll need to do:

git fetch origin
git reset --hard origin/qtbindingsnotfounderror

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @StSav012!

@CAM-Gerlach CAM-Gerlach mentioned this pull request Mar 21, 2023
@StSav012
Copy link
Contributor Author

Thank you all! I don't have much spare time these days. @CAM-Gerlach, I do appreciate that you've taken matters into your hands.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here @StSav012 !

@dalthviz dalthviz merged commit 84d3643 into spyder-ide:master Mar 21, 2023
@StSav012 StSav012 deleted the moved-modules-in-qt6 branch March 23, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make QtWidgets and QtGui modules compatible with PySide6/PyQt6 import locations for PySide2/PyQt5
4 participants