Fix QtDesigner crash in debug mode

[lassoan]

Debug-mode Qt Designer crashed when tried to instantiate a ctkMatrixWidget because of an assert failure.
It may be useful to know that some operations fail, but making the Qt Designer crash is a too strong way of notifying developers (especially when developers only use CTK and not intend to fix its potential internal errors).

Fixed by converting assert to a debug log.

See user problem report here: https://discourse.slicer.org/t/designer-will-not-load-with-slicer-widgets/5496/4

[pieper]

+1

I would prefer it if we could get rid of all asserts.

[lassoan]

@jcfr What do you think about the new CTK_VALIDATE macro that only logs a warning but does not terminate the application on failure? (I've forced push updated code that uses this macro)

[lassoan]

Done. Renamed macro to CTK_SOFT_ASSERT.

[jcfr]

One last nitpick, i suggest to use the do {...} while (0) idiom.

For reference, see https://stackoverflow.com/questions/923822/whats-the-use-of-do-while0-when-we-define-a-macro

[pieper]

I have never understood why people assume it's okay to ignore these conditions in release mode. I prefer either exceptions or the vtkErrorMacro followed by graceful cleanup approach for anything that really "shouldn't happen". When people use asserts they allow release mode code to silently ignore that the program has gotten into a bad state. If you are going to allow a user to go into a bad state then at least leave a log message.

[lassoan]

Fully agree.

Do you think we should log in release mode builds, too?

[jcfr]

should log in release mode builds, too

it depends ...

if we decide "yes", then in the case of this PR, the check in ctkMatrixWidgetPrivate::updateGeometries being not a critical error, I still recommend to do:

#ifndef QT_NO_DEBUG
CTK_SOFT_ASSERT(this->Table->columnWidth(j) == newWidth);
#endif

It basically started to be asserted with the transition to Qt5, it is something we should be fixed/investigated and having the debug version reporting a message is valuable. I looked into it not long ago but got side tracked.

[pieper]

For our typical use cases I think it's best if debug and release builds behave identically to the extent we can. That makes it easier to replicate user issues and compare log files.

[nolden]

For our typical use cases I think it's best if debug and release builds behave identically to the extent we can. That makes it easier to replicate user issues and compare log files.

I fully agree. The "assert" macro and its optimization in release mode doesn't make much sense unless it is in super time-critical loop which UI code probably isn't ;)

[lassoan]

Thanks for all the feedbacks. It is good that we all agree on this.

I've updated the macro to log warning regardless of build mode and it uses do{}while(0) idiom.

[jcfr]

[...]
/// in two key aspects: (1) it only logs a warning (instead of
/// terminating the application) and (2) the message is logged
/// in both release and debug mode builds (instead of ignoring
/// the condition in release mode).

[lassoan]

Thanks, fixed.