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

Add basic tests for the ScreenshotDialog widget and fixes #6057

Merged
merged 22 commits into from Aug 19, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6bb162d
Add basic tests for the ScreenshotDialog widget
dalthviz Jul 13, 2023
eca7fa6
Fix tests on macOS by explicitly setting non-native dialog usage
dalthviz Jul 13, 2023
08e639e
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Jul 13, 2023
95a7942
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Jul 14, 2023
179c844
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Jul 14, 2023
933c4a3
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Jul 24, 2023
fc7a55a
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Jul 24, 2023
dd86ade
Update test to trigger found error with non-native dialog overwrite m…
dalthviz Jul 24, 2023
a0e61d3
Don't call super accept after saving file to prevent Qt overwrite mes…
dalthviz Jul 24, 2023
aa2c444
Prevent exec recursion call while still keeping dialog open to choose…
dalthviz Jul 24, 2023
8ee027b
Add validation for dialog super accept result before saving
dalthviz Jul 25, 2023
e246353
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Jul 25, 2023
a9d65d1
Update napari/_qt/dialogs/screenshot_dialog.py
dalthviz Jul 25, 2023
1a6f775
Use dialog result for validation
dalthviz Jul 25, 2023
667aaf3
Apply suggestions from code review
dalthviz Jul 26, 2023
49631d6
Merge branch 'main' into add_screenshot_dialog_test
GenevieveBuckley Aug 1, 2023
fea1e0f
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Aug 1, 2023
4d86b54
Merge branch 'main' into add_screenshot_dialog_test
dalthviz Aug 10, 2023
03d15d6
Apply suggestions from code review
dalthviz Aug 14, 2023
d032949
Change 'qtbot.waitUntil' for 'assert' when checking for file being ov…
dalthviz Aug 14, 2023
3ac9565
overwrote -> overwritten
jni Aug 17, 2023
b833681
Merge branch 'main' into add_screenshot_dialog_test
psobolewskiPhD Aug 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
89 changes: 89 additions & 0 deletions napari/_qt/dialogs/_tests/test_screenshot_dialog.py
@@ -0,0 +1,89 @@
import pytest
from qtpy.QtCore import QTimer
from qtpy.QtWidgets import QApplication, QFileDialog, QLineEdit, QMessageBox

from napari._qt.dialogs.screenshot_dialog import ScreenshotDialog
from napari.utils.history import get_save_history


@pytest.mark.parametrize("filename", ["test", "test.png", "test.tif"])
def test_screenshot_save(qtbot, tmp_path, 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
if "." not in filename:
assert ".png" in path
dalthviz marked this conversation as resolved.
Show resolved Hide resolved

# 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("")

qt_overwrite_shown = False

def handle_qt_overwrite_message():
for widget in QApplication.topLevelWidgets():
if isinstance(widget, QMessageBox):
dalthviz marked this conversation as resolved.
Show resolved Hide resolved
# test should not enter here!
widget.accept()
nonlocal qt_overwrite_shown
qt_overwrite_shown = True
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

imagen

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())

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


# setup dialog
dialog = ScreenshotDialog(
save_function, directory=str(tmp_path), history=get_save_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
QTimer.singleShot(100, handle_qt_overwrite_message)
dialog.accept()
qtbot.wait(100)
dalthviz marked this conversation as resolved.
Show resolved Hide resolved
assert not qt_overwrite_shown, "Qt non-native overwrite message was shown!"


def test_screenshot_overwrite_save(qtbot, tmp_path, monkeypatch):
"""Check overwriting file validation."""
filename = tmp_path / "test.png"
filename.write_text("")
dalthviz marked this conversation as resolved.
Show resolved Hide resolved

def save_function(path):
assert "test.png" in path

def overwrite_message(*args):
dalthviz marked this conversation as resolved.
Show resolved Hide resolved
box, parent, title, text, buttons, default = args
assert parent == dialog
assert title == "Confirm overwrite"
assert "test.png" in text
assert "already exists. Do you want to replace it?" in text
assert buttons == QMessageBox.Yes | QMessageBox.No
assert default == QMessageBox.No

return QMessageBox.Yes

# monkeypath custom overwrite QMessageBox usage
monkeypatch.setattr(QMessageBox, "warning", overwrite_message)
dalthviz marked this conversation as resolved.
Show resolved Hide resolved

dialog = ScreenshotDialog(
save_function, directory=str(tmp_path), history=get_save_history()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

)
qtbot.addWidget(dialog)
dialog.setOptions(QFileDialog.DontUseNativeDialog)
dialog.show()

# check dialog, set filename and trigger accept logic
assert dialog.windowTitle() == 'Save screenshot'
line_edit = dialog.findChild(QLineEdit)
line_edit.setText("test")
dialog.accept()
Czaki marked this conversation as resolved.
Show resolved Hide resolved
Czaki marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 7 additions & 5 deletions napari/_qt/dialogs/screenshot_dialog.py
Expand Up @@ -62,8 +62,10 @@ def accept(self):
QMessageBox.No,
)
if res != QMessageBox.Yes:
# standard accept return 1, reject 0. This inform that dialog should be reopened
super().accept()
self.exec_()
self.save_function(save_path)
return super().accept()
# return in this case since still a valida name for the
# file is needed and the dialog need to still be visible
return None
base_result = super().accept()
if base_result:
self.save_function(save_path)
return base_result
Copy link
Collaborator

Choose a reason for hiding this comment

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

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()

Copy link
Member Author

@dalthviz dalthviz Jul 25, 2023

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe just if self.result()