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

Add support for registering scripted IOOptionsWidget #6953

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Apr 20, 2023

No description provided.

@jcfr jcfr marked this pull request as draft April 20, 2023 07:12
@jcfr jcfr force-pushed the add-qSlicerScriptedIOOptionsWidget branch from 41af0a2 to 6808a72 Compare April 20, 2023 07:41
Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great, just added a few comments inline.

Base/QTCore/qSlicerCoreIOManager.cxx Outdated Show resolved Hide resolved
Base/QTCore/qSlicerCoreIOManager.cxx Outdated Show resolved Hide resolved
Base/QTCore/qSlicerCoreIOManager.h Outdated Show resolved Hide resolved
Base/QTCore/qSlicerScriptedFileReader.cxx Outdated Show resolved Hide resolved
Py_DECREF(arguments);
if (!result)
{
// Method call failed (probably an omitted function), call default implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that we do this all the time, but now that you are working on this and you are so knowledgable about Python integration, I would ask: is it possible to do this better? The main limitation of this is that if an uncaught exception is raised in the reimplemented method then it is silently ignored and it is assumed that the method is just not overridden.

Copy link
Member Author

@jcfr jcfr Apr 20, 2023

Choose a reason for hiding this comment

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

There is way to address this and simplify across Slicer, will do so in a follow-up PR.

Base/QTGUI/qSlicerScriptedIOOptionsWidget.h Outdated Show resolved Hide resolved
@jcfr jcfr force-pushed the add-qSlicerScriptedIOOptionsWidget branch from 6808a72 to d4f4dfa Compare April 20, 2023 16:09
@jcfr jcfr mentioned this pull request May 30, 2023
@jamesobutler jamesobutler force-pushed the add-qSlicerScriptedIOOptionsWidget branch from d4f4dfa to b442ad4 Compare June 27, 2023 17:46
@jamesobutler
Copy link
Contributor

jamesobutler commented Jun 27, 2023

I have rebased this PR to help with testing. Note that former commits in this PR were already integrated through PR:

Commits listed below:

@jcfr jcfr added the Type: Enhancement Improvement to functionality label Jun 28, 2023
@jcfr jcfr force-pushed the add-qSlicerScriptedIOOptionsWidget branch from b442ad4 to 81f1444 Compare October 26, 2023 21:58
@jcfr
Copy link
Member Author

jcfr commented Oct 26, 2023

@jamesobutler This is now ready for review & testing.

@jcfr jcfr marked this pull request as ready for review October 26, 2023 22:01
@jamesobutler jamesobutler self-requested a review October 26, 2023 22:54
@jamesobutler
Copy link
Contributor

@jcfr A few merge conflicts here with various recent integrations.

This change is implemented anticipating the integration of scripted
IOOptionsWidget that will required to override the options() method
and instantiate a qSlicerScriptedIOOptionsWidget object.
@jcfr jcfr force-pushed the add-qSlicerScriptedIOOptionsWidget branch from 81f1444 to 7f45704 Compare January 20, 2024 19:46
@jcfr jcfr force-pushed the add-qSlicerScriptedIOOptionsWidget branch from 7f45704 to c15664f Compare January 20, 2024 19:49
Copy link
Contributor

@jamesobutler jamesobutler left a comment

Choose a reason for hiding this comment

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

@jcfr Thanks for moving this work forward 🙏🏻 . Using this example I was able to see the progress you made to have options for a scripted file reader:
image

pass

def updateProperties(self):
self.parent.properties["coordinateSystem"] = vtkMRMLStorageNode.GetCoordinateSystemTypeFromString(
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying this update in my custom scripted file reader, it appears that the properties dictionary isn't updated with these new custom entries. It is only has fileName and fileType. Oddly reviewing self.parent.properties at the end of this method was revealing that the new entry wasn't added and therefore not used in the FileReader's load method which then uses properties as an input argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

it appears that the properties dictionary isn't updated

Thanks for testing 🙏 I also observed this. I have an idea of the issue and will follow up.

…dget (3)

Support updating "Properties" by updating qSlicerScriptedIOOptionsWidget
to derive its pimpl from qSlicerIOOptionsPrivate and updating the Q_PROPERTY
declaration to use "setProperties" as a write method.
@jcfr
Copy link
Member Author

jcfr commented Jan 22, 2024

@jamesobutler What we are still missing is the ability to implement examineFileInfoList from Python.

Specialization may allow to:

  • preprocess the list of files
  • define new properties (the method updateGui is called automatically after examineFileInfoList allowing the widget to update itself based on the new properties)

For example:

bool qSlicerVolumesReader::examineFileInfoList(QFileInfoList &fileInfoList, QFileInfo &archetypeFileInfo, qSlicerIO::IOProperties &ioProperties)const
{
//
// Check each file to see if it's recognized as part of a series. If so,
// keep it as the archetype and remove all the others from the list
//
foreach(QFileInfo fileInfo, fileInfoList)
{
itk::ArchetypeSeriesFileNames::Pointer seriesNames = itk::ArchetypeSeriesFileNames::New();
std::vector<std::string> candidateFiles;
seriesNames->SetArchetype(fileInfo.absoluteFilePath().toStdString());
candidateFiles = seriesNames->GetFileNames();
if (candidateFiles.size() > 1)
{
archetypeFileInfo = fileInfo;
QMutableListIterator<QFileInfo> fileInfoIterator(fileInfoList);
while (fileInfoIterator.hasNext())
{
const QString &path = fileInfoIterator.next().absoluteFilePath();
if (path == archetypeFileInfo.absoluteFilePath())
{
continue;
}
if (std::find(candidateFiles.begin(), candidateFiles.end(), path.toStdString()) != candidateFiles.end())
{
fileInfoIterator.remove();
}
}
ioProperties["singleFile"] = false;
return true;
}
}
return false;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improvement to functionality
Development

Successfully merging this pull request may close these issues.

None yet

3 participants