The code snippet I provided in #2364 is just a draft, there are several points to fix:

• The argument figsize should be size
• I think you accidentally delete initializations of other attributes such as fig, canvas
• I think you accidentally delete the class doc.

Sorry, I got ahead of myself. I added back the doc string and the other missing fields. The syntax of the constructors is great but unfamiliar, so I added some unit tests to make sure the constructors are behaving as expected i.e. a constructor with only the parent or figsize and dpi are recognized as defaults to be compatible with pyuic5 and the matplotlib syntax that the original author emulated.

Hi @Dolphindalt

Thanks for the PR, appreciate you adding some extra tests!

We may need to remove the entry in pyqtgraph/__init__.py as that change makes matplotlib a hard dependency for which we definitely want to avoid.

CI test failure:

pyqtgraph system info
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/runner/work/pyqtgraph/pyqtgraph/pyqtgraph/__init__.py", line 279, in <module>
    from .widgets.MatplotlibWidget import *
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/shiboken2/files.dir/shibokensupport/feature.py", line 139, in _import
    return original_import(name, *args, **kwargs)
  File "/home/runner/work/pyqtgraph/pyqtgraph/pyqtgraph/widgets/MatplotlibWidget.py", line 1, in <module>
    from matplotlib.backends.backend_qt5agg import FigureCanvasQTAgg as FigureCanvas
  File "/usr/share/miniconda3/envs/test/lib/python3.8/site-packages/shiboken2/files.dir/shibokensupport/feature.py", line 139, in _import
    return original_import(name, *args, **kwargs)
ModuleNotFoundError: No module named 'matplotlib'

Read the docs failure:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyqtgraph/envs/2366/lib/python3.10/site-packages/sphinx/config.py", line 343, in eval_config_file
    exec(code, namespace)
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyqtgraph/checkouts/2366/doc/source/conf.py", line 24, in <module>
    import pyqtgraph
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyqtgraph/checkouts/2366/doc/source/../../pyqtgraph/__init__.py", line 279, in <module>
    from .widgets.MatplotlibWidget import *
  File "/home/docs/checkouts/readthedocs.org/user_builds/pyqtgraph/checkouts/2366/doc/source/../../pyqtgraph/widgets/MatplotlibWidget.py", line 1, in <module>
    from matplotlib.backends.backend_qt5agg import FigureCanvasQTAgg as FigureCanvas
ModuleNotFoundError: No module named 'matplotlib'

We can put that line in __init__.py wrapped in something like

if spec := importlib.util.find_spec('matplotlib') is not None:
    from .widgets.MatplotlibWidget import *

This PR also introduces more typing annotations than we have in the library so far. This is fine; we want type annotations!

Initially, I was going to suggest fully defining the type annotations for each of the overloads, but as we have no mechanism to test/check those annotations, probably best to leave it as is.

Could you add pytest.importorskip("matplotlib") to test_matplotlibwidget.py like in
https://github.com/pyqtgraph/pyqtgraph/blob/master/tests/exporters/test_matplotlib.py?
After adding this, it would not be necessary to pull in matplotlib into the conda environments in order to get it to pass.

@pijyoi beat me to it with the pytest.importskip suggestion.

Tests look good, once the unused import is removed, this is ready for merging. Thanks for the PR @Dolphindalt

Thank you reviewers for being so patient with me despite my many mistakes. Learned a lot from doing this PR. Hopefully I can do more in the future with less mistakes.

There is one more problem that the argument size has been changed to figsize, thanks to early discussion in #2364, I think it is not a good option to do this.

Thank you reviewers for being so patient with me despite my many mistakes. Learned a lot from doing this PR. Hopefully I can do more in the future with less mistakes.

Don't beat yourself up about it (or give it a second thought), some of my PRs get loads of rounds of updates/changes requested; happens to everyone. I periodically refer to PR 1318 (intentionally not hot-linking it) as it shows how many things I goofed on during creation/editing. And there were still issues after it was merged.

There is one more problem that the argument size has been changed to figsize, thanks to early discussion in #2364,

oh right! please verify the argument names are appropriate as well (I don't use matplotlib much so I'm not much of an authority here)

I think it is not a good option to do this.

@haiiliin I'm confused by this sentence, are you suggesting this PR isn't a good idea, or that merging in the current state?

I think it is not a good option to do this.

@haiiliin I'm confused by this sentence, are you suggesting this PR isn't a good idea, or that merging in the current state?

I mean changing size to figsize is not a good option to do, sorry i didn't make it clear.

@haiiliin got it, thanks for reviewing.

It wouldn't need just the overloaded signature to have size instead of figsize but the kwargs.get('figsize') calls would need to be changed to kwargs.get('size') as well, no?

@haiiliin got it, thanks for reviewing.

It wouldn't need just the overloaded signature to have size instead of figsize but the kwargs.get('figsize') calls would need to be changed to kwargs.get('size') as well, no?

I think so.

The change of size to figsize was made because figsize is more descriptive of what the parameter is and it mirrors the Matplotlib Figure constructor prototype that is called internally in the MatplotlibWidget class constructor.

# Matplotlib Figure constructor first arguments
class matplotlib.figure.Figure(figsize=None, dpi=None, ...)

# MatplotlibWidget.py line 46
self.fig = Figure(figsize, dpi=dpi)

It may not be appropriate to make this change because it will break existing code that is using the size parameter as a named argument. Please advise on what to do.

Is it appropriate to support both size and figsize, but raise a warning when size is used?

Passing keyword arguments after the parent argument does not work properly.

class QWidget:
    pass

class X:
    def __init__(self, *args, **kwargs):
        if (
            (args and not isinstance(args[0], QWidget))
            or
            (kwargs and "figsize" in kwargs or "dpi" in kwargs)
        ):
            print("AAA")
            figsize = args[0] if len(args) > 0 else kwargs.get("figsize", (5.0, 4.0))
            dpi = args[1] if len(args) > 1 else kwargs.get("dpi", 100)
            parent = args[2] if len(args) > 2 else kwargs.get("parent", None)
        else:
            print("BBB")
            parent = args[0] if len(args) > 0 else kwargs.get("parent", None)
            figsize = kwargs.get("figsize", (5.0, 4.0))
            dpi = kwargs.get("dpi", 100)
        print(f'{figsize=}, {dpi=}')

# neither of the following work correctly, both enter AAA branch
X(QWidget(), figsize=(1, 1))
X(QWidget(), dpi=200)

Passing keyword arguments after the parent argument does not work properly.

Will add unit tests to make sure these cases are behaving as expected + modifications to the constructor itself.

Passing keyword arguments after the parent argument does not work properly.

class QWidget:
    pass

class X:
    def __init__(self, *args, **kwargs):
        if (
            (args and not isinstance(args[0], QWidget))
            or
            (kwargs and "figsize" in kwargs or "dpi" in kwargs)
        ):
            print("AAA")
            figsize = args[0] if len(args) > 0 else kwargs.get("figsize", (5.0, 4.0))
            dpi = args[1] if len(args) > 1 else kwargs.get("dpi", 100)
            parent = args[2] if len(args) > 2 else kwargs.get("parent", None)
        else:
            print("BBB")
            parent = args[0] if len(args) > 0 else kwargs.get("parent", None)
            figsize = kwargs.get("figsize", (5.0, 4.0))
            dpi = kwargs.get("dpi", 100)
        print(f'{figsize=}, {dpi=}')

# neither of the following work correctly, both enter AAA branch
X(QWidget(), figsize=(1, 1))
X(QWidget(), dpi=200)

I think if you pass the parent as the first positional argument, you are supposed to call the method in Qt style without matplotlib arguments.

@typing.overload
def __init__(self, figsize=(5.0, 4.0), dpi=100, parent=None):
    pass

@typing.overload
def __init__(self, parent=None):
    pass

I think if you pass the parent as the first positional argument, you are supposed to call the method in Qt style without matplotlib arguments.

This behavior is supported along with providing named arguments in any order. This is typical Python behavior I think.

I think if you pass the parent as the first positional argument, you are supposed to call the method in Qt style without matplotlib arguments.

This behavior is supported along with providing named arguments in any order. This is typical Python behavior I think.

Right, but then I think you have to modify the overloaded signatures, because calling it this way does not match any of the overloaded signatures.

Right, but then I think you have to modify the overloaded signatures, because calling it this way does not match any of the overloaded signatures.

I tried to do this in the new commit. Please let me know if it is not what you have in mind.

I mean calling it in this way.

X(QWidget(), figsize=(1, 1))
X(QWidget(), dpi=200)

Perhaps we should define an overloaded method like this:

@typing.overload
def __init__(self, parent=None, size=(5.0, 4.0), dpi=100):
    pass

But anyway, I am not sure about making it this complicated is necessary.

@typing.overload
def __init__(self, figsize=(5.0, 4.0), dpi=100, parent=None):
     pass

@typing.overload
def __init__(self, parent=None, figsize=(5.0, 4.0), dpi=100):
    pass

So something like this? Both styles are supported this way and all parameters are present in each constructor, so there will only be two constructors.

Right, I think it is quite a good solution now.

@Dolphindalt Thank you so much for the PR, @haiiliin thank you so much for your continued comments/reviews, this LGTM, merging!