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

STYLE: Merge common code using ctkMessageBox #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JEHoctor
Copy link
Collaborator

I saw an opportunity to increase code-reuse surrounding the ctk.ctkMessageBox dialog. In addition, several functions can become static methods.

@JEHoctor JEHoctor requested a review from jcfr July 16, 2020 02:26
@JEHoctor
Copy link
Collaborator Author

Let's not merge this right away as we need to give Lucia et al. time to evaluate PRs #45 and #47.

Copy link
Contributor

@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.

Thanks for working on this.

I suggest to remove the display of Q3DCLogic.warningMessage(' Node not modified') and instead use slicer.util.confirmYesNoDisplay

The fact the node has not been modified is not relevant to the user and require the user to click on OK

@JEHoctor
Copy link
Collaborator Author

I was thinking of using slicer.util.delayDisplay for that. Note that as currently written, that message is displayed with the default is_query=False parameter, and so only an "OK" button will be on that dialog. It looks like slicer.util.confirmYesNoDisplay has both a "yes and a "no".

@bpaniagua
Copy link
Contributor

Hi @JEHoctor

Can you please work on a commit to address the suggestion done by JC?
Thank you!

Bea

@bpaniagua
Copy link
Contributor

Resurrecting this issue. Should we close this MR or complete it and merge it @jcfr

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

Successfully merging this pull request may close these issues.

3 participants