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: Make QAction.setShortcut and setShortcuts accept many types #461

Merged
merged 16 commits into from
Nov 9, 2023

Conversation

StSav012
Copy link
Contributor

Test QMenu.addAction with a shortcut of Qt.Key type as well. The argument type causes the following error on PySide2:

TypeError: 'PySide2.QtWidgets.QAction.setShortcut' called with wrong argument types:
  PySide2.QtWidgets.QAction.setShortcut(Key)
Supported signatures:
  PySide2.QtWidgets.QAction.setShortcut(PySide2.QtGui.QKeySequence)

This is a direct follow-up to #460.

Test `QMenu.addAction` with a shortcut of `Qt.Key` type as well. The argument type causes the following error on `PySide2`:
```plaintext
TypeError: 'PySide2.QtWidgets.QAction.setShortcut' called with wrong argument types:
  PySide2.QtWidgets.QAction.setShortcut(Key)
Supported signatures:
  PySide2.QtWidgets.QAction.setShortcut(PySide2.QtGui.QKeySequence)
```
@dalthviz dalthviz added this to the v2.4.2 milestone Oct 18, 2023
@StSav012 StSav012 changed the title Test QMenu.addAction with various shortcut types Make QAction.setShortcut and setShortcuts accept many types Oct 19, 2023
@StSav012
Copy link
Contributor Author

  • Bumped the max patched version to 6.4, for the Ubuntu builds of PySide6 == 6.3.2 still need the patches.
  • Fixed the omission of Qt.Key check in QMenu.addAction and QToolBar.addAction.
  • Fixed a crash when QAction.setShortcuts gets a single shortcut of QKeySequence.StandardKey.
  • Added tests for the cases.
  • Refactored a little.

As the PR clearly conflicts with #460, you may cherry-pick the changes to that PR or compose a new PR from scratch. I don't really care as long as it leads to the QtPy improvement.

@StSav012 StSav012 marked this pull request as ready for review October 19, 2023 17:31
@dalthviz
Copy link
Member

Thanks for the work here @StSav012 ! I think we could merge first this one since it improves the tests, does some refactoring already and most importantly fixes a crash, After that, then we will be able to reassess checking for ways to simplify the related code as #460 tries to do.

What do you think @ccordoba12 @CAM-Gerlach ?

@dalthviz dalthviz changed the title Make QAction.setShortcut and setShortcuts accept many types PR: Make QAction.setShortcut and setShortcuts accept many types Oct 24, 2023
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.

Thanks @StSav012 for your work on this!

qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/QtWidgets.py Outdated Show resolved Hide resolved
qtpy/QtWidgets.py Outdated Show resolved Hide resolved
qtpy/QtWidgets.py Outdated Show resolved Hide resolved
qtpy/QtWidgets.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
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 !

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!

@ccordoba12 ccordoba12 merged commit a5f2548 into spyder-ide:master Nov 9, 2023
13 checks passed
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.

3 participants