User:Barre/Slicer

A few notes about Slicer 3.0

• I updated the CMakeLists.txt to match the changes in KWWidgets
• Slicer3 would not compile for Windows in shared mode, because the dll export symbol was missing in all headers in Base/GUI classes; there is a VTK_SLICER_BASE_LOGIC_EXPORT inside Base/Logic, and a VTK_SLICER_BASE_EXPORT in the parent dir. I introduced a new one in the Base/GUI, VTK_SLICER_BASE_GUI_EXPORT, but we have to make sure we want a unique one for each sub-libs in Base/ (this is the case in VTK for each packages for example, say VTK_HYBRID_EXPORT for the Hybrid/ subdir), or a global one (which was the case some time ago in VTK, with a unique VTK_EXPORT for the whole tree). Also, you do not need to prefix the export symbol with VTK_ anymore, SLICER_BASE_EXPORT is fine, I updated the wrappers some time ago to support it :)
• A set of useful setup paths scripts are now automatically created using KWWidgets macros: check your bin directory for Slicer3SetupPaths.bat (or .sh, .csh), source them so that you can run Slicer3 executable on Win32, especially when it is built in shared lib mode. Without this script, it becomes pretty nightmarish to setup both the PATH, TCLLIBPATH, LD_LIBRARY_PATH (which name changes depending on the OS btw) alltogether. This is especially true since it seems the libraries in Slicer3 are in lib/ instead of bin/, where the executables are; very Win32 unfriendly :). This set of scripts will do all that for you, point to the right ITK, VTK and KWWidgets lib and bin dirs, set the TCLLIBPATH, etc. Ultimately you probably won't need that if you include all third-party libs in the repository (as links to their CVS repositories counterparts I assume).
• I found no less than 3 different instances of '#define SLICER_VTK5' and two of '#define VTKSLICER_STATIC'. This should probably be unified on top.
• We can talk about Tcl wrapping tomorrow, there is actually a macro in KWWidgets that sits on top of the macros in VTK and try to cope for various VTK versioning issues (I mentioned it in the main CMakeList.txt inside comments, and used it in Base/GUI).
• I ran Slicer3.exe, and a lot of warnings are issued because the pipeline is not setup correctly.
• Slicer crashed on exit, I fixed one crash point, and this is something interesting actually: the Slicer app was listening to the ModifiedEvent of one of its UI components in order to update itself; the trick here is that nothing prevents a class destructor to do something like this: this->SetFooBar(NULL);, which deallocates the FooBar string if it was defined using the very common vtkSetStringMacro and vtkGetStringMacro macros in its header. What this means is that you may be called back during an object destruction! Now as long as you carefully check that the object still exist (which is what I fixed, and that should be a good practice anyway), you should be OK. I would recommend listening to more specific events though, and/or ask me to add them for you in KWWidgets.
• Slicer3.exe is still crashing in ~vtkSlicerApplicationGUI because this->KWApplication is probably deleted multiple times. The whole KWApplication attribute that is very confusing in the current design, since it is found in different classes *and* duplicated: remember that any vtkKWObject *already* have a pointer to a vtkKWApplication object, i.e. you are either duplicating it, or using two at the same time, and if not synced properly, bad things will happen :) In our case here, there has probably something to do with ref counting, etc. I would *strongly* recommend you use the one already in KWWidgets, that can be set using vtkObject::SetApplication, and which is ref-counted properly. I can help you with that.

=> OK, I have not much time to fix more crashes, but here are few more things I noticed:

• Comments. Comments. Comments. My personal opinion is that it is really important that the code is commented *as soon as possible*. Right now a large number of methods are undocumented, which makes it really tricky to follow (I think somebody should be able to get a good grasp of the architecture from the header files, which I was unable to do at the moment). I would definitely recommend grouping and commenting methods together to avoid blocks of unrelated and undocumented methods like:

   // GUI FUNCTIONS:
   virtual int BuildGUI ( vtkSlicerApplicationGUI *app ) { return 1; }
   virtual void SetWindow ( vtkKWWindowBase *win );
   vtkKWWindowBase *GetWindow ( );
   virtual void SetSlicerApplication ( vtkSlicerApplicationGUI *app );
   vtkSlicerApplicationGUI *GetSlicerApplication ( );
   virtual void SetKWApplication ( vtkKWApplication *app ) ;
   vtkKWApplication *GetKWApplication ( );

Also remember that grouping is really important if you are planning to create documentation automatically using Doxygen for example.

• Style. Style. Style. It is probably equally important to adopt a consistent coding style. I'll let you guys decide which one, but since most classes inherit from VTK, I would strongly recommend using the VTK style, i.e. 2 spaces indentation, newline before curly braces, avoid one liners (dangerous), etc. I.e. this code:

   if ( this->LogicCommand )
       this->LogicCommand->Delete ( );

is in VTK:

 if (this->LogicCommand) 
   {
   this->LogicCommand->Delete();
   }

We have emacs code to do that automatically for you, if you are interested.

Comment and style seems to be OK in Libs/MRML and Libs/vtkITK