PR: Add missing kwargs to add_action wrapper#531
PR: Add missing kwargs to add_action wrapper#531beatreichenbach wants to merge 3 commits intospyder-ide:masterfrom
Conversation
508b392 to
8920bed
Compare
|
Hi @beatreichenbach thank you for giving that issue a check! The summary of possible signatures that you mention makes sense to me 👍 For completness, checking to the different bindings and got the following signatures when checking
So seems like one missing signature from the ones listed in the OP is passing a |
|
Okay thanks, I'll add a check for all of them and try to refactor that |
8920bed to
5b509df
Compare
for more information, see https://pre-commit.ci
| menu.addAction(icon, text, func, shortcut=shortcut) | ||
| menu.addAction(icon, text, receiver, member, shortcut) | ||
|
|
||
| # Qt 5 |
There was a problem hiding this comment.
These signatures are valid in Qt5 but not Qt6, do we still need to support them?
They won't error with Qt5 but will error with Qt6.
There was a problem hiding this comment.
I think they should still be available to ensure things with Qt5 bindings work. If no Qt6 bindings signature is usable with the passed arguments we should detect that case and probably at the very least show a warning explaining the situation (something like no Qt6 compatible signature is available please use one of the following to keep compatibility between Qt versions...).
There was a problem hiding this comment.
Hm, we'd have to also change this section here:
Line 225 in cc853c3
I feel like the error is pretty clear already:
AttributeError: PySide6.QtWidgets.QMenu.addAction(): unsupported keyword 'shortcut'
This way we're also ensuring that we're not adding unnecessary code for when the user uses a modern Qt6 binding.
Are you okay, if we just leave it as is?
| shortcut = kwargs.pop("shortcut", None) | ||
| connection_type = kwargs.pop("type", None) | ||
| if connection_type: | ||
| warnings.warn("type argument is not supported in Qt<6.3") |
There was a problem hiding this comment.
These signatures got added in 6.3 and won't error in Qt5 but type is not supported. How should this be handled?
There was a problem hiding this comment.
Showing a warning if someone uses the argument with a binding that doesn't support it makes sense to me
There was a problem hiding this comment.
If we leave the AttributeError, should we then also change this warning to an exception?
AttributeError: QtWidgets.QMenu.addAction(): unsupported keyword 'type'
| "QtPy with an icon and a QKeySequence shortcut", | ||
| QtGui.QKeySequence.UnknownKey, | ||
|
|
||
| # Actions |
There was a problem hiding this comment.
These tests have only been added to QMenu, in theory qtpy supports these for QToolBar as well.
But in Qt6 these also are valid for QWidget. But in Qt5 they don't exist at all.
Does QtPy provide backwards compatibility for everything? I'm a bit confused with that.
There was a problem hiding this comment.
As a little bit of context the original PR from where this patch approach comes is #437 The idea shown there only takes care of the more simple cases of compatibility for QMenu and QToolbar so I will say we currently only support backwards compatibility for QMenu and QToolbar addAction
There was a problem hiding this comment.
I added tests for QToolbar.
This adds the kwargs back to the add_action function and should hopefully fix #472.
However, before proceeding, I would like to add tests for all cases. I'm a bit unsure what cases should be supported. Does this cover everything?