ENH: Add loadable warning in case of loading failure

[cpoetteCertis]

The loadable warning flag can be used to manage loading failure if not empty

[jcfr]

Thanks for the contribution 🙏

Could you provide more details regarding the workflow that required you to explicitly set the warning attribute of the DICOMLoadable ¹ ?

My understanding is that the warning is usually set in the loading code of specific plugin. I suspect a specific plugin is not setting the warning attribute which in turn prevent the following code from working as expected:

Slicer/Modules/Scripted/DICOMLib/DICOMBrowser.py

Lines 561 to 575 in 3f64c37

	def warnUserIfLoadableWarningsAndProceed(self):
	warningsInSelectedLoadables = False
	details = ""
	for plugin in self.loadablesByPlugin:
	for loadable in self.loadablesByPlugin[plugin]:
	if loadable.selected and loadable.warning != "":
	warningsInSelectedLoadables = True
	logging.warning(_('Warning in DICOM plugin {load_type} when examining loadable {name}: {message}').format(
	load_type=plugin.loadType, name=loadable.name, message=loadable.warning))
	details += loadable.name + " [" + plugin.loadType + "]: " + loadable.warning + "\n"
	if warningsInSelectedLoadables:
	warning = _("Warnings detected during load. Examine data in Advanced mode for details. Load anyway?")
	if not slicer.util.confirmOkCancelDisplay(warning, parent=self, detailedText=details):
	return False
	return True

cc: @pieper @lassoan

Footnotes

1. https://github.com/Slicer/Slicer/blob/3f64c3780139447be3b0f2bbee3896dd3716ac54/Modules/Scripted/DICOMLib/DICOMPlugin.py#L24-L64 ↩

[lassoan]

I agree that it could be useful to provide a mechanism to determine result of a loading, but warning property of DICOMLoadable class is used for something else (for storing result of examine step as a user-displayable warning message), so we should not overwrite this property during loading.

We could specify new properties in the loadable, such as loadingSuccess (True or False) and loadingMessage (user-displayable message about the loading result). Or, we could add an optional loadSuccess (list of flags, one for each loadable) or loadResults argument (list of success flag, message, maybe evennodeIDs for each loadable) to loadLoadables method that would return more detailed information about which loadable failed and why.

@cpoetteCertis Could you write a bit more about your use case? What information do you need and what do you do with it?

[cpoetteCertis]

Dear All,

Both solutions seems suitable for us!

Here are more details:
Depending on the loading issues, a node can be added (for example if irregular volume geometry is detected, the user can keep it or discard it). In that case, we need to provide all the necessary information for the given patient.
Or the node is not added (for example if a scalar volume could not be loaded), in that case, no need to go further.

The problem is that currently we have no information about the loading state.

Thanks to this modification, we simply check if the warning message is empty (success) or not empty (failure or issue while loading). If it is not empty, we do checks to know if a node has been created or not...

But with your suggestions, it could be much easier!

[lassoan]

@jcfr do you have any preference (add a list argument that is updated with loading status; add properties to each loadable object)?