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

[Bug]: NavigationToolbar2QT should use @Slot annotation #27214

Closed
bersbersbers opened this issue Oct 27, 2023 · 2 comments · Fixed by #27215
Closed

[Bug]: NavigationToolbar2QT should use @Slot annotation #27214

bersbersbers opened this issue Oct 27, 2023 · 2 comments · Fixed by #27215
Labels
Milestone

Comments

@bersbersbers
Copy link
Contributor

bersbersbers commented Oct 27, 2023

Bug summary

https://doc.qt.io/qtforpython-6/tutorials/basictutorial/signals_and_slots.html#the-slot-class says

We recommend marking all methods used by signal connections with a @QtCore.Slot() decorator. Not doing causes run-time overhead due to the method being added to the QMetaObject when creating the connection. This is particularly important for QObject classes registered with QML, where missing decorators can introduce bugs.

NavigationToolbar2QT does not do that.

Code for reproduction

import os

from matplotlib.backends.backend_qt import NavigationToolbar2QT
from matplotlib.backends.backend_qtagg import FigureCanvasQTAgg
from PySide6.QtWidgets import QApplication

os.environ["QT_LOGGING_RULES"] = "qt.pyside.libpyside.warning=true"

app = QApplication()
canvas = FigureCanvasQTAgg()
NavigationToolbar2QT(canvas)

Actual outcome

qt.pyside.libpyside: Warning: Registering dynamic slot "home()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "back()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "forward()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "pan()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "zoom()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "configure_subplots()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "edit_parameters()" on NavigationToolbar2QT. Consider annotating with @Slot()
qt.pyside.libpyside: Warning: Registering dynamic slot "save_figure()" on NavigationToolbar2QT. Consider annotating with @Slot()

Expected outcome

Silence

Operating system

Windows 10 (also Ubuntu 23.10)

Matplotlib Version

3.8.0

Matplotlib Backend

QtAgg

Python version

Python 3.12.0

Jupyter version

irrelevant

Installation

pip

@QuLogic
Copy link
Member

QuLogic commented Oct 27, 2023

It could do that, but is that portable to PySide2 and PyQt? We would need compatibility with all Qt Python implementations, which may require lots of special casing.

@bersbersbers
Copy link
Contributor Author

I will provide a PR shortly which can be used as a basis for such testing.

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 a pull request may close this issue.

2 participants