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: Various additional fixes for Qt 6 compatibility #22620

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

rear1019
Copy link
Contributor

@rear1019 rear1019 commented Oct 1, 2024

Description of Changes

Various fixes for Qt 6 compatibility. The changes are backward compatible with Qt 5.

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

rear1019

@pep8speaks
Copy link

pep8speaks commented Oct 1, 2024

Hello @rear1019! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-11-07 13:30:47 UTC

@rear1019 rear1019 force-pushed the qt6_fixes2 branch 2 times, most recently from 12e4250 to a4c3612 Compare October 1, 2024 16:47
@ccordoba12
Copy link
Member

Hey @rear1019, thanks a lot for you continuing help to improve our Qt6 support!

I'm going to leave this for 6.1 though (to be released in six months or so) because it includes a lot of changes that could introduce regressions to existing functionality.

And perhaps you'll be pleased to know that I have a WIP branch that makes Spyder start with PySide 6 (I'll try to push it soon to include it in 6.1 as well).

@ccordoba12 ccordoba12 requested a review from dalthviz October 1, 2024 17:51
@ccordoba12 ccordoba12 added this to the v6.1.0 milestone Oct 1, 2024
@ccordoba12 ccordoba12 changed the title PR: Various fixes for Qt 6 compatibility PR: Various additional fixes for Qt 6 compatibility Oct 1, 2024
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 @rear1019 for your work here and sorry for the late review! Left a couple of comments related with using the enum name to access some values but besides that the changes look good 👍

Also, thinking about the work here. it could be nice to have at some point the tests run over a job using PyQt6/PySide6 🤔

Anyhow, leaving this approved since technically QtPy should take care of accessing enum values without having to explicitly call via the enum.

Feel free to merge if you think this is ready @ccordoba12

spyder/plugins/shortcuts/widgets/table.py Outdated Show resolved Hide resolved
spyder/plugins/variableexplorer/widgets/dataframeeditor.py Outdated Show resolved Hide resolved
spyder/plugins/variableexplorer/widgets/dataframeeditor.py Outdated Show resolved Hide resolved
spyder/plugins/variableexplorer/widgets/dataframeeditor.py Outdated Show resolved Hide resolved
@ccordoba12
Copy link
Member

I agree with your suggestions @dalthviz. So @rear1019, please address them so we can merge your work.

The changes are backward compatible with Qt 5
There is no `called_once_with` method in `unittest.mock.Mocker`. Use the
probably intended `assert_called_once_with()`. Remove the now redudant
assert for `call_count`.
@rear1019
Copy link
Contributor Author

rear1019 commented Nov 7, 2024

I have applied the review suggestions and rebased the branch against current master.

@rear1019
Copy link
Contributor Author

rear1019 commented Nov 7, 2024

And perhaps you'll be pleased to know that I have a WIP branch that makes Spyder start with PySide 6 (I'll try to push it soon to include it in 6.1 as well).

@ccordoba12 Do you mind providing the patches? We intend to switch to Qt 6/PySide 6 this or next month, and are more than happy to help the project with the Qt 6 transition.

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 7, 2024

Do you mind providing the patches?

Will try to do it next week (I was waiting for this PR to be merged to rebase mine on top).

and are more than happy to help the project with the Qt 6 transition.

That's great, thanks! Would you be willing to open a PR adding a new CI slot to run our test suite with PyQt6 or PySide6? You could add a single slot for Linux to start with.

I think that's the best we could do to ensure that Spyder actually works with Qt6.

@ccordoba12 ccordoba12 merged commit a152e1c into spyder-ide:master Nov 7, 2024
17 checks passed
@rear1019
Copy link
Contributor Author

Would you be willing to open a PR adding a new CI slot to run our test suite with PyQt6 or PySide6?

Sorry for the slow response. I will look into that this or next week.

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.

4 participants