Add basic tests for the ScreenshotDialog widget and fixes

[dalthviz]

Fixes/Closes

closes #6078

Description

Adds some tests to the ScreenshotDialog as a continuation of the effort started at #5864 to increase Qt widgets code coverage and fixes some errors with the non-native version (the one used when launching napari from and IPython console)

Notes and questions

• The tests call the dialog show method and also sets the non-native option so the tests here only cover the case when napari is being launched from an IPython console. The testing of the OS native version of the dialog is more involved.
• Checking the code related with the dialogs available I noticed that some of them (files/widgets classes) have the qt_* /Qt prefix or the _dialog/Dialog suffix but others don't (for example screenshot_dialog.py/ScreenshotDialog vs qt_about/QtAbout). Is that something worthy to standardize or maybe there is already some logic behind the naming?

Type of change

• Tests improvements/additions

How has this been tested?

• all tests pass with my change
• I check if my changes works with both PySide and PyQt backends
  as there are small differences between the two Qt bindings.

Final checklist:

• My PR is the minimum possible work for the desired functionality

[codecov]

Codecov Report

Merging #6057 (b833681) into main (670c971) will increase coverage by 0.04%.
The diff coverage is 98.18%.

@@            Coverage Diff             @@
##             main    #6057      +/-   ##
==========================================
+ Coverage   91.62%   91.67%   +0.04%     
==========================================
  Files         579      580       +1     
  Lines       50749    50800      +51     
==========================================
+ Hits        46498    46569      +71     
+ Misses       4251     4231      -20     

Files Changed	Coverage Δ
napari/_qt/dialogs/screenshot_dialog.py	93.10% <75.00%> (+58.62%)	⬆️
...apari/_qt/dialogs/_tests/test_screenshot_dialog.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[brisvag]

@dalthviz sorry this has been hanging so long! Maybe @Czaki could review, qt tests are always tricky :/

[dalthviz]

No problem @brisvag ! In fact I was rechecking this and updating it since seems like there is an issue about the screenshot dialog (#6078) which I'm able to reproduce locally but the tests are not picking up 🤔

[Czaki]

Wait. It looks like I was wrong here. Base on Qt documentation the accept() should return None (void in c++) https://doc.qt.io/qt-6/qdialog.html#accept

Suggested change

	if base_result:
	self.save_function(save_path)
	return base_result
	self.save_function(save_path)
	return base_result

If we would like to validate then we should depend on self.result()

[dalthviz]

Oh totally right! So the validation should be something like: if self.result() == QDialog.DialogCode.Accepted: maybe?

[Czaki]

maybe just if self.result()

[Czaki]

Few remarks

[Czaki]

Why do we use get_save_history here? Why not an empty list?

[dalthviz]

Will change it! I think what confused me was that in the ScreenshotDialog constructor the default value for the history kwarg is None

[Czaki]

maybe something like this:
https://github.com/4DNucleome/PartSeg/blob/b03118cdcbb109bef5a6905a70fee7bfb1e10db1/package/tests/test_PartSeg/conftest.py#L129-L142
instead of this iteration?

[dalthviz]

I ended up with this logic since I tried patching the warning method as in the link but the patch was not being triggered. Maybe patching exec could work 🤔. My initial guess was that since the warning QMessageBox is instantiated in Qt code (C++) the monkey patching didn't have any effect on it but I will check!

[dalthviz]

Checked and seems like even when monkey patching exec the Qt generated QMessageBox warning message keeps appearing.

If I do the monkey patch and revert the changes, to first call super().accept and then the save_function in the accept method for the dialog, the test hangs until I manually close the Qt generated QMessageBox warning:

Maybe I'm missing something in the monkey patch procedure @Czaki ? 🤔

@pytest.mark.parametrize("filename", ["test", "test.png", "test.tif"])
def test_screenshot_save(qtbot, tmp_path, monkeypatch, filename):
    """Check passing different extensions with the filename."""

    def save_function(path):
        # check incoming path has extension event when a filename without one
        # was provided
        assert filename in path
        assert "." in filename or ".png" in path

        # create a file with the given path to check for
        # non-native qt overwrite message
        with open(path, "w") as mock_img:
            mock_img.write("")

    def qt_overwrite_qmessagebox_warning(*_, **__):
        raise RuntimeError("exec_ call")

    monkeypatch.setattr(QMessageBox, "exec_", qt_overwrite_qmessagebox_warning)
    monkeypatch.setattr(QMessageBox, "critical", qt_overwrite_qmessagebox_warning)
    monkeypatch.setattr(QMessageBox, "information", qt_overwrite_qmessagebox_warning)
    monkeypatch.setattr(QMessageBox, "question", qt_overwrite_qmessagebox_warning)
    monkeypatch.setattr(QMessageBox, "warning", qt_overwrite_qmessagebox_warning)

    # setup dialog
    dialog = ScreenshotDialog(
        save_function, directory=str(tmp_path), history=[]
    )
    qtbot.addWidget(dialog)
    dialog.setOptions(QFileDialog.DontUseNativeDialog)
    dialog.show()

    # check dialog and set filename
    assert dialog.windowTitle() == 'Save screenshot'
    line_edit = dialog.findChild(QLineEdit)
    line_edit.setText(filename)

    # check that no warning message related with overwriting is shown
    dialog.accept()

    # check the file was created
    save_filename = filename if '.' in filename else f'{filename}.png'
    qtbot.waitUntil(lambda: (tmp_path / save_filename).exists())

[Czaki]

Oh. So it looks like Your procedure was better as maybe Qt uses inner references.

[psobolewskiPhD]

Thanks @dalthviz
I think the bug fix makes this for 0.4.19?