Improve add_action implementation #460
Conversation
Simplify code, reduce cyclomatic complexity from level C to A
Simplify code, reduce cyclomatic complexity from level C to A
for more information, see https://pre-commit.ci
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…into improve_add_action
There was a problem hiding this comment.
Thank you @ramses44 for checking how to improve this! Left a question regarding some comments that where left in the changes. Also, maybe adding more test to this util could be worthy? Besides that this looks good to me 👍 Thank you again for giving this a check!
Also, what do you think about this @ccordoba12 ? and sorry for the out of the blue ping @StSav012 but do you have any comment on these changes? Probably you have a better understanding of the shortcomings that could come when changing this util function :)
| # 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 |
There was a problem hiding this comment.
Is this a pending change to be done?
|
Should this be a blocker for v2.4.1 given the breakage that's evidently causing per #458 ? |
| # 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 |
There was a problem hiding this comment.
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
Line 83 in 948c872
| ) | ||
| and len(args) >= 2 |
There was a problem hiding this comment.
Here, you lose the case when only an icon and a text are provided. Indeed, from
Line 87 in 948c872
args becomes ('text', ), i.e., len(args) == 1.
| return old_add_action(self, *args) | ||
| text, shortcut, *args = args | ||
| action = old_add_action(self, icon, text, *args) | ||
| action.setShortcut(shortcut) |
There was a problem hiding this comment.
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 issetShortcut(self, shortcut: Union[QKeySequence, QKeySequence.StandardKey, Optional[str], int]);
- for
PySide2, the valid call issetShortcut(self, shortcut: PySide2.QtGui.QKeySequence);
- for
PyQt6, the valid call issetShortcut(self, shortcut: Union[QKeySequence, QKeySequence.StandardKey, Optional[str], int]);
- and only for
PySide6, the valid calls aresetShortcut(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.
I thought that maybe this was a kind of simple improvement but now I see that this would need way more work to accomodate things with the new findings (thanks @StSav012 for giving this a check!). So not a blocker 👍 |
Since (according to the description) this is refactoring, @StSav012 knows this code quite well and his review shows that it has several problems, and @StSav012's PR #461 conflicts with this one, I think we should close it in favor of that one. |
Simplify code, reduce cyclomatic complexity from level C to A of add_action()