See pull request #77 on Github, and also here:
https://github.com/NifTK/MITK/commit/e01e4bc989ccf6b94c0f0f50a358f5950f52d3e4

Thanks

Matt

To add to this. The problem is, that when saving a temporary file, the code generates a random file name and then iterates through the list of file extensions, trying each one until it works. But it incorrectly adds an extra dot.

So, the workaround is to make sure the xml has fileExtension attributes with a comma separated list of file extensions WITHOUT dots. Its only a problem when the XML has leading dots.

So, this change should handle both.

M

And I also tried to tighten up the error handling, as some exceptions were squashed.

Hi Sascha,

I did your first two comments about change fileName.size() == 0 to fileName.IsEmpty() and also const correctness.

But, Im not sure about the QTemporaryFile suggestions. The way it is currently written, the files are created in one method, and removed when the QProcess completes. How would this work with QTemporaryFile. I would have to store pointers/references to the object throughout the lifetime of the QProcess. Also, the user can specify the temporary directory via user preferences. So, QTemporaryFile would remove that facility wouldn't it?

Please advise.

M

(In reply to Matt Clarkson from comment #4)

I did your first two comments about change fileName.size() == 0 to
fileName.IsEmpty() and also const correctness.

Great.

But, Im not sure about the QTemporaryFile suggestions. The way it is
currently written, the files are created in one method, and removed when the
QProcess completes. How would this work with QTemporaryFile. I would have to
store pointers/references to the object throughout the lifetime of the
QProcess. Also, the user can specify the temporary directory via user
preferences. So, QTemporaryFile would remove that facility wouldn't it?

The QTemporaryFile stuff was a late night idea... if it proves to be too much of "refactoring", we should just stay with the current way of doing it. Ideally, we would use QTemporaryFile to create the file and keep it open until we finished writing to it. This avoids race conditions when accessing tmp files from multiple processes.

I think we could at least re-use the temporary name generation of QTemporaryFile, which is more robust then the hand written one. I am also not sure about the benefits of providing a custom temporary path.

But don't put too much effort into it...

I just pushed to bug-18067-file-extensions-on-save
What do you think?

M

Nice!

Just a last small idea:

In the QmitkCmdLineModuleRunner, could we change the QList<QTemporaryFile*> to a QHash<QString,QTemporaryFile> (parameter name -> tmp file) and let the SaveTemporaryImage(...) method insert the QTemporaryFile in the hash itself instead of returning a pointer (it could return the error message instead)?

The idea is to not use QTemporaryFile objects on the heap, so that the compiler takes core of the destruction. You might have to do m_TmpFiles["paramName"].setFileTemplate(...) because of the missing copy constructor for QTemporaryFile. If that doesn't work out, we can use "this" as the parent for QTemporaryFile objects, so we also don't need to clean them up manually.

What do you think? If you are tired of suggestions, I would be happy to merge it as is :-) (except for that master merge commit in the branch...)

I just tried that on the train into work. QTemporaryFile has a private copy constructor. So as soon as you try putting one in a QHash, compiler complains. Also, using "this" may not be a good idea. The QmitkCmdLineModuleRunner derives from QWidget. Its displayed on screen. When the process finishes, currently, the ClearUpTemporaryFiles() is called. If we resort to destroying QTemporaryFile only when the widget is destroyed there will be a delay for as long as the widget is on screen. Is that a good idea?

You are right. It looked to me like the files are only deleted in the destructor of the widget - I didn't realize that the clean-up code is called from somewhere else too.

I would then cherry-pick your changes and close the PR, if you agree.

I agree on the basis

a) Its completely wrong now.
b) Anything is better than a)

:-)

M

User zelzer has pushed new remote branch:

bug-18067-cli-file-extensions-not-parsed-correctly

[0687df]: Merge branch 'bug-18067-cli-file-extensions-not-parsed-correctly'

Merged commits:

2014-09-11 21:23:16 Matt Clarkson [50b9bc]
Rename QmitkCmdLineModuleProgressWidget to QmitkCmdLineModuleRunner

2014-09-11 21:13:33 Matt Clarkson [5f23e4]
Use QTemporaryFile when running CLI

2014-09-11 08:10:46 Matt Clarkson [499f09]
Integrate Sascha's changes re: const correctness and fileName.isEmpty()

2014-09-08 14:47:44 Matt Clarkson [caf571]
Fix CLI file extension handling, improve error trapping

Conflicts:
Plugins/org.mitk.gui.qt.cmdlinemodules/src/internal/QmitkCmdLineModuleProgressWidget.cpp

I believe everything related to this bus has been merged. Please re-open if there are more issues.