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. I'm sure you guys are aware of them, but I'm just trying to give some constructive feedback regarding what the source tree:

• 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.

• Forward declaration. It's actually a very good practice to include as few as possible dependent headers in a class declaration. I.e., if you do not explicitly refer to the vtkCollection API in your class header, you want to use

  class vtkCollection;

instead of

  #include "vtkCollection.h"

A counterpart to this is to try to avoid inlining in the header file as much as possible, unless it's only one line of code. Inlining in declaration files makes them so much longer and distract from the API, i.e. you should only be exposed to the public interface, not know what is going on under the hood. Inlining also creates a lot of dependences. For example, in vtkSlicerApplicationLogic.h, you probably want to avoid:

 void Connect (const char *URL) {
   if (this->MRMLScene)
     {
     this->MRMLScene->SetURL(URL);
     this->MRMLScene->Connect();
     }
 };

Now this is not just fancy :) We actually cleaned up VTK 4.0 and remove all extraneous #include that way, and this sped up the compilation *tremendously* as well as avoided a lot of dependencies checking. As you probably know, moving the above lines to the definition file (.cxx) will not harm your performance at all here (and virtual functions are used so much in VTK that inlining is actually not used very often).

• I would recommend using STL instead of the (aging) vtkCollection for all your internal container needs :)