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

BUG: Fix crash instantiating ctkDICOMBrowser #977

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

jamesobutler
Copy link
Contributor

Following up to #973, this makes changes for ctkDICOMBrowser to not crash on initialization. @lassoan Would you like to make the additional DICOM related changes here now that the "Copy" import mode wouldn't be used? I'm a user that does not do any DICOM work so I do not know what would be appropriate behavior.

@jamesobutler jamesobutler requested a review from lassoan June 11, 2021 15:30
Copy link
Member

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Since ctkFileDialog (see [1]) already has an ivar called UsingNativeDialog, what about adding a convenience getter called ctkFileDialog::isUsingNativeDialog() ? Or would that be overkill ?

[1]

bool UsingNativeDialog;

Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkFileDialog.cpp Outdated Show resolved Hide resolved
ctkFileDialog now uses the native dialog by default which was causing an issue.
@jamesobutler jamesobutler force-pushed the dicom-browser-import-dialog branch from 0c2fc7c to 0cec298 Compare June 11, 2021 15:58
@jamesobutler
Copy link
Contributor Author

Since ctkFileDialog (see [1]) already has an ivar called UsingNativeDialog, what about adding a convenience getter called ctkFileDialog::isUsingNativeDialog() ? Or would that be overkill ?

Doesn't matter to me. If you think ivar's need a method associated with them instead of getting them directly, I could change it.

@jcfr jcfr force-pushed the dicom-browser-import-dialog branch from 0cec298 to 5cf320a Compare June 11, 2021 16:04
@jcfr
Copy link
Member

jcfr commented Jun 11, 2021

@jamesobutler I just fixed up the commit message prefix to be STYLE: instead of STYL:E. To easily catch this type of issue in the future, I also enabled the commitcheck GitHub app like we have in Slicer.

Regex is the same one ... (BUG|COMP|DOC|ENH|PERF|STYLE)\: \w+

@jcfr
Copy link
Member

jcfr commented Jun 11, 2021

Also just added the following check as required ...

image

@jcfr
Copy link
Member

jcfr commented Jun 11, 2021

Doesn't matter to me. If you think ivar's need a method associated with them instead of getting them directly, I could change it.

If we need to check for the status of the widget in application code, we may want to revisit. In the meantime, no need for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants