commontk / CTK

A set of common support code for medical imaging, surgical navigation, and related purposes.
https://commontk.org
Apache License 2.0
860 stars 490 forks source link

ctkMessageBox don't show again message overlaps informative text #585

Open naucoin opened 9 years ago

naucoin commented 9 years ago

If I make a ctkMessageBox that has both text and informativeText set, and then use the don't show again functionality, the informative text and the don't show again text overlap in one cell. This is due to the grid layout addWidget call, it doesn't automatically move the rows of widgets down when adding a new widget to the layout: https://github.com/commontk/CTK/blob/master/Libs/Widgets/ctkMessageBox.cpp#L203

To reproduce from a Slicer python console:

mb = ctk.ctkMessageBox() mb.setText('this is the text') mb.setInformativeText('this is the informative text') mb.dontShowAgainSettingsKey = 'dontshowagainsettingskeytest' mb.show() ctkmessagebox-info-dont-show

jcfr commented 9 years ago

@naucoin Thanks for reporting the issue.

Could you try using a modified version of setDontShowAgainVisible considering the following:

  [...]
  d->DontShowAgainCheckBox->setVisible(true);
  int fromRow = 1;
  if (!this->informativeText().isEmpty())
    {
    fromRow = 2;
    }
  grid->addWidget(d->DontShowAgainCheckBox, fromRow, 1, 1, 1);
  [...]

If it works as expected, would be great to extension the text do a PR. See https://github.com/commontk/CTK/blob/master/Libs/Widgets/Testing/Cpp/ctkMessageBoxDontShowAgainTest.cpp

Thanks Jc

naucoin commented 9 years ago

@jcfr I tried something similar to that already, it will then overlap with the row of buttons and causes the message box to not work very well at all (clicks randomly go to the check box or the button). The problem with the grid layout is that it doesn't actually support inserts + moving rows down, it just layers widgets over each other in the same cells. One Qt forum suggested moving to a combo of box layouts in order to support dynamic widget adjustment. I considered also always adding the checkbox to the widget but keeping it hidden until needed, but decided to submit a bug report to get some more feedback.