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: Fix mappings of instance method and slot alias (PyQt6 and PySide6) #308

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

kumattau
Copy link
Contributor

@kumattau kumattau commented Dec 23, 2021

I'm sorry, #305 is caused by my PR: #287.
I'm trying to fix and add tests.
(I sent a pull request to see if tests using QApplication would work with CI.)

NOTE: This PR can be closed without merging when more appropriate fixes and tests are provided.

Fixes #305

@dalthviz dalthviz added this to the v2.1.0 milestone Dec 23, 2021
@dalthviz dalthviz linked an issue Dec 23, 2021 that may be closed by this pull request
@kumattau
Copy link
Contributor Author

kumattau commented Dec 23, 2021

@dalthviz,

My tests using QApplication abort with (maybe) SIGSEGV.

../qtpy/tests/test_qtprintsupport.py::test_qprintdialog_functions ./.github/workflows/test.sh: line 53:  4217 Aborted                 (core dumped) python -I -bb -X dev -W error -m pytest ../qtpy --cov-config ../.coveragerc --cov-append

Is there test examples using QApplication ?
Or, can I do test with gdb like as follows to get backtrace ?

/usr/bin/gdb -quiet -batch -ex run -ex bt -ex quit --args python -bb -X dev -W error -m pytest 

@ccordoba12 ccordoba12 changed the title [WIP] Fix exec_ of PyQt6 and Add tests [WIP] Fix exec_ of PyQt6 and add tests Dec 23, 2021
@kumattau
Copy link
Contributor Author

kumattau commented Dec 23, 2021

The test seems to stall.
Can I stop already running workflow/job ?

ref:
https://stackoverflow.com/questions/58895283/stop-already-running-workflow-job-in-github-actions

Screenshot from 2021-12-24 01-41-35

@dalthviz
Copy link
Member

dalthviz commented Dec 23, 2021

Thanks @kumattau for taking the initiative to fix this!

My tests using QApplication abort with (maybe) SIGSEGV.

../qtpy/tests/test_qtprintsupport.py::test_qprintdialog_functions ./.github/workflows/test.sh: line 53: 4217 Aborted (core dumped) python -I -bb -X dev -W error -m pytest ../qtpy --cov-config ../.coveragerc --cov-append

Is there test examples using QApplication ?
Or, can I do test with gdb like as follows to get backtrace ?

My guess is that to instantiate stuff we can guide us from the test done in Spyder. For example: https://github.com/spyder-ide/spyder/blob/master/spyder/widgets/tests/test_browser.py . Also I think we will need to add to the tests requirements pytest-qt

Maybe @ccordoba12 or @CAM-Gerlach know better

The test seems to stall.
Can I stop already running workflow/job ?

I think you need to have write rights in the repo for that but I can stop them/cancel the worflow 👍

@kumattau
Copy link
Contributor Author

Thank you very much for stopping job @dalthviz !
I'm not going to test it because I can't stop the workflow/job myself.

I will study https://github.com/spyder-ide/spyder/blob/master/spyder/widgets/tests/test_browser.py.
I think this problem cannot be found without running.

@kumattau
Copy link
Contributor Author

kumattau commented Dec 23, 2021

I think the problem is fixed by changing all exec_ s like as follows,
but the test has not been completed yet.
I will create tests at the weekend if more appropriate fixes and tests are not provided.

QDialog.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)

@kumattau
Copy link
Contributor Author

Is it possible to set a timeout for job?

https://stackoverflow.com/questions/59073731/set-default-timeout-on-github-action-pipeline

I think it is reasonable to set 10 min timeout for job from recent action's result.

Screenshot from 2021-12-24 22-08-20

@dalthviz
Copy link
Member

dalthviz commented Dec 24, 2021

Is it possible to set a timeout for job?

Sure, I think that 10 min should be enough 👍

Edit; Not totally sure if that can be done in this same PR, but you want to check @kumattau ? It will be to add the timeout-minutes config into the https://github.com/spyder-ide/qtpy/blob/master/.github/workflows/ci.yml file

@kumattau
Copy link
Contributor Author

I will send ci-timeout PR in a minute.
After the PR is merged, I will create draft PR executing 15 min jobs to test timeout.

@kumattau kumattau force-pushed the pyqt6-exec_ branch 2 times, most recently from a11e492 to 8d75572 Compare December 26, 2021 00:04
@kumattau kumattau changed the title [WIP] Fix exec_ of PyQt6 and add tests [WIP] Fix method mappings and add tests Dec 26, 2021
@kumattau
Copy link
Contributor Author

This problem (exec(self): first argument of unbound method must have type 'QDialog') seems not to occur in QApplication.

But, maybe, the current following mapping is no good.

QApplication.exec_ = QApplication.exec

Because Subclass methods are not called properly.
I think in an alias method, a original method should be called via self instance.

class QApplication:
    def exec(self):
        print("QApplication exec")

class SubQApplication(QApplication):
    def exec(self):
        print("SubQApplication exec")

QApplication.exec_ = QApplication.exec
app = SubQApplication()
app.exec_()
# => QApplication exec (no good)

QApplication.exec_ = lambda self: QApplication.exec(self)
app = SubQApplication()
app.exec_()
# => QApplication exec (no good)

QApplication.exec_ = lambda self: self.exec()
app = SubQApplication()
app.exec_()
# => SubQApplication exec (good)

@dalthviz dalthviz modified the milestones: v2.1.0, v2.0.1 Jan 10, 2022
@dalthviz
Copy link
Member

Hi again @kumattau , maybe we could merge this as it is and I could give it a try to add tests in a following PR, what do you think? Let us know!

@kumattau
Copy link
Contributor Author

kumattau commented Jan 17, 2022

@dalthviz,

Sorry for the delay.

maybe we could merge this as it is and I could give it a try to add tests in a following PR

I think so, too. I may not study qtbot and create test codes right away because of busy my jobs.

These commits replace all method assignments with lambdas in PyQt6 and PySide6.
But I keep QDateTime.toPython = QDateTime.toPyDateTime assignment in PyQt5.
I think keeping current behavior is better than fixing the rare problem because PyQt5 is current default.
What do you think ?

I rebased these commits onto master branch but python3.6 tests were failed.
It seems that python3.6 tests are broken.

@dalthviz
Copy link
Member

I think so, too. I may not study qtbot and create test codes right away because of busy my jobs.

Awesome, and no problem thank you so much for checking into this 👍

These commits replace all method assignments with lambdas in PyQt6 and PySide6.
But I keep QDateTime.toPython = QDateTime.toPyDateTime assignment in PyQt5.
I think keeping current behavior is better than fixing the rare problem because PyQt5 is current default.
What do you think ?

Sounds good to me

I rebased these commits onto master branch but python3.6 tests were failed.
It seems that python3.6 tests are broken.

Yep, I did a fix for that at #313 . When that one gets merged a new rebase will be needed here to fix the tests

@kumattau kumattau marked this pull request as ready for review January 17, 2022 16:40
@kumattau kumattau changed the title [WIP] Fix method mappings and add tests [WIP] Fix method mappings Jan 17, 2022
Copy link
Contributor Author

@kumattau kumattau left a comment

Choose a reason for hiding this comment

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

maybe, the change of QLibraryInfo.location mapping is not needed because this is static method.

https://doc.qt.io/qt-6/qlibraryinfo.html

Copy link
Contributor Author

@kumattau kumattau left a comment

Choose a reason for hiding this comment

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

maybe, the change of QPrintPreviewWidget.print_ is not needed because this is public slot.

https://doc.qt.io/qt-6/qprintpreviewwidget.html

(update)
slot seems to be handled same as instance method.
(ref. QDialog.exec)

@CAM-Gerlach
Copy link
Member

#313 is merged now, FYI, so you can rebase you branch (please rebase rather than merging to keep the commit history cleaner, thanks) on master now, @kumattau . Thanks!

@kumattau
Copy link
Contributor Author

This problem (exec(self): first argument of unbound method must have type 'QDialog') seems not to occur in QApplication.

But, maybe, the current following mapping is no good.

QApplication.exec_ = QApplication.exec

Because Subclass methods are not called properly. I think in an alias method, a original method should be called via self instance.

class QApplication:
    def exec(self):
        print("QApplication exec")

class SubQApplication(QApplication):
    def exec(self):
        print("SubQApplication exec")

QApplication.exec_ = QApplication.exec
app = SubQApplication()
app.exec_()
# => QApplication exec (no good)

QApplication.exec_ = lambda self: QApplication.exec(self)
app = SubQApplication()
app.exec_()
# => QApplication exec (no good)

QApplication.exec_ = lambda self: self.exec()
app = SubQApplication()
app.exec_()
# => SubQApplication exec (good)

Sorry, I had understood it wrongly.
QApplication.exec_ = QApplication.exec is no problem because exec is static method.

https://doc.qt.io/qt-6/qapplication.html

I am checking all mappings again to fix instance method only.
I will push squashed and rebased commit after checking.

@kumattau
Copy link
Contributor Author

kumattau commented Jan 18, 2022

I will push squashed and rebased commit after checking.

@dalthviz

I pushed the commit.

  • QLibraryInfo.location, Qt.mightBeRichText, QCoreApplication.exec_, QGuiApplication.exec_ and QApplication.exec_ are static functions or class methods.
    These should keep the current mapping.
  • Others are instance method or slots.
    These have been modified to lambda functions so that the original methods can be accessed via self instances.
list

# static functions or class methods
QtCore.py:            QLibraryInfo.location = QLibraryInfo.path
QtCore.py:            Qt.mightBeRichText = guiQt.mightBeRichText
QtCore.py:            QCoreApplication.exec_ = QCoreApplication.exec
QtGui.py:             QGuiApplication.exec_ = QGuiApplication.exec
QtWidgets.py:         QApplication.exec_ = QApplication.exec

# instance methods or slots
QtCore.py:            QEventLoop.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtCore.py:            QDateTime.toPython = lambda self, *args, **kwargs: self.toPyDateTime(*args, **kwargs)
QtCore.py:            QTextStreamManipulator.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtCore.py:            QThread.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)

QtGui.py:             QDrag.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtGui.py:             QFontMetrics.width = lambda self, *args, **kwargs: self.horizontalAdvance(*args, **kwargs)
QtGui.py:             QTextDocument.print_ = lambda self, *args, **kwargs: self.print(*args, **kwargs)

QtPrintSupport.py:    QPrintPreviewWidget.print_ = lambda self, *args, **kwargs: self.print(*args, **kwargs) # slot
QtPrintSupport.py:    QPageSetupDialog.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtPrintSupport.py:    QPrintDialog.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)

QtSql.py:             QSqlDatabase.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtSql.py:             QSqlQuery.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtSql.py:             QSqlResult.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)

QtWidgets.py:         QDialog.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs) # slot
QtWidgets.py:         QMenu.exec_ = lambda self, *args, **kwargs: self.exec(*args, **kwargs)
QtWidgets.py:         QPlainTextEdit.print_ = lambda self, *args, **kwargs: self.print(*args, **kwargs)
QtWidgets.py:         QPlainTextEdit.setTabStopWidth = lambda self, *args, **kwargs: self.setTabStopDistance(*args, **kwargs)
QtWidgets.py:         QPlainTextEdit.tabStopWidth = lambda self, *args, **kwargs: self.tabStopDistance(*args, **kwargs)
QtWidgets.py:         QTextEdit.print_ = lambda self, *args, **kwargs: self.print(*args, **kwargs)
QtWidgets.py:         QTextEdit.setTabStopWidth = lambda self, *args, **kwargs: self.setTabStopDistance(*args, **kwargs)
QtWidgets.py:         QTextEdit.tabStopWidth = lambda self, *args, **kwargs: self.tabStopDistance(*args, **kwargs)

@kumattau kumattau changed the title [WIP] Fix method mappings Fix mappings of instance method and slot alias (PyQt6 and PySide6) Jan 18, 2022
@kumattau kumattau changed the title Fix mappings of instance method and slot alias (PyQt6 and PySide6) PR: Fix mappings of instance method and slot alias (PyQt6 and PySide6) Jan 18, 2022
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 @kumattau for all the help with this one! LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Qt6: exec_ vs. exec
3 participants