BUG: avoid app freeze on mac

[pieper]

After recent changes to allow native file browsers,
clicking the button would apparently create a dialog
on mac but it was not visible. Because it was modal
the app would be alive but unresponive.

This change replaces show() and raise() which work
for QDialog with exec() which is apparently needed
for the native dialog to work properly.

https://discourse.slicer.org/t/slicer-is-freezing-on-macos-when-clicking-on-import-dicom-files/18624

Co-authored-by: Andras Lasso lasso@queensu.ca

[lassoan]

Raise+show was an undocumented way of displaying a popup, so it is better to switch to exec.

[lassoan]

We could probably don't need this->ImportDialog->setWindowModality(Qt::ApplicationModal); (I think this is the default for modal popups already).

We may also consider using exec() for the query popup, too (instead of raise+show).

[pieper]

We could probably don't need this->ImportDialog->setWindowModality(Qt::ApplicationModal); (I think this is the default for modal popups already).

At least with raise and show this change made the dialog appear, but you could still click on buttons and interact with the app even though the dialog remained on top, so very unconventional behavior.

We may also consider using exec() for the query popup, too (instead of raise+show).

I never saw bad behavior with that one, probably because it's a non-native dialog, so I left it as-is.

[lassoan]

I never saw bad behavior with that one, probably because it's a non-native dialog, so I left it as-is.

OK. Maybe just add a comment there, that exec may work better if issues are observed (appears on the wrong monitor, etc.).

[jamesobutler]

May want to already begin using open() over exec(). The note in the documentation says

Note: Avoid using this function; instead, use open(). Unlike exec(), open() is asynchronous, and does not spin an additional event loop. This prevents a series of dangerous bugs from happening (e.g. deleting the dialog's parent while the dialog is open via exec()). When using open() you can connect to the finished() signal of QDialog to be notified when the dialog is closed.

https://doc.qt.io/qt-5/qdialog.html#exec
https://doc.qt.io/qt-5/qdialog.html#open

[pieper]

May want to already begin using open() over exec().

This behaves the same as raise() and show() (at least on mac with current Qt). Probably because it's a native dialog so let's stick with exec for now.

It turns out ctkDICOMQueryRetrieveWidget is a QWidget not a QDialog so it doesn't have an exec().

So I think we're good with this and ready to merge.

[lassoan]

I agree, the current content of the PR looks good to me.