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

Port changes from upstream QVTKRWI. #1300

Merged

Conversation

prabhuramachandran
Copy link
Member

This ports several changes from upstream as a result this works well with PyQt6.

@larsoner
Copy link
Collaborator

larsoner commented May 21, 2024

The macos-latest failure can be fixed by using the macos-13 image it's meant to use. -latest recently changed to -14 and that's ARM64 rather than Intel so other changes are likely needed. (In principle we want both, but for expediency it's easiest to change here).

Then ideally some merge requirements could be set for the CIs (i.e., allow "merge when green" button clicking).

The other failure is probably an issue with PySide 6.7 so you could pin to PySide!=6.7 if you want, or try to fix it.

- Thanks to @larsoner for help with the macos-latest issue.
- The test failure seems some strange issue with recent PySide6 versions
  all the way back to 6.6.2.
@prabhuramachandran
Copy link
Member Author

The macos-latest failure can be fixed by using the macos-13 image it's meant to use. -latest recently changed to -14 and that's ARM64 rather than Intel so other changes are likely needed. (In principle we want both, but for expediency it's easiest to change here).

Thanks, I am trying that now.

Then ideally some merge requirements could be set for the CIs (i.e., allow "merge when green" button clicking).

Yes, the interface expects some tests in the last week so I wasn't able to add those.

The other failure is probably an issue with PySide 6.7 so you could pin to PySide!=6.7 if you want, or try to fix it.

It fails even with older versions and looks to be a bit bizarre. For now I am skipping this test with pyside6.

Can you please review the PR.

@prabhuramachandran
Copy link
Member Author

@larsoner -- could you please review the PR once so I can merge this and start the release?

@@ -787,4 +794,5 @@ def _qt_key_to_key_sym(key):


if __name__ == "__main__":
print(PyQtImpl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to delete debug print output?

Suggested change
print(PyQtImpl)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of the original upstream version and is in the __main__ block so I think it is fine as it is convenient when debugging without having to edit the source.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it thanks!

Copy link
Collaborator

@larsoner larsoner 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!

@larsoner
Copy link
Collaborator

I would merge, but it looks like my approval is insufficient to enable the green button

@prabhuramachandran
Copy link
Member Author

prabhuramachandran commented May 23, 2024

I would merge, but it looks like my approval is insufficient to enable the green button

@dpinte @mdickinson any ideas on how this could be done?

@mdickinson
Copy link
Member

Hi @prabhuramachandran. I've (temporarily?) disabled the "Require approvals" settings in the branch protections for master, so that this PR can be merged. We should discuss (possibly by email) a longer-term solution.

@tkoyama010
Copy link
Contributor

tkoyama010 commented May 23, 2024

Hi @prabhuramachandran. I've (temporarily?) disabled the "Require approvals" settings in the branch protections for master, so that this PR can be merged. We should discuss (possibly by email) a longer-term solution.

We has set up a GitHub Bot so that when an admin user makes an LGTM comment on a PR of which they are the author, the GitHub Bot approves that PR. This allows admins to merge their PRs as they wish, while leaving the rule that admins approve other PRs.
pyvista/pyvista#4941
Edit: I can create PR for this if you want :)

@prabhuramachandran
Copy link
Member Author

Hi @prabhuramachandran. I've (temporarily?) disabled the "Require approvals" settings in the branch protections for master, so that this PR can be merged. We should discuss (possibly by email) a longer-term solution.

Thank you, yes let us discuss this via email. For now this is fine.

@prabhuramachandran
Copy link
Member Author

@tkoyama010 -- Thank you for the offer. I am not sure about this and how this would work. Let me first discuss this and let you know. Thank you for the kind offer though.

@prabhuramachandran prabhuramachandran merged commit 284c8e9 into enthought:master May 23, 2024
11 checks passed
@prabhuramachandran prabhuramachandran deleted the update-qt-interactor branch May 23, 2024 11:01
@mdickinson
Copy link
Member

Thank you, yes let us discuss this via email.

Email sent ... Let me know if you didn't receive anything.

@prabhuramachandran
Copy link
Member Author

Thank you, yes let us discuss this via email.

Email sent ... Let me know if you didn't receive anything.

Got it, thank you. Sorry for the slow response.

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.

None yet

4 participants