Refactor qt notification and its test solve problem of segfaults #5138

Czaki · 2022-09-25T16:36:44Z

Description

During investigate why the qt notifications test fails with segfaults, I found that there exists Qdialog that is created without setting Parent that may cause segfaults (this is my assumption)

Type of change

Bug-fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

References

Try to fix segfaults showed in #5137

How has this been tested?

example: the test suite for my feature covers cases x, y, and z
example: all tests pass with my change
example: 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
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
If I included new strings, I have used trans. to make them localizable.
For more information see our translations guide.

…egfaults

codecov · 2022-09-25T16:49:50Z

Codecov Report

Merging #5138 (c75ca46) into main (8bb7e78) will increase coverage by 0.17%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #5138      +/-   ##
==========================================
+ Coverage   87.62%   87.79%   +0.17%     
==========================================
  Files         587      578       -9     
  Lines       50051    49432     -619     
==========================================
- Hits        43855    43398     -457     
+ Misses       6196     6034     -162

Impacted Files	Coverage Δ
napari/_qt/dialogs/qt_notification.py	`92.59% <86.95%> (+0.12%)`	⬆️
napari/_qt/_tests/test_qt_notifications.py	`95.72% <100.00%> (+0.03%)`	⬆️
napari/_vispy/visuals/clipping_planes_mixin.py	`88.88% <0.00%> (-5.56%)`	⬇️
napari/plugins/_tests/test_utils.py	`92.85% <0.00%> (-0.48%)`	⬇️
napari/conftest.py
napari/_tests/test_key_bindings.py
napari/types.py
napari/__init__.py
napari/experimental/__init__.py
napari/settings/__init__.py
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Czaki · 2022-09-25T16:50:44Z

failing is unrelated and connected with lack of remote resources required by the briefcase.

github-actions · 2022-09-25T17:08:12Z

Click here to download the docs artifacts
docs
(zip file)

andy-sweet

I don't understand this yet, but that doesn't mean it's not a good idea. If you can help clarify some of my questions and misunderstanding, can definitely switch to approval. Or maybe @Carreau has a better idea here.

andy-sweet · 2022-09-27T17:39:36Z

napari/_qt/dialogs/qt_notification.py

@@ -436,3 +405,32 @@ def _debug_tb(tb):
        print("Entering debugger. Type 'q' to return to napari.\n")
        pdb.post_mortem(tb)
        print("\nDebugging finished.  Napari active again.")
+
+
+class TracebackDialog(QDialog):


Is there any benefit to having the new dialog type TracebackDialog? It doesn't expose any new public attributes or methods and I think the behavior is the same (i.e. the code here is mostly the same, just swapping self with tbdialog.

I don't feel strongly here, but just wanted to check I understood the changes.

Oh, is it so you can patch it specifically in the test?

There are three reasons:
The Main is to change _enter_debug_mode into the method that is better for not leak memory.
The second one you mention: More specific patching.
And for last it looks better for me.

That works.

andy-sweet · 2022-09-27T17:44:45Z

napari/_qt/dialogs/qt_notification.py

@@ -358,38 +358,7 @@ def from_notification(
        if isinstance(notification, ErrorNotification):

            def show_tb(parent_):


Not directly related to these changes, but should the typing of ActionSequence be updated that each action callable associated with a NapariQtNotification passes through the parent (i.e. a QWidget) as the only argument?

I bring this up because I was confused about how the parent actually gets passed through. Is my understanding correct? That is occurs when the action callable is executed? And the parent is the same as the parent of the original notification (likely mainwindow or some main widget in napari)?

I have fixed ActionSequence definition. I'm not sure what the general idea for this mechanism was introduced by @Carreau.

My first idea is that it is introduced to manipulate NapariQtNotification but it is closed by the same event. So maybe it should just pass the parent object.

Might be a good idea to rename parent_? If I followed the existing logic properly here (and it's possible that I didn't!), then this will always be an instance of NapariQtNotification. In some sense that's the parent dialog of the dialog created here, but not in the sense of the QObject parent/child hierarchy.

andy-sweet · 2022-09-27T17:51:22Z

napari/_qt/_tests/test_qt_notifications.py

+    widget = QWidget()
+    qtbot.add_widget(widget)
+
+    dialog = NapariQtNotification.from_notification(notif, parent=widget)


Why does the dialog need a parent? From the QDialog docs, it seems fine to have a dialog as a top level widget without a parent. And then qtbot should handle closing it, which should also eventually free up memory.

Not saying you're wrong to do this, but I don't understand this change yet.

The NapariQtNotification dialog doe not need to have a set parent, as it could be handled by qtbot, but without this, I do not know how to handle TracebackDialog to be guarded by qtbot

Ah, I get it, the Qt parent hierarchy is:

QWidget NapariQtNotification TracebackDialog

Previously, we just tracked the instance of NapariQtNotification, which didn't have a parent, but that means TracebackDialog wasn't tracked by qtbot. So parent here is representing some main widget or window in napari.

github-actions · 2022-09-27T19:09:42Z

Click here to download the docs artifacts
docs
(zip file)

andy-sweet

Looks good to me, but it's probably worth @Carreau taking a look too.

github-actions · 2022-09-27T21:35:43Z

Click here to download the docs artifacts
docs
(zip file)

github-actions · 2022-09-28T10:24:35Z

Click here to download the docs artifacts
docs
(zip file)

* refactor qt notification and its test to hopefully solve problem of segfaults * update mocking * fix ActionSequence definition * update `show_tb` argument name * add information about current qtbot guard structure to comment

refactor qt notification and its test to hopefully solve problem of s…

1b7729c

…egfaults

github-actions bot assigned Czaki Sep 25, 2022

github-actions bot added qt Relates to qt tests Something related to our tests labels Sep 25, 2022

Czaki requested a review from Carreau September 25, 2022 16:50

update mocking

b62b68c

Czaki requested a review from andy-sweet September 26, 2022 19:15

andy-sweet requested changes Sep 27, 2022

View reviewed changes

fix ActionSequence definition

f7f44a5

andy-sweet approved these changes Sep 27, 2022

View reviewed changes

update show_tb argument name

4ba9d1a

Carreau approved these changes Sep 28, 2022

View reviewed changes

add information about current qtbot guard structure to comment

c75ca46

andy-sweet merged commit 679331f into napari:main Sep 30, 2022

Czaki deleted the refactor/qt_notifications_tests branch September 30, 2022 08:51

Czaki added this to the 0.5.0 milestone Sep 30, 2022

Czaki modified the milestones: 0.5.0, 0.4.18 Jun 16, 2023

Czaki mentioned this pull request Jun 16, 2023

v0.4.18 - release process #5911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor qt notification and its test solve problem of segfaults #5138

Refactor qt notification and its test solve problem of segfaults #5138

Czaki commented Sep 25, 2022

codecov bot commented Sep 25, 2022 •

edited

Loading

Czaki commented Sep 25, 2022

github-actions bot commented Sep 25, 2022

andy-sweet left a comment

andy-sweet Sep 27, 2022

andy-sweet Sep 27, 2022

Czaki Sep 27, 2022

andy-sweet Sep 27, 2022

andy-sweet Sep 27, 2022

Czaki Sep 27, 2022

andy-sweet Sep 27, 2022

Czaki Sep 27, 2022

andy-sweet Sep 27, 2022

Czaki Sep 27, 2022

andy-sweet Sep 27, 2022

Czaki Sep 27, 2022

github-actions bot commented Sep 27, 2022

andy-sweet left a comment

github-actions bot commented Sep 27, 2022

github-actions bot commented Sep 28, 2022

		@@ -358,38 +358,7 @@ def from_notification(
		if isinstance(notification, ErrorNotification):

		def show_tb(parent_):

Refactor qt notification and its test solve problem of segfaults #5138

Refactor qt notification and its test solve problem of segfaults #5138

Conversation

Czaki commented Sep 25, 2022

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Sep 25, 2022 • edited Loading

Codecov Report

Czaki commented Sep 25, 2022

github-actions bot commented Sep 25, 2022

andy-sweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 27, 2022

andy-sweet left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 27, 2022

github-actions bot commented Sep 28, 2022

codecov bot commented Sep 25, 2022 •

edited

Loading