PR: Patch QFiledialog and offer new methods #122
Conversation
|
@Nodd better late than never. So this patch exposes the methods with Qt case, plus a 2, because they change the return values. So changing compatibility for existing code, is just adding a 2 and accepting a tupple instead of a single argument. Thoughts @ccordoba12 ? |
| QFileDialog.getOpenFileName2 = _getOpenFileName | ||
| QFileDialog.getOpenFileNames2 = _getOpenFileNames | ||
| QFileDialog.getSaveFileName2 = _getSaveFileName | ||
| QFileDialog.getExistingDirectory2 = _getExistingDirectory |
There was a problem hiding this comment.
Do these names exists in Qt? I mean, getOpenFileName2, etc?
There was a problem hiding this comment.
No they don't, but if we replace the normal methods, the return is changed, so it would be weird.
We can warn QFileDialog.getExistingDirectory to better use QFileDialog.getExistingDirectory2.
Unfortunately I cant run any test on this, cause I am either stuck in the exec loop or segfaulting (which points to the bigger problem of using these methods :-|). I think that in general is a very bad idea/bad practice to use this methods that call exec/modal directly. It is better and safer (as seen by the latest fixes of @rlaverde in spyder) to avoid using these methods :-\
There was a problem hiding this comment.
RuntimeError: no access to protected functions or signals for objects not created from Python there is no way to add tests for this PR cause testing methods that call exec block testing, even if I try to access them by name, I will not be able to call the methods.
|
Tests are failing. |
Wow really??!?! thanks for letting me know 😈 |
|
Jeje, just in case you haven't noticed it ;-p |
|
Yeah, this is definitely not testable. I would say we should not really encourage the use of the compat funcs. And we should warn the user when using methods that do not create python objects the user can access or interact with (QMessageBox and QFileDialog methods). Thoughts @ccordoba12 ? |
Ok, no problem with that. However, I don't agree too much with the addition of warnings in this PR because we will have to add code in Spyder to swallow them. |
Yes, I can remove it for now. My point is that we should move away from using those methods in Spyder as we cannot test them plus they sometimes lead to odd segfaults. It is always better to create the object first and then exec it (but we have the reference to the python object and can interact with it if needed on tests or avoid segfaults) like https://github.com/spyder-ide/spyder/pull/4870/files#diff-0308a6d064ae2a204bd97de4f832e1f4R1430 |
|
@goanpeca, could you finish this PR? |
|
I personally think that qtpy should not patch the imported Qt objects. The rationale is the same as I already expressed in #119 : side effects for people not even using qtpy should be avoided at all costs. For example, with this PR an application using perfectly valid PyQt4 code could break just because it imported a widget from spyder |
|
Ok, let's close this for now due to @cpascual's remarks and this being stalled. |
Fixes #26