From IGSTK

Classes Scheduled for Code Reviews

These classes are ready for immediate review.

Please fix the code when you do the review

Please use the Code Review Check List

Code Review Check List

Reviews

FLTKWidget

Matt's Comments

• This class currently has relatively low code coverage (~57%). Dashboard

Much of the uncovered code deals with mouse or keyboard events. However, adding calls to GetRenderWindowInteractor, AddObserver, RequestEnableInteractions, RequestDisableInteractions would significantly improve coverage. (will fix)

• The brief class description needs a period. (will fix) Andinet 11:58, 25 January 2008 (EST) Confirmed

• This class could use a more detailed class description for doxygen. Andinet 11:58, 25 January 2008 (EST) Description added

• May want to declare & define (?) a copy construction and assignment operator.

• Do we need the public method GetRenderWindowInteractor()? I haven't found where the method is used and its code coverage is 0. It also exposes a lot of the vtk interaction API to the developer. Andinet Per request from Torleif, it is kept for now. We might need to revisit it later

• Can the PickerType typedef be moved to protected/private? It is not used in any of the public functions. Andinet 08:51, 31 January 2008 (EST) Picker is moved to the View class

• GetRenderWindowInteractor() could be made const. (will fix) Andinet 08:51, 31 January 2008 (EST) Confirmed

• The class is not in igstkStateMachineExportTest.cxx and the documentation does not contain a diagram of the state machine. However, currently igstkStateMachineExportTest has no UI dependencies. Do we want to introduce the UI dependency? Andinet 08:51, 31 January 2008 (EST) Added

• Class will need a #include "vtkRenderWindow.h" in the .cxx file if forward instantiation is used elsewhere. Andinet 11:46, 25 January 2008 (EST) fixed

Patrick's Comments

• It will be nice to have a void RemoveObserver(unsigned long tag); Andinet 12:12, 25 January 2008 (EST) Since AddObserver method is removed from the widgets, this method will not be necessary

• There is no corresponding states for Request Enabling/Disabling interactions Andinet 11:52, 25 January 2008 (EST) As discussed in the tcon, the boolean variable m_InteractionHandling indicates the state whether the widget is enabled or disabled.

• There are some over length debug message in .cxx file Andinet 09:08, 31 January 2008 (EST) Fixed

• Feel like Picker should be moved into View Picker is moved to the view

• unnecessary include files, minor typos, and indentation (Fixed) Andinet 11:56, 25 January 2008 (EST) Confirmed

QTWidget

Matt's Comments

• I can't find any code coverage results for this class. Added Andinet 13:43, 4 February 2008 (EST)
• #include "vtkInteractorStyle.h" can be replaced by a forward declaration of class vtkRenderWindowInteractor. Andinet Fixed
• #include "vtkRenderer.h" can be replaced by a forward declaration of class vtkRenderer. Andinet Fixed
• Brief description needs a period. Andinet Fixed
• Class needs a full description. Andinet Fixed
• May want to declare and define (?) a copy constructor and assignment operator.
• Do we need the public method GetRenderWindowInteractor()? (see igstkFLTKWidget) Andinet 08:55, 31 January 2008 (EST) Kept for now
• Can the PickerType typedef be moved to protected/private? (see igstkFLTKWidget) Andinet 08:55, 31 January 2008 (EST) Moved to the view class
• GetRenderWindowInteractor() could be made const. Andinet 08:55, 31 January 2008 (EST) Fixed
• The class is not in igstkStateMachineExportTest.cxx (see igstkFLTKWidget) Andinet 08:55, 31 January 2008 (EST) Added
• Lines 234-244 in the mousePressEvent method contain statements wider than 78 characters. Andinet 08:55, 31 January 2008 (EST) Fixed
• Lines 415-423 in the wheelEvent method contain statements wider than 78 characters. Andinet 08:55, 31 January 2008 (EST) Fixed
• Line 439 (the comment to ReportInvalidRequestProcessing) is wider than 78 characters. Andinet 08:55, 31 January 2008 (EST) Fixed

SINTEF (JB, TS, FL):

• igstkQTWidget.h
  • \brief lacks a period at the end Andinet 12:00, 25 January 2008 (EST) Fixed
  • No complete description Andinet 12:00, 25 January 2008 (EST) Fixed
    • Private copy constructor and assignment operator
    • \image is missing
  • Forward declaration Andinet 12:00, 25 January 2008 (EST) Fixed
    • "vtkRenderer.h"
    • "vtkWorldPointPicker.h"
    • "itkCommand.h"
    • "itkLogger.h"
  • Code lines longer than 78.
    • 76
  • Public?
    • Why is void Print( std::ostream& os, ::itk::Indent indent=0) const; public? All it does is call a protected function, PrintSelf().
  • Code coverage not available.
  • Not added to igstkStateMachineExportTest.
  • Code does not follow recommandation from IGstk Style Guide: "Use spaces around arguments of functions and around operators. For example, instead of function(sum,operator,output); write function( sum, operator, output );"
• igstkQTWidget.cpp
  • Missing transitions: need three more, (2 states * 4 input tokens = 8 transitions).
  • Code lines longer than 78.
    • 181, 190, 198, 205, 230, 233, 234, 236, 414, 415, 432
  • Missing "this->". (IGstk Style Guide: "When refereeing to a member variable, an explicit this-> pointer should be used. Derived classes must us the combination of this-> and Get/Set methods.")
    • 60, 61, 63, 65, 66, 91, 100, 102, 133, 137, 152, 164, 166, 199, 206, 220, 273, 296, 298, 300, 316, 345, 366(x2), 369, 386, 401
  • Local variabel:
    • 267: vtkRenderWindowInteractor* iren = NULL; The name iren should be a more explanatory name. (IGstk Style Guide: "Local variables begin in lowercase. There is more flexibility in the naming of local variables. Please remember that others will study, maintain, fix, and extend your code. Explanatory variable names and comments will go a long way towards helping other developers. ariable names are considered to be part of the documentation of the code. Avoid short names that do not describe the role of the variable.") Andinet fixed
  • 306 & 378: "//Should be obtained from picked object."

Additional comments:

• QTWidget.h
  • Line 30: #include vtkInteractorStyle.h is not necessary Andinet 11:30, 25 January 2008 (EST) Fixed
  • Line 31: vtkRenderer can be forward/declared Andinet 11:30, 25 January 2008 (EST) Fixed
  • Line 32: vtkWorldPointPicker.h can be forward declared. Andinet 11:30, 25 January 2008 (EST) Fixed
  • Brief class comment lacks punctuation. Andinet 11:30, 25 January 2008 (EST) Fixed
  • Complete class doxygen description is lacking. Andinet 11:30, 25 January 2008 (EST) Fixed
  • #includes from other libraries should use <> instead of ""
  • Is the typedef View ViewType at line 58 unnecessary?
  • Why typedef vtkWorldPointPicker to PickerType at line 60?
  • Copy Constructor/assignment operator
  • Comments are not complete: E.g. AddObserver(): Which events are available? Andinet 12:16, 25 January 2008 (EST) the user specifies the events

• QTWidget.cxx:
  • #includes from other libraries should use <> instead of ""
  • 22:Unnecessary #include: vtkVersion is not used anywhere Andinet 11:36, 25 January 2008 (EST) Fixed
  • 24: Unnecessary #include vtkWindowToImageFilter, vtkPNGWriter not used. (screenshot is moved to View class) Andinet 11:36, 25 January 2008 (EST) Fixed
  • 26: Unnecessary #include vtkViewport unused Andinet 11:36, 25 January 2008 (EST) Fixed
  • 27: Unnecessary #include vtkREnderwindow not used anywhere Andinet 11:36, 25 January 2008 (EST) Fixed
  • constructor: Why not use initializer list for all initialization?

• • Line 107:
    • private method SetRenderer() not used anywhere in the class. Could add a comment saying that ViewProxy uses this method to connect view Andinet 08:57, 31 January 2008 (EST) Comment added
    • No validity checking for parameter renderer.

• • Line 114: See comments for line 107

• • mouseReleaseEvent(), mouseMoveEvent:
    • These functions invokes igstk events for some events but not all.
    • GUI events from widgets should be handled consistently. Duplicate code for handling mouse event transforms should be factored out into a function.

• • ReportInvalid*:
    • Events should be sent to error observers in these cases, otherwise an app developer cannot act upon these error conditions.

View

Matt's Comments

• Brief description needs a period. (I will fix) Andinet 10:41, 25 January 2008 (EST) Confirmed
• vtk includes in the header may be replaced with (I will fix) Andinet 10:41, 25 January 2008 (EST) Confirmed

class vtkRenderWindow;

class vtkCamera;

class vtkProp;

class vtkInteractorStyle;

class vtkRenderer;

• Using the foward declarations requires the following includes in the .cxx (I will fix) Andinet 10:41, 25 January 2008 (EST) Confirmed

#include "vtkRenderWindow.h"

#include "vtkCamera.h"

#include "vtkInteractorStyle.h"

• View needs a private copy constructor and assignment operator with no implementations. (I will fix) Andinet 10:44, 25 January 2008 (EST) Confirmed

• Part of the API seems inconsistent. An application developer needs to request that the camera be reset, but on the other hand, methods like SetCameraPosition, and SetFocalPoint, for instance, are publicly available. ResetCamera is a convenient method provided by vtkRenderer to automatically position the camera based on the visible actors. But if the user chooses to set the camera parameters manually, having public methods to modify the camera parameters is handy.

• m_Camera is protected, not private. Andinet 10:56, 25 January 2008 (EST) Fixed

• typedef ObjectListType::iterator ObjectListIterator is not necessary. (I will fix) Andinet 10:44, 25 January 2008 (EST) Confirmed

• The following typedefs can be moved from public to private: (I will fix) Andinet 10:44, 25 January 2008 (EST) Confirmed

typedef ObjectRepresentation::Pointer ObjectPointer;

typedef std::list< ObjectPointer > ObjectListType;

typedef ObjectListType::const_iterator ObjectListConstIterator;

• Code coverage is 89%+
• GetRenderer, GetRenderWindow, GetRenderWindowInteractor, GetReporter can be made const. (I will fix) Andinet 10:44, 25 January 2008 (EST) Confirmed

• Various lines are wider than 78 characters. (I will fix) Andinet 10:41, 25 January 2008 (EST) Confirmed
• Needs \image in doxygen doc. Andinet 10:41, 25 January 2008 (EST) Fixed

Luis' Comments

• #include igstkEvents.h redundant in cxx.
  • was already included in the header file (fixed)
• Method RequestAddAnnotation2D ( Annotation2D::Pointer annotation )
  • should be RequestAddAnnotation2D ( Annotation2D * annotation )
• Code Coverage
  • igstkView.cxx 89.04% 48 lines never tested
  • igstkView.h 100.00% great !
• PrintSelf() method is not printing all the member variables.
• igstkView is missing from the StateMachineExport file. (fixed)
  • How is the file /IGSTK/Testing/igstkStateMachineExportTest.cxx compiling ??!!
    • Answer: the file was asking #if IGSTK_USE_FLTK but was not including igstkSandboxConfigure.h
  • The API of the View2D and View3D classes there is deprecated...
  • igstk::View2D view2D(0,0, 100, 100, "dummy view for testing");
  • igstk::View3D view3D(0,0, 100, 100, "dummy view for testing");
• One minor coding style issue, in header comment (fixed).

Transform

Ziv's Comments

Logical bug in the IsNumericallyEquivalent method.

To test equivalence of two transforms T1=[r1,t1], T2=[r2,t2] the invocation r1.IsNumericallyEquivalent(r2) is a combination of two logical approaches:

a. Literally checking that the two transforms are numerically equivalent (read the ==, as subtraction and comparison of absolute value to eps) r1==r2 && t1==t2

b. How far is T1*T2^{-1} from the identity transformation: T1*T2{-1} = [r1,t1]* [r2^{-1}, -r2^{-1}t2] = [r1*r2^{-1}, -r1*r2^{-1}(t2) + t1]

The code currently checks that r1*r2^{-1} is close to the identity transformation (part 1 of option b) and then checks that t1-t2 is close to the identity (part 2 of option a).

The combination of both approaches is a logical bug. Either option a or b but not their combination. I prefer option b but that is a personal preference.

Ibanez 11:56, 5 February 2008 (EST) (Fixed) The IsNumericallyEquivalent method now is defined as:

   T = Compose( ThisTransform, ThisTransform.GetInverse() )
   T.IsIdentity()

and the new IsIdentity() method is defined as

• False if
  • The cosinus of the half angle is smaller than (1.0 - tolerance)
  • The norm of the translation vector is larger than (tolearance)
• True otherwise

Patrick's Comments

• No need to touch this class

Tracker

Ziv's Comments

• State machine is incomplete. 10 states X 10 input tokens = 100 transitions. There are about 40 transitions.

• Possible error not checked in constructor, following invocation may fail

m_PulseGenerator->RequestSetFrequency( DEFAULT_REFRESH_RATE );

• The method RequestSetReferenceTool is not part of the state machine. It shouldn't do any work, only push input to the state machine. An additional state 'AttemptingToSetReferenceTool' is required. Right now if the tool is not already attached to the tracker an error message is printed to cerr.

• Virtual methods with implementation stubs for subclasses, instead of pure virtual methods. Andinet 17:39, 26 January 2008 (EST) All the virtual methods with just implementation stubs have been converted to be pure virtual methods. The tracker class is now an abstract class.

Luis' Commments

• Static variables not needed. They can be local consts. (fixed). Andinet 10:13, 30 January 2008 (EST) Confirmed
  • DEFAULT_REFRESH_RATE
  • DEFAULT_VALIDITY_TIME
• Operator= and Copy constructor were missing from the private section (fixed) Andinet 10:13, 30 January 2008 (EST) Confirmed
  • They must be declared and not implemented, since the Tracker uses SmartPointers.
• Unnecessary Includes removed Andinet 10:13, 30 January 2008 (EST) Confirmed
  • Versor
  • AxesObject
• Obsolete typedef removed Andinet 10:13, 30 January 2008 (EST) Confirmed
  • typedef AxesObject CoordinateReferenceSystemType;
• Forward declaration of TrackerTool is not needed because #include TrackerTool is there anyways. (fixed: removed) Andinet 10:13, 30 January 2008 (EST) Confirmed
• When inside the igstk namespace, there is no need to use igstk:: to refer to other classes Andinet 10:13, 30 January 2008 (EST) Confirmed
  • typedef igstk::TrackerTool TrackerToolType; (fixed)
• Why do we need a CalibrationTransformType when we already have a TransformType ?
  • and both are defined as "Transform" (fixed : removed ) Andinet 10:13, 30 January 2008 (EST) Confirmed
• ResultType doesn't need to be public.
  • It is only used by protected methods. (fixed) Andinet 10:13, 30 January 2008 (EST) Confirmed
• ErrorType is never used (fixed : typedef removed) Andinet 10:13, 30 January 2008 (EST) Confirmed
• TransformType shouldn't be public. Andinet 10:13, 30 January 2008 (EST) Moved to the protected section
  • It is only used in one protected method: SetTrackerToolRawTransform() (fixed: moved to protected section) Andinet 10:13, 30 January 2008 (EST) Confirmed
• TrackerToolPointer shouldn't be public Andinet 10:13, 30 January 2008 (EST) Confirmed
  • It is only used in private member variable (fixed: typedef moved to private section)
• TrackerToolConstPointer type is never used (fixed: removed) Andinet 10:13, 30 January 2008 (EST) Confirmed
• TrackerToolsContainerType shouldn't be public
  • It is only used in private member variable, and in protected method. (fixed: moved to protected section) Andinet 10:13, 30 January 2008 (EST) Confirmed
• Code Coverage insufficient
  • igstkTracker.cxx 91.79% 34 lines never tested
  • igstkTracker.h 90.91%2 2 lines never tested
• ValidityTime variable shouldn't be publicly accessible (Set/Get)
  • It should be set internally by derived classes, according to hardware capabilities. (fixed/moved to protected section) Andinet 10:13, 30 January 2008 (EST) Confirmed
  • This also moves the typedef for TimePeriodType to the protected section. (fixed). Andinet 10:13, 30 January 2008 (EST) Confirmed
• Method VerifyTrackerToolInformation() should be "const".
• Method GetTrackerToolContainer() should return a const & to the container, not a copy of the std::map<> !! (critical)(fixed)
• Method ReportTrackingToolNotAvailable() should be const Andinet 10:13, 30 January 2008 (EST) Confirmed
• Method ReportTrackingToolVisible() Andinet 10:13, 30 January 2008 (EST) Confirmed
  • Signature in the header does not match the one in the .cxx file (surprisingly compilers do not complain??)
  • The method must be "const", since it doesn't change the Tracker.
• Method SetTrackerToolRawTransform() has poor signature:
  • void SetTrackerToolRawTransform( TrackerToolType * trackerTool, TransformType transform );
  • It should be:
  • void SetTrackerToolRawTransform( TrackerToolType * trackerTool, const TransformType & transform ) const; Andinet 11:03, 30 January 2008 (EST) Fixed
• Method SetTrackerToolTransformUpdate() should be const.
• PrintSelf() is not printing out all member variables.
• Several coding style issues (fixed)
  • Make sure that no new ones appear when fixing the code.

TrackerTool

Ziv's Comments

• State machine is incomplete. 8 states X 13 input tokens = 104 transitions. There are about 16 transitions.

• Virtual methods with implementation stubs for subclasses, instead of pure virtual methods. Andinet 17:38, 26 January 2008 (EST) There was one virtual method with just implementation stubs and it is converted to be pure virtual methods. The trackertool class is now an abstract class.

Luis' Comments

• Destructor must be virtual (fixed). Andinet 19:48, 29 January 2008 (EST) Confirmed
• Copy constructor and operator= declarations were missing from the private section Andinet 19:48, 29 January 2008 (EST) Confirmed
  • These methods must be declared and not implemented since the TrackerTool class uses SmartPointers.
• Why is the type "CalibrationTransformType" needed ?
  • It is just a TransformType. (fixed removed).
• Operator<<( ostream ) is defined for some classes and not for others...
  • Maybe we should add it as a standard method and built it inside the Standard Traits Macros...
  • Its implementation is always the same... just call Print()
• AxesObject used as coordinate reference system is obsolete (fixed : removed)
  • typedef AxesObject CoordinateReferenceSystemType; (removed) Andinet 19:48, 29 January 2008 (EST) Confirmed
• typedef double ErrorType; (not used, fixed: removed) Andinet 19:48, 29 January 2008 (EST) Confirmed
• typedef double TimePeriodType; (not needed in the public API) Andinet 19:48, 29 January 2008 (EST) Confirmed
  • Only used once inside the constructor
  • Moved there... (fixed). Andinet 19:48, 29 January 2008 (EST) Confirmed
• Bad method signature:
  • std::string GetTrackerToolIdentifier( );
  • It should be
  • const std::string & GetTrackerToolIdentifier() const; Andinet 19:48, 29 January 2008 (EST) Fixed
• The following public methods must be removed (CRITICAL)
  • igstkGetMacro( CalibratedTransformWithRespectToReferenceTrackerTool, TransformType );
  • igstkGetMacro( CalibrationTransform, TransformType );
  • void SetCalibrationTransform( const TransformType & );
  • igstkGetMacro( RawTransform, TransformType );
  • igstkGetMacro( CalibratedTransform, TransformType );
  • igstkGetMacro( Updated, bool );
  • Only a RequestSetCallibrationTransform() method should exist and should be regulated by the StateMachine.
  • The functionalities of all the other methods:
    • Are provided by RequestTransfromTo() of the Coordinate System classes.
    • Are returned as Events with a payload including a Transform and two coordinate systems.
    • Updated flag is not needed.
      • Classes will know when the TrackerTool is "updated" by attaching a Command/Observer to it.
• Method "RequestDetach()"
  • should be called "RequestDetachFromTracker" in order to be symmetric to "RequestAttachToTracker()". Andinet 19:48, 29 January 2008 (EST) Fixed
• SetTrackerToolIdentifier() has poor signature
  • void SetTrackerToolIdentifier( std::string identifier );
  • It should be
  • void SetTrackerToolIdentifier( const std::string & identifier ); Andinet 19:48, 29 January 2008 (EST) Fixed
• Method CheckIfTrackerToolIsConfigured() should be const. Andinet 19:48, 29 January 2008 (EST) Fixed
• Method igstkSetMacro( Updated, bool ); should be removed: Update flag is unnecessary.
• The following Member variables should be removed
  • m_CalibratedTransformWithRespectToReferenceTrackerTool
  • m_CalibratedTransform
  • m_Updated
  • They are redundant variables. We should only need the m_CallibrationTransform and the m_RawTransform
• Method AttemptToDetachTrackerToolFromTrackerProcessing()
  • Assumes that m_TrackerToAttachTo still contains the pointer of the Tracker that this tool is attached to.
  • That's a misuse of the temp variable that is only expected to be used during state machine transitions.
  • We should add a m_Tracker variable that is the official pointer to the tracker.
  • Method AttemptToAttachTrackerToolToTrackerProcessing() should do
    • m_Tracker = m_TrackerToAttachTo;
• Code Coverage Insufficient
  • TrackerTool.cxx 84.74% 29 lines never tested
  • TrackerTool.h 85.00% 3 lines never tested
• The following methods should be "const"
  • ReportTrackerToolVisibleStateProcessing()
  • ReportTrackerToolNotAvailableProcessing()
  • ReportTrackingStartedProcessing()
  • ReportTrackingStoppedProcessing()
• The following methods should be renamed in order to have "Request" since they are insert inputs in the StateMachine. (they must remain private) Andinet 19:48, 29 January 2008 (EST) Fixed
  • ReportTrackingStarted()
  • ReportTrackingStopped()
  • ReportTrackingToolNotAvailable()
  • ReportTrackingToolVisible()
  • ReportSuccessfulTrackerToolAttachment()
  • ReportFailedTrackerToolAttachment()
• PrintSelf() method is mnissing member variables
  • In particular: m_TrackerToolIdentifier;
• RequestAttachToTracker() must be protected to enforce type safety. (fixed)
  • In particular to prevent a TrackerTool of one type to be attached to a Tracker of incompatible type (e.g. attach a MicronTrackerTool to a PolarisTracker).

PolarisTracker

Ziv's Comments

• VerifyTrackerToolInformation implementation:

Missing test for dynamic_cast at the beginning of the function. Andinet 08:33, 28 January 2008 (EST) Fixed

Replace all of the following literals (1024, 64, 129, 256, 512, 9, 21) with constants that have meaningful names.

Remove all the messages printed to std::cout and std::cerr. In some cases these come before a return FAILURE/SUCCESS. In some cases no value is returned, just printing a message. Andinet 08:33, 28 January 2008 (EST) Fixed

• InternalThreadedUpdateStatus implementation:

Replace literal (8) with a constant, NDI_DATA_RECORD_SIZE?

• SUSPECTED CRITICAL LOGIC FLAW: The tracker allows loading of the same SROM file to different tools. I haven't found file name checking and even that will not help as the file content is the important issue and not its name.

Luis' Comments

• The following typedefs are not needed/used in the public API (fixed) Andinet 09:35, 11 February 2008 (EST) Confirmed
  • typedef igstk::PolarisTrackerTool PolarisTrackerToolType;
  • typedef PolarisTrackerToolType::Pointer PolarisTrackerToolPointer;
  • typedef PolarisTrackerToolType::ConstPointer PolarisTrackerToolConstPointer;
  • PolarisTrackerToolType is only needed internally for performing dynamic_casting.
• Private typedefs are needed for the std::map member variables (fixed). Andinet 09:35, 11 February 2008 (EST) Confirmed
  • m_PortHandleContainer
  • m_ToolAbsentStatusContainer
  • m_ToolStatusContainer
  • TrackerToolTransformContainerType
• CheckError() method should be const.(fixed)
• PrintSelf() method is not printing out any member variable
• Coding Style issues (fixed)
  • Please check that no new issues appear when adding the fixes.

PolarisTrackerTool

Ziv's Comments

• State machine is incomplete.

• RequestSetPortNumber(unsigned portNumber)

Declaration, Missing documentation in header. It is unclear if portNumber is the physical number of ports or a port handle. If this refers to a physical port then valid values are [1..12] or [0..11], if it is the port handle then [0..255].

Implementation, As this is an interface to the polaris tracker the user doesn't know about port handles, so I assume this is the physical port number. I would use [1..12] these are the numbers that appear on the control box. Right now the validity check is "if ( portNumber > 255 )": Andinet 09:27, 11 February 2008 (EST) Fixed

a. Use a constant and not a literal (255).

b. Change the validity check to

if(portNumber > MAXIMAL_PORT_NUMBER || portNumber < MINIMAL_PORT_NUMBER) if valid values are in [1..12] or to

if(portNumber > MAXIMAL_PORT_NUMBER) if valid values are in [0..11] (portNumber is unsigned int).

• ReportInValidPortNumberSpecifiedProcessing() Andinet 09:27, 11 February 2008 (EST) Fixed

This method doesn't invoke any event when a wrong port number is given, just writes to log file.

• RequestSetSROMFileName() implementation

Checking for the existence of the SROM file here is not useful. It can be erased from the hard drive between this point and the time the tracker actually tries to use it. I believe we can accept any file name here and the failure will happen when we try to load it (tracker does check for this). In any case this test doesn't ensure a correct file when it is needed.

Luis' Comments

• Destructor must be virtual (fixed) Andinet 14:11, 4 February 2008 (EST) Fixed
• Acronyms are used for SROM Andinet 14:11, 4 February 2008 (EST) Agree
  • we probably can tolerate this one.
• Methods that take std::string as arguments should use const std::string & Andinet 14:11, 4 February 2008 (EST) Confirmed
  • RequestSetSROMFileName()
  • RequestSetToolId()
• Method GetSROMFileName() Andinet 14:11, 4 February 2008 (EST) Confirmed
  • should be const
  • should return a const std::string &
  • can be replaced with an igstkGetStringMacro() (fixed)
• Method GetPortNumber() Andinet 14:11, 4 February 2008 (EST) Confirmed
  • should be const
  • can be replaced with igstkGetMacro() (fixed)
• Method IsToolWirelessType()
  • should be const (fixed)
• Method IsSROMFileNameSpecified() Andinet 14:11, 4 February 2008 (EST) Fixed
  • should be const (fixed)
• Code Coverage
  • igstkPolarisTrackerTool.cxx 75.94%  : 32 lines never tested.
  • igstkPolarisTrackerTool.h 33.33%  : 2 lines never tested.
• In method RequestSetToolId()
  • Contains a FIXME line: //FIXME DO ToolID verification
• PrintSelf() method is not printing any of the member variables. Andinet 14:11, 4 February 2008 (EST) Added
• Coding Style Isses
  • Several issues in the .h and .cxx files (fixed)
  • Please make sure that no new issues are introduced while making the code fixes.

AuroraTracker

Ziv's Comments

• All of the comments as given for the polaris tracker.

• Remove references to wireless tools (empty else) in VerifyTrackerToolInformation implementation. Andinet 08:56, 28 January 2008 (EST) Removed

Luis' Comments

• The following typedefs are not needed in the public API
  • typedef igstk::AuroraTrackerTool AuroraTrackerToolType; (moved to the .cxx)
  • typedef AuroraTrackerToolType::Pointer AuroraTrackerToolPointer; (removed)
  • typedef AuroraTrackerToolType::ConstPointer AuroraTrackerToolConstPointer; (removed)
• CommandInterpreterType is only used in the private section
  • moved from the protected section into the private (fixed)
• typedefs are missing for
  • std::map< std::string, int > m_PortHandleContainer;
  • std::map< std::string, int > m_ToolAbsentStatusContainer;
  • std::map< std::string, int > m_ToolStatusContainer;
  • They should be typedef so that their traits (iterators) can be defined consistently in the .cxx (fixed).
• Code Coverage
  • igstkAuroraTracker.h 100.00% Great !
  • igstkAuroraTracker.cxx 80.65% 48 lines never tested (not so great)
• Method VerifyTrackerToolInformation() should be const
• PrintSelf() method is missing all member variables
• Several coding style issues (fixed)
  • Please make sure that no new issues appear...

AuroraTrackerTool

Ziv's Comments

• All of the comments as given for the polaris tracker.

• RequestSetChannelNumber(unsigned int) Declaration, Missing documentation in header. What are valid values for channel number.

Luis' Comments

• RequestAttachToTracker( Tracker * ) made protected in the TrackerTool class
  • added here a RequestAttachToTracker( AuroraTracker * ) method to prevent an AuroraTrackerTool to be attached to trackers different from the AuroraTracker.
• Destructor must be virtual (fixed)
• Acronyms in Method Names
  • SROM
  • 5DOF
  • 6DOF
  • They are so domain specific that we could tolerate them... but.. we should at least expand them in the Documentation. (fixed)
• #includes for ITK classes should use "" instead of <> (fixed)
• Methods that take std::string as argument
  • should use const std::string & instead of just std::string (fixed)
• Why is RequestSetToolId() public ?
  • Shouldn't the tool id be taken from the SROM file ?
• method GetSROMFileName()
  • should have returned const std::string &
  • should have been const
  • can be fully replaced with igstkGetStringMacro() (fixed)
• method GetPortNumber()
  • should have been const
  • can be fully replaced with igstkGetMacro() (fixed)
• Method IsSROMFileNameSpecified()
  • should be const (fixed)
• Method IsTrackerTool5DOF()
  • should be const (fixed)
• The following methods could have been (const) but.. they are callbacks to the state machine (nothing to change).
  • ReportInValidPortNumberSpecifiedProcessing()
  • ReportInValidChannelNumberSpecifiedProcessing()
  • ReportInValidSROMFileSpecifiedProcessing()
  • ReportInValidToolIdSpecifiedProcessing()
  • Report5DOFTrackerToolSelectedProcessing()
  • Report6DOFTrackerToolSelectedProcessing()
• Code Coverage
  • AuroraTrackerTool.cxx 70% 48 lines never tested
  • AuroraTrackerTool.h 100% great !
• PrintSelf() method is not printing any of the member variables.
• RequestSetToolId() method has a FIXME:
  • //FIXME DO ToolID verification
• Coding style issues (fixed)
  • Please verify that no new issues appear while fixing the code.

CoordinateSystemInterfaceMacros

Patrick's comments

• Missing copyright (Fixed) Matt.Turek 11:20, 5 February 2008 (EST) Verified.

Luis' Comments

• In Macro igstkCoordinateSystemClassInterfaceMacro - Matt.Turek 17:57, 31 January 2008 (EST) Fixed
  • Method GetInternalTransform() should be const
• High level objects that require coordinate systems should only need to include one header. E.g. the CoordinateSystemInterfaceMacro.h (fixed) Matt.Turek 11:20, 5 February 2008 (EST) Verified.

CoordinateReferenceSystem

Patrick's comments

• Rename it to CoordinateSystem ?

Luis' Comments

• Doxygen documentation of the class was too short. Some more text has now been added. It requires another revision.
• CoordinateReferenceSystemHelper was missing Doxygen documentation. (now added). Matt.Turek 19:52, 31 January 2008 (EST) Confirmed
• Copy constructor and operator=() declarations in the private section Matt.Turek 19:52, 31 January 2008 (EST) Confirmed
  • were moved to the top of the section where we usually look for them. (style)
  • Note that they are purposely not implemented.
• When finished debugging and profiling, the TimeCollector header and member variables must be removed from this class. Matt.Turek 19:52, 31 January 2008 (EST) Fixed
• SetName()/ GetName() methods should have use the igstkSetStringMacro() and igstkGetStringMacro(). (Now replaced) Matt.Turek 19:52, 31 January 2008 (EST) Confirmed
  • These methods are indicated to be removed, but maybe we want to keep them around for generating grapviz representation of the scene graph. Matt.Turek 19:52, 31 January 2008 (EST) Will leave.
• void RequestDetach() comment ?? Is not implemented. Matt.Turek 13:58, 5 February 2008 (EST) Implemented.
  • It be implemented or we must delete the comment.
• Several methods are calling "ProcessInputs()" without being "Request" methods Matt.Turek 11:17, 5 February 2008 (EST) Methods are called as a result of Request methods. Will document
  • This may indicate a flaw in the State Machine wiring of this class.
  • The offending methods are:
    • FindLowestCommonAncestor()
    • ComputeTransformToValidTargetProcessing()
• PrintSelf() method is not printing out all member variables. Matt.Turek 19:52, 31 January 2008 (EST) Fixed
  • This should make the class failt the PrintSelf test.
• "std::cout"s must be removed or replaced with a method that takes std::ostream as an argument. Matt.Turek 19:52, 31 January 2008 (EST) Removed
  • Currently they are in the destructor, and they seem to be for debugging. This must be removed before moving the class to the main library.
• The class was in the StateMachineExport test, but its StateMachine diagram was not included in the Doxygen documentation (fixed). Matt.Turek 17:35, 1 February 2008 (EST) Confirmed.
• Code Coverage is insufficient:
  • .cxx 91.29%  : 21 lines never tested
  • .h 96.61  : 2 lines never tested
• Coding Style Violations
  • .h 3 problems : fixed
  • .cxx 1 problem : related to comments, and the use of /** */ inside code. (fixed)
    • The format of /** comments */ should only be used in the header file. Doxygen does not parse the implementation of methods. Matt.Turek 19:52, 31 January 2008 (EST) Fixed. (Wasn't expecting Doxygen to use cxx comments.)
• Private methods and member variables lack Doxygen documentation Matt.Turek 19:52, 31 January 2008 (EST) Fixed.
  • Is is not-critical, but it will facilitate code maintenance down the line.
• Member variables have "strange" names: (find a better name...) Matt.Turek 19:52, 31 January 2008 (EST) Changed names. Luis should comment on them.
  • Transform m_RequestSetTransformAndParentTransform;
  • Self::ConstPointer m_RequestSetTransformAndParentParent;
  • Self::ConstPointer m_ComputeTransformToTarget;
• Additional classes used as payload for events are better in their own files (fixed....) Matt.Turek 12:00, 5 February 2008 (EST) Confirmed. Moved implementations to cxx files. Added documentation.
  • Source/igstkCoordinateSystemTransformToErrorResult.cxx
  • Source/igstkCoordinateSystemTransformToErrorResult.h
  • Source/igstkCoordinateSystemTransformToResult.cxx
  • Source/igstkCoordinateSystemTransformToResult.h
  • BUT THE CODE IMPLEMENTING METHODS NEEDS TO BE MOVED TO THE .CXX FILES.
• In method RequestSetTransformAndParent() Matt.Turek 11:14, 5 February 2008 (EST) Verified.
  • Remove the need for using smartpointer->GetPointer() in the second argument. It breaks the style of the rest of IGSTK. (fixed).

CoordinateReferenceSystemDelegator

Patrick's comments

Luis' Comments

• .cxx files was missing copyright header (fixed). Matt.Turek 19:56, 31 January 2008 (EST) Confirmed.
• The word "stuff" shouldn't be used in the documentation. (fixed). Matt.Turek 19:56, 31 January 2008 (EST) Confirmed.
• The brief and complete Doxygen documentation were fused in a single paragraph (fixed). Matt.Turek 19:56, 31 January 2008 (EST) Confirmed.
• The doxygen group is missing.
  • Maybe we should create a "Coordinate System" group ?
• since igstkCoordinateReferenceSystem.h is #included Matt.Turek 19:56, 31 January 2008 (EST) Confirmed.
  • you can remove the #includes for igstkStateMachine.h, igstkMacros.h and igstkObject.h (fixed)
• Implementations of (non-templated) methods must be in the .cxx file, not the .h file. Matt.Turek 12:01, 1 February 2008 (EST) Fixed.
  • Move to the .cxx the implementation of method RequestGetTransformToParent()
• Insufficient Code Coverage
  • .cxx 81.82% : 6 lines are never tested.
  • .h 68.42% : 6 lines are never tested.
• PrintSelf() method missing. Matt.Turek 12:27, 1 February 2008 (EST) Fixed.
• Missing from the StateMachineExport Test file Matt.Turek 13:35, 1 February 2008 (EST) Fixed.
• \image of the StateMachine diagrams is missing from the Doxygen header in the .h file. Matt.Turek 13:35, 1 February 2008 (EST) Fixed.
• A couple of KWStyle coding style violations: (fixed). Matt.Turek 13:39, 1 February 2008 (EST) Style looks clean.
• In method RequestComputeTransformTo() Matt.Turek 19:56, 31 January 2008 (EST) Confirmed.
  • Modify the argument so that it doesn't need to use GetPointer() (fixed)

MicronTracker

SINTEF (JB, TS, FL)

• igstkMicronTracker.h
  • Can be forward declared: Andinet 09:20, 27 January 2008 (EST) Fixed
    • "igstkMicronTrackerTool.h"
    • "igstkTracker.h"
    • "Markers.h"
    • "Marker.h"
    • "Persistence.h"
    • "Cameras.h"
    • "Facet.h"
    • "Xform3D.h"
    • "MTC.h"
    • <math.h>
  • Complete doxygen description is too short.
  • Code coverage not available.
  • Missing (private) copy and assignment constructors. Andinet 09:20, 27 January 2008 (EST) Added
  • 120, 133 & 136: Missing "void" in parameter list.
  • Const?
    • 123: static const std::string GetErrorDescription( unsigned int );, this one should be const. Andinet 09:20, 27 January 2008 (EST) Not allowed and not needed
  • Remove?:
    • 56: /** The should be converted to the new Tool class */
  • Layout fault:
    • 42: { (IGstk Style Guide: "Braces are placed on a line by themselves:")
  • 71 & 77: "FIXME: this should be pushed through the state machine"
  • Code does not follow recommandation from IGstk Style Guide: "Use spaces around arguments of functions and around operators. For example, instead of function(sum,operator,output); write function( sum, operator, output );"
  • Some comments are not terminated with a period.
  • 150 & 170: Starts with a small letter.

• igstkMicronTracker.cpp
  • Replace literals (0) and (55) with constants.
  • Missing "this->". (IGstk Style Guide: "When refereeing to a member variable, an explicit this-> pointer should be used. Derived classes must us the combination of this-> and Get/Set methods.")
    • 39, 46, 49, 52, 55, 58, 61, 66, 69, 76, 78, 81, 83, 86, 88, 95-150, 157, 180, 202, 243, 249, 260, 319, 320, 344, 380, 381, 394, 442, 446, 447, 456, 503, 519, 522, 525, 530, 545, 595, 597, 599, 600, 618
  • Code lines with more than 78 chars (the "Code Review Check Lists" says 78, but the "IGstk Style Guide" says 79).
    • 95-150 (not all, but many of them), 168, 177, 197, 208, 243, 245, 319, 322, 326, 356, 393, 402, 413, 433, 449, 461, 496, 497, 503, 534, 540, 549, 556, 595, 627
  • Bug:
    • 365: Is counting on to short, should be: for (unsigned int idx = 1 ; idx < totalNumberOfTemplates + 1 ; idx++ ). This is also commented on this wiki page.
  • 487: // Get error value from the tracker. TODO

Additional comments:

igstkMicronTracker.h

• • Line 57: Why use igstk prefix iside igstk namespace? Why typedef?

• igstkMicronTracker.cxx
  • Constructor
    • Use initialization list to initialize class members
  • Destructor:
    • C++ guarantees that delete checks for NULL before deleting. Unnecessary if.
  • SetCameraCalibrationFilesDirectory() and SetInitializationFile() and LoadMarkerTemplate()
    • Check if file exists before setting member variable and not later. Fail early.
  • Line 195: ise m_MarkerTemplateDirectory.empty() instead of checking against empty string
  • Line 196: App developer cannot act on this error! Invoke event.
  • Line 213: Consistency: Do or Don`t use void in parameter lists. (Several places) Andinet 09:40, 27 January 2008 (EST) fixed
  • Line 363: Should check for NULL after dynamic_cast for trackerTool Andinet 09:40, 27 January 2008 (EST) fixed
  • Line 380: Use at() instead to get boundschecking from vector
  • Line 469: Defer instantiation.

Luis' Comments

• Missing "\ingroup Tracker" in the Doxygen documentation (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• Misaligned "*" in the top \class comment (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• Added explanation of where to find the MicronTracker utility classes. (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• Misaligned "*" in the documentation comments of several methods (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• Methods that take std::string as argument should use const std::string &
  • SetCameraCalibrationFilesDirectory()
  • SetInitializationFile()
  • LoadMarkerTemplate()
  • (fixed all three) Andinet 18:08, 29 January 2008 (EST) Confirmed
• ResultType typedef is in the public API
  • but is only needed in the protected section
  • moved there (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• Unnecessary typedefs in the public API
  • typedef igstk::MicronTrackerTool MicronTrackerToolType;
  • typedef MicronTrackerToolType::Pointer MicronTrackerToolPointer;
  • typedef MicronTrackerToolType::ConstPointer MicronTrackerToolConstPointer;
  • Only the first one is needed, and it is used in a single place in the .cxx file. The declaration was moved there. The other typedefs were removed. (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• Method with bad signature
  • const std::string MicronTracker::GetErrorDescription( unsigned int code )
  • Returning std::string by copy as a const doesn't add to the API. It should be returned by copy or by const reference. In this case, since the strings are internal, copy is the best option. (fixed). Andinet 18:08, 29 January 2008 (EST) Confirmed
• #include for MicronTrackerTool.h is not needed in the header file.
  • moved to the .cxx file (fixed) Andinet 18:08, 29 January 2008 (EST) Confirmed
• The following methods should have use the igstkSetStringMacro() Andinet 18:08, 29 January 2008 (EST) Fixed
  • SetCameraCalibrationFilesDirectory()
  • SetInitializationFile()
  • and made the variable name consistent with the method name (currently CalibrationFilesDirectory and CameraCalibrationFilesDirectory do not match).
• Code Coverage
  • igstkMicronTracker.h 0%  !!!
  • igstkMicronTracker.cxx 0%  !!!
  • We must find a way of providing code coverage for it. Isn't it possible to connect the MicronTracker to the Linux machine that runs the coverage build ?
• Method RemoveTrackerToolFromInternalDataContainers() Andinet 18:20, 29 January 2008 (EST) Fixed
  • Should take a "const" TrackerToolType pointer
  • Since the argument is not modified inside.
• PrintSelf() method is printing out only one of the member variables.
  • There are 6 to 8 other member variables that should be printed out.
• Many coding style violations
  • 56 in the .cxx
  • 3 in the .h
  • Please make sure that no new issues are introduced while fixing the code.
• Local variables:
  • defaultLightCoolness
  • defaultFrameInterleave
  • defaultTempMatchToleranceMM
  • defaultSmallerXPFootprint
  • SmallerXPFootprint
  • defaultExtrapolatedFrames
  • ExtrapolatedFrames
  • Should be "const". They were introduced to clarify literals. (fixed). Andinet 18:09, 29 January 2008 (EST) Confirmed

MicronTrackerTool

SINTEF (JB, TS, FL)

• igstkMicronTrackerTool.h
  • Forward declaration:
    • "Markers.h" Andinet 10:18, 27 January 2008 (EST) fixed
• igstkMicronTrackerTool.cpp
  • Missing "this->". (IGstk Style Guide: "When refereeing to a member variable, an explicit this-> pointer should be used. Derived classes must us the combination of this-> and Get/Set methods.")
    • 34, 46, 49, 57
  • Code lines longer than 78 chars.
    • 52, 56, 62
  • Code coverage not available
  • Code does not follow recommandation from IGstk Style Guide: "Use spaces around arguments of functions and around operators. For example, instead of function(sum,operator,output); write function( sum, operator, output );"

• Line 43: unnecessary comment

Ziv's Comments

• For consistency, the method RequestSetMarkerName() should check that the marker file exists. This is then similar to what is done for the SROM files in the PolarisTool. This requires that the given string be the fully qualified name (including directory path). Once that is checked, the path is stripped and only the file name is retained for later comparison when attaching to tracker. Andinet 16:55, 30 January 2008 (EST) marker name is not a filename. But I have still added a check whether the name specified is not an empty string.

Luis' Comments

• Forward declaration of "Markers" is unnecessary. (fixed, removed) Andinet 16:29, 30 January 2008 (EST) Fixed
• igstkGetMacro() for string should use igstkGetStringMacro() (fixed) Andinet 16:29, 30 January 2008 (EST) Fixed
• Misaligned documentation for RequestSetMarkerName() (fixed) Andinet 16:29, 30 January 2008 (EST) Fixed
• Destructor was not marked as virtual (fixed) Andinet 16:29, 30 January 2008 (EST) Fixed
  • This is mostly for documentation, since the declaration that matters is the one in the base class.
• Method CheckIfTrackerToolIsConfigured() should be const (fixed) Andinet 16:29, 30 January 2008 (EST) Fixed
• #include for Markers.h is not necessary. (fixed | removed ) Andinet 16:29, 30 January 2008 (EST) Fixed
• Code Coverage
  • igstkMicronTrackerTool.h 0% !!
  • igstkMicronTrackerTool.cxx 0% !!
  • We must find a way of computing code coverage for this class. (urgent)
• PrintSelf() method is not printing out the member variables: Andinet 16:29, 30 January 2008 (EST) Added
  • m_MarkerName
  • m_TrackerToolConfigured
• A couple of coding style errors (fixed)
  • Please make sure that no new style issues are introduced while fixing the code. Andinet 16:29, 30 January 2008 (EST) Fixed

SpatialObject

SINTEF (JB, TS, FL)

• igstkSpatialObject.h
  • igstkCoordinateReferenceSystem(.h & .cpp) and igstkCoordinateReferenceSystemDelegator(.h & .cpp) remove Reference? (fixed) Ibanez 09:17, 4 February 2008 (EST)
  • Too long \brief? No complete description. (fixed) Ibanez 09:27, 4 February 2008 (EST)
  • Constructor and destructor not documented. (fixed) Ibanez 09:27, 4 February 2008 (EST)
  • Lines longer than 78 chars: (fixed) Ibanez 09:35, 4 February 2008 (EST)
    • 72, 115
  • Code coverage:
    • 100%
  • Const?
    • Can be constant: void NoProcessing(); (Luis: Nope, it can't be const because it is a callback to the State Machine).Ibanez 09:27, 4 February 2008 (EST)
  • Statemachine diagram isn't generated. (?)
• igstkSpatialObject.cpp
  • "igstkEvents.h" is include both in the header and the source file. (it was already removed by the time I looked at the file)Ibanez 09:35, 4 February 2008 (EST)
  • Lines longer than 78 chars: (fixed) Ibanez 09:35, 4 February 2008 (EST)
    • 116
  • Missing "this->". (IGstk Style Guide: "When refereeing to a member variable, an explicit this-> pointer should be used. Derived classes must us the combination of this-> and Get/Set methods.")
    • 30,43,69,71, 74, 79, 93(x2), 100 (fixed) Ibanez 11:20, 4 February 2008 (EST)
  • Code coverage:
    • 83,33%

Additional comments:

• igstkSpatialObject.h
  • Events emitted from spatial objects should be documented. (fixed) Ibanez 15:38, 4 February 2008 (EST)
  • Line 71: Comment should not refer to past implementations but explain how this currently works (it was already fixed) Ibanez 15:38, 4 February 2008 (EST)
  • Line 60: Superfluous public: directive (not superfluous: it is needed because the macro above introduces a private section) (explanatory comment added) Ibanez 15:38, 4 February 2008 (EST)
• igstkSpatialObject.cxx
  • constructor: Use initialization list to initialize all class members (the only class member that is worth initializing in the class list is the m_StateMachine, and it is already there) Ibanez 15:38, 4 February 2008 (EST)
  • NoProcessing() is not used so it should be removed. (fixed) Ibanez 15:38, 4 February 2008 (EST)
  • Line 104: typo in comment: arised->arisen (fixed) Ibanez 15:38, 4 February 2008 (EST)
  • ReportInvalid*() should invoke error events to enable app developer to act on errors; (fixed)(now invoking InvalidRequestErrorEvent() ) Ibanez 15:38, 4 February 2008 (EST)

Andinet's comments:

• igstkSpatialObject.h
  • igstkTransfrom.h header file include not needed (Fixed) (verified Ibanez 15:39, 4 February 2008 (EST))
• igstkSpatialObject.cxx
  • igstkEvent.h is included again in the cxx file..(It was already included in the header file ) ( Fixed) (verified Ibanez 15:39, 4 February 2008 (EST))
• igstkSpatialObjectReader
  • igstkSpatialObjectReader.h
  • igstkSpatialObjectReader.cxx
    • Incomplete state transition matrix (7 inputs, 4 states, 12 transitions) (fixed)(now 28 transitions) Ibanez 16:11, 4 February 2008 (EST)

ObjectRepresentation

SINTEF (JB, TS, FL)

• igstkObjectRepresentation.h
  • Forward declaration:
    • "igstkSpatialObject.h" (can't be forward declared because we hold a SmartPointer to it) Ibanez 17:42, 4 February 2008 (EST)
    • "vtkProp.h" (fixed) Ibanez 17:42, 4 February 2008 (EST)
  • Lines longer than 78 chars: (fixed) Ibanez 17:42, 4 February 2008 (EST)
    • 43, 88, 129, 133, 157, 165, 209
  • Comment on lines 86 and 133 starts with a small letter (fixed) Ibanez 17:42, 4 February 2008 (EST)
  • Code coverage:
    • 100%
  • Delete? (objects that use smart pointers do not need a Delete() method: Ibanez 17:42, 4 February 2008 (EST))

(Luis: The following it was fixed when I look at them Ibanez 17:47, 4 February 2008 (EST))

/** DEPRECATED */
virtual void RequestUpdateRepresentation( const TimeStamp & time );
/** DEPRECATED */

/** DEPRECATED !!! DELETE THIS METHOD */
void RequestUpdatePosition( const TimeStamp & time ) {};
/** DEPRECATED !!! DELETE THIS METHOD */

/** DEPRECATED !!! DELETE THIS METHOD */
void RequestVerifyTimeStamp() {};
/** DEPRECATED !!! DELETE THIS METHOD */

• • Const?
    • Can be constant: void NoProcessing(); (It would be nice, but it can't. It is a callback for the StateMachine:Ibanez 17:42, 4 February 2008 (EST))
• igstkObjectRepresentation.cpp
  • Code coverage:
    • 94,23%
      • Ibanez 08:47, 5 February 2008 (EST):
        • This lack of code coverage revealed an important issue. The Object representation was still listening for TransformNotAvailableEvents, and not for the detailed events:
          • CoordinateReferenceSystemTransformToNullTargetEvent
          • CoordinateReferenceSystemTransformToDisconnectedEvent
          • CoordinateReferenceSystemNullParentEvent
          • CoordinateReferenceSystemThisParentEvent
          • CoordinateReferenceSystemParentCycleEvent
        • The negative events were not deriving from TransformNotAvailableEvent. (this has now been fixed. We should verify that it boosts code coverage to 100%
        • IMPORTANT : TransformModifiedEvent must be removed !!!!
          • It must be replaced with CoordinateReferenceSystemTransformToEvent
  • Lines longer than 78 chars: (fixed: now passing KWStyle test Ibanez 17:59, 4 February 2008 (EST))
    • 29, 47-51, 53-57, 59-63, 72, 73, 75, 86, 91, 112, 220, 237, 245, 264, 274, 302, 306, 321
  • Missing "this->". (fixed: Ibanez 17:59, 4 February 2008 (EST))
    • (IGstk Style Guide: "When refereeing to a member variable, an explicit this-> pointer should be used. Derived classes must us the combination of this-> and Get/Set methods."):
    • 31-35, 67, 72-73, 75-76, 78, 83, 88, 93, 98, 99, 114, 120, 121, 128, 140, 141, 144, 149, 158, 159, 165(x3), 169-171, 174, 175, 180(x3), 189, 193, 196, 197, 200, 213-217, 224, 226, 236, 245(x2), 249, 252, 253, 277-288, 308-309, 323-324, 344-346
  • 234: " // The response should be sent back in an event" (not necessary: getting the transform from the Spatial object is part of the internal dialog between the ObjectRepresentation and the SpatialObject: Ibanez 17:59, 4 February 2008 (EST))
  • Remove? (267-270) (not to remove: the comment are a valid description of what is happening in this function: Ibanez 17:59, 4 February 2008 (EST))

  // No new transform for us to use.
  //
  // Check if the old one has not expired.
  //

Additional commets:

• igstkObjectRepresentation.h
  • Line 42: Add to comment what determines if a representation will be visible. (transform validity..) (fixed: documentation comment added : Ibanez 18:10, 4 February 2008 (EST))
  • Line 63: Why typedef SpatialObject to SpatialObjectType? (good point, there is no need for it. fixed : Ibanez 18:10, 4 February 2008 (EST))
  • Line 64: Why typedef double to ScalarType? (good point, no needed: fixed: Ibanez 18:10, 4 February 2008 (EST))
  • Line 69: Arguments to SetColor should be double! (fixed: typedef added for it: Ibanez 18:16, 4 February 2008 (EST))
  • Line 77: Arguments to SetOpacity should be double! (fixed: typedef added for it: Ibanez 18:16, 4 February 2008 (EST))
  • Line 91:--> Remove deprecated functions (verified: they were already removed when I looked : Ibanez 11:02, 5 February 2008 (EST))
  • Line 105-106: Consistency:either use void as param definition or don´t (Good point: we should add this to the coding style. I don't think we have a rule on it yet)11:02, 5 February 2008 (EST).
  • Line 126: mColor[3] should be of type double (fixed: Ibanez 11:02, 5 February 2008 (EST))
  • Line 176: unnecessary private directive (fixed: Ibanez 11:02, 5 February 2008 (EST))
  • Group member variables (fixed: Ibanez 11:02, 5 February 2008 (EST))

• igstkObjectRepresentation.cxx
  • Constructor: Initialize members in initialization list (not really needed: Ibanez 11:02, 5 February 2008 (EST))
  • Line 199: Static cast used instead of dynamic cast. why? (good point: should be dynamic and tested after for not NULL : fixed: Ibanez 11:02, 5 February 2008 (EST))
  • Invalid state transitions should invoke error event (fixed: now invoking InvalidRequestErrorEvent: Ibanez 11:07, 5 February 2008 (EST))

Andinet's comments

• igstkObjectRepresentatio.h
  • Deprecated methods ( Removed!) (verified: Ibanez 11:08, 5 February 2008 (EST))
  • move method implementations to .cxx (verified: Ibanez 11:08, 5 February 2008 (EST))
  • Copy constructor and assignment operator overload private and not implemented ( Added ) (verified:Ibanez 11:08, 5 February 2008 (EST))
• igstkObjectRepresentation.cxx
  • codeline length style issues in the AddTransitionMatrix macro invocations ( Fixed ) (verified:Ibanez 11:08, 5 February 2008 (EST))

ViewProxy

Matt

• igstkViewProxy.h
  • template type should have a more meaningful name than 'T'. How about WidgetType? Andinet 12:00, 28 January 2008 (EST) Changed!
  • public methods should have documentation. Andinet 12:00, 28 January 2008 (EST) Added
  • Doesn't have a \brief doxygen description. Andinet 12:00, 28 January 2008 (EST) Added
  • 78% code coverage - SetPickedPointCoordinates and TypeMacro
  • One style indent issue (fixed).

Luis' Comments

• Minor changes in the Doxygen documentation (fixed)
  • Added \see also to the View.
• Code coverage 78%
  • Missing call to GetNameOfClass(). (fixed)
    • Added a helper method to the DefaultWidget, and call it from the igstkViewTest.cxx

ViewProxyBase

Matt

• igstkViewProxyBase.h
  • vtkRenderer and vtkRenderWindowInteractor could be forward declared and their includes removed. Andinet 12:04, 28 January 2008 (EST) Fixed
  • View could be forward declared and its include removed. Andinet 12:04, 28 January 2008 (EST) Fixed
  • 100% code coverage.
  • Style clean.

• igstkViewProxyBase.cxx
  • 80% code coverage - SetPickedPointCoordinates is not covered. (3 lines).
  • Style clean.

Luis' Comments

• Code Coverage (fixed)
  • Call to SetPickedPointCoordinates() added to the TestProxy() method in igstkDefaultWidget.h, which is called from igstkViewTest.cxx

View2D

Matt

• igstkView2D.h
  • Brief and long Doxygen description run together. Andinet 11:45, 28 January 2008 (EST) Fixed
  • Needs operator= and copy constructor private and unimplemented. (fixed)
  • 100% Code coverage
  • Style clean.

• igstkView2D.cxx
  • 100% Code coverage
  • Style clean.

Luis' Comments

• destructor was not marked as virtual (minor) (fixed)
• #include for the vtkCamera.h is no longer needed (fixed)
  • Now camera interactions go through protected methods of the View.
• Code Coverage
  • igstkView2D.cxx 100% Great !!
  • igstkView2D.h 100% Great !!
• The class is not in the StateMachineExportTest (fixed)
  • Its old (deprecated) API is being used there.

View3D

Matt

• igstkView3D.h
  • Needs operator= and copy constructor private and unimplemented. (fixed)
  • Brief and complete doxygen description run together. Andinet 11:46, 28 January 2008 (EST) Fixed
  • 100% code coverage
  • Style clean.

• igstkView3D.cxx
  • 77% code coverage - PrintSelf uncovered.
  • Style clean.

Luis's Comments

• It was missing from the StateMachineExportTest file (fixed).
• Code coverage
  • missing Exercising Print() method (fixed).
• virtual modifier in the destructor (minor, mostly for doc) (fixed).