Skip to content

Improve add_action implementation #460

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Improve add_action implementation #460

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dd2013
Improve add_action implementation
ramses44 Oct 13, 2023
d6fa72d
Improve add_action implementation
ramses44 Oct 13, 2023
cc424ae
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 13, 2023
1a77685
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 13, 2023
948c872
Merge branch 'improve_add_action' of https://github.com/ramses44/qtpy…
ramses44 Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 26 additions & 59 deletions qtpy/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,68 +79,35 @@ def add_action(self, *args, old_add_action):
icon: QIcon
text: str
shortcut: QKeySequence | QKeySequence.StandardKey | str | int
receiver: QObject
member: bytes
if all(
isinstance(arg, t)
for arg, t in zip(
args,
[
str,
(QKeySequence, QKeySequence.StandardKey, str, int),
QObject,
bytes,
],
)
):
if len(args) == 2:
text, shortcut = args
action = old_add_action(self, text)
action.setShortcut(shortcut)
elif len(args) == 3:
text, shortcut, receiver = args
action = old_add_action(self, text, receiver)
action.setShortcut(shortcut)
elif len(args) == 4:
text, shortcut, receiver, member = args
action = old_add_action(self, text, receiver, member, shortcut)
else:
return old_add_action(self, *args)
return action
if all(
isinstance(arg, t)
for arg, t in zip(
args,
[
QIcon,
str,
(QKeySequence, QKeySequence.StandardKey, str, int),
QObject,
bytes,
],

# if args and isinstance(args[0], QIcon):
if any(
isinstance(arg, QIcon) for arg in args[:1]
): # Better to use previous line instead of this
Comment on lines +83 to +86
Copy link
Member

Choose a reason for hiding this comment

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

Is this a pending change to be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

The condition means that an instance of QIcon is not the last argument. However, in the next line, you imply that the instance is the first argument. This might work correctly, but only by chance. I'd rather use the commented out line

qtpy/qtpy/_utils.py

Line 83 in 948c872

# if args and isinstance(args[0], QIcon):
here.

icon, *args = args
else:
icon = QIcon()

if (
all(
isinstance(arg, t)
for arg, t in zip(
args,
[
str,
(QKeySequence, QKeySequence.StandardKey, str, int),
QObject,
bytes,
],
)
)
and len(args) >= 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, you lose the case when only an icon and a text are provided. Indeed, from

qtpy/qtpy/_utils.py

Line 87 in 948c872

icon, *args = args
, you get the icon, and the args becomes ('text', ), i.e., len(args) == 1.

):
if len(args) == 3:
icon, text, shortcut = args
action = old_add_action(self, icon, text)
action.setShortcut(QKeySequence(shortcut))
elif len(args) == 4:
icon, text, shortcut, receiver = args
action = old_add_action(self, icon, text, receiver)
action.setShortcut(QKeySequence(shortcut))
elif len(args) == 5:
icon, text, shortcut, receiver, member = args
action = old_add_action(
self,
icon,
text,
receiver,
member,
QKeySequence(shortcut),
)
else:
return old_add_action(self, *args)
text, shortcut, *args = args
action = old_add_action(self, icon, text, *args)
action.setShortcut(shortcut)
Copy link
Contributor

Choose a reason for hiding this comment

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

You might have to explicitly convert the shortcut to QKeySequence, for Qt accepts only that type of the argument here. See the Qt5 docs and the Qt6 docs.

For the latest packages, help(QAction.setShortcut) reads the following:

  • for PyQt5, the valid call is
    • setShortcut(self, shortcut: Union[QKeySequence, QKeySequence.StandardKey, Optional[str], int]);
  • for PySide2, the valid call is
    • setShortcut(self, shortcut: PySide2.QtGui.QKeySequence);
  • for PyQt6, the valid call is
    • setShortcut(self, shortcut: Union[QKeySequence, QKeySequence.StandardKey, Optional[str], int]);
  • and only for PySide6, the valid calls are
    • setShortcut(self, arg__1: Qt.Key) -> None,
    • setShortcut(self, shortcut: Union[QKeySequence, QKeyCombination, QKeySequence.StandardKey, str, int]) -> None.

It appears that all the flavors support a string as the shortcut. However, PySide2 fails when a Qt.Key is passed here. I admit, there is an omission in the tests.

I've composed #461 thanks to this PR. If you wish, you may cherry-pick the test from #461. I'm going to add a back port for QAction.setShortcut to call it with Qt.Key argument. You are welcome to prepare the latter PR before I do.

return action

return old_add_action(self, *args)


Expand Down