Still a problem.

We discussed at the developer hangout making the filenames always track the node name.

http://www.slicer.org/slicerWiki/index.php/Developer_Meetings/20130514#To_discuss

Please give r22004 a try and let me know if it doesn't do the trick.

This looks great - thanks!

r22004 introduced the following pb:
1) Open "Download Sample Data" module
2) download MRHead

• it downloads a file named "MR-Head.nrrd" in C:/Users/Julien/AppData/Local/Temp/RemoteIO/MR-Head.nrrd
  3) Open "Save data" dialog
  4) Save scene (-> don't save the volume).
  5) close scene
  6) load saved scene
  -> it tries to load the non existing file "C:/Users/Julien/AppData/Local/Temp/RemoteIO/MRHead.nrrd" (notice the missing '-')

Interesting - I would say that is a special case and I wonder what we should do about it. What happens here is that the SampleData the loaded volume name doesn't match the filename. I guess there's an easy fix - I could just change the name of the temp file to match the volume name.

But still the use case that Julien pointed out could be problematic in other ways - basically the SampleData is stored in a transient directory and the scene could become invalid after a reboot. Probably the better thing would be to remove the storage node from the SampleData after loading so that the user will be explicitly prompted to save.

Removed storage node from sample data downloads r22162

I'm still not convinced :-)
1)a) Load a file from disk, in the "Add data" dialog, check the "options", change the node name to "fixed"
or
1)b) Load a file from disk, from the slice controller change the volume name to "fixed"
2) Save scene (don't check the volume)
3) close scene
4) reload scene
-> missing volume.

Ah - I see what you mean! :o

Is there a place we can move this easily in the file dialog code? Basically I think we want to set the text that appears in the File Name column to match the node name - that way the storage node is only modified if the save takes place.

Specifying the file name for a node to save is done here:
https://github.com/Slicer/Slicer/blob/master/Base/QTGUI/qSlicerSaveDataDialog.cxx#L769

Effectively changing the node filename is done automatically here:
https://github.com/Slicer/Slicer/blob/master/Base/QTGUI/qSlicerSaveDataDialog.cxx#L771

Therefore, the filename table widget item contains the filename of the node to save. If the row is unchecked, then nothing happens to the node:
https://github.com/Slicer/Slicer/blob/master/Base/QTGUI/qSlicerSaveDataDialog.cxx#L709

So I believe what can be done is to change the initial value of the filename table widget item:
https://github.com/Slicer/Slicer/blob/master/Base/QTGUI/qSlicerSaveDataDialog.cxx#L624

I still don't think it will solve the "uniqueness" of filenames. What if 2 nodes have the same names (names are not uniques, only IDs are (actually IDs are not really unique (because of scene view nodes), but it's a different issue :-) )? if you write them, they will overwrite each others.
I believe this can be done in the fixupFileName() method (if a list of other filenames is given as parameter).

Thanks for the pointers - I'll look into it.

I notice that a few tests are now failing because they assumed that the nodes loaded by sample data would still have storage nodes. I'll look into changing the tests so they don't make this assumption.

Related commit: r22162
See http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=22162

@Steve: Commit r22162 related to this issue seems sub-optimal, it is now causing the following tests to fail: py_vtkITKArchetypeScalarReaderFile and py_vtkITKArchetypeDiffusionTensorReaderFile
EDIT: I just read note 8979 and realized you are already aware of the problem.

Issues related to r22162 fixed by r22217.

Original issue related to file renaming still under investigation.

I think r22260 is a good compromise that handle the reported use cases.

Nice fix !

Comments about r22259:
a) I agree with your TODO: such code could probably be moved in the Data module logic.
b) the new filename might contain forbidden characters
c) this does not handle filename uniqueness

Comments about r22260:
Marking the storable node as "modified" is not exactly what we mean. Instead, we should inform that the storage node is no longer pointing to the right file. This can be done by vtkMRMLStorageNode:: InvalidateFile().
Even better, maybe when changing the filename (vtkMRMLStorageNode::SetFileName), InvalidateFile() could be called automatically ?

Thanks for the review J2 :)

Regarding r22259:
a) yes, eventually...
b) I tested some bad characters and the dialog box strips them already
c) No, but the user is prompted to avoid overwrite

Regarding r22260: maybe actually want vtkMRMLStorableNode::SetName to call this->StorableModifiedTime.Modified() since we have modified the storable node. I still think the right thing is to update the time stamp of the storable node itself is more logical, since that's the node the user actually did explicitly change. Invalidating the storage node is a side effect.

If you want we can leave this open but retarget for 4.4 since the current bad behavior has been fixed.

I think something might be goofy in the heuristic here, because I just saw an mrb with files called "xxx.nhdr.nrrd.nrrd.nrrd"

Closing resolved issues that have not been updated in more than 3 months