diva.canvas: code review, February 3rd, 1999
John Reekie, 20 Jul 2001

diva.canvas: code review, February 3rd, 1999

Preliminary notes

Code review of the mature parts of the Diva canvas. This is the second of a series of reviews on the canvas code.

• Moderator: neuendor
• Scribe: yuhong
• Author: johnr
• Reader: neuendor
• Reviewers:

Review started: 1:15pm
Review ended: 2:30pm

Review materials

This is a code review so the review materials are the source files as listed below. The list is structured into groups, with each group containing some pages from the design reference, followed by the list of classes. Note that the pages from the design reference are not under review, but are provided for context. The complete design reference is here; a printable (single file) version is here. The source code is in the same zip file as the previous review.

Figure sets and containers and Transform contexts

1. FigureSet.java
2. FigureContainer.java
3. ZList.java
4. TransformContext.java

Figure classes

1. AbstractFigure.java
2. AbstractFigureContainer.java
3. CompositeFigure.java
4. FigureDecorator.java

Identified defects

1. OverlayLayer: Document that no Z depth in OverlayLayer.

   Done.

2. OverlayLayer: The constructor with 0 argument should call the one with 2 arguments.

   It's only two lines of code, there's no need.

3. OverlayLayer: FIXME in repaint() needs to be taken care of.

   Fixed. Now if it doesn't have a BasicStroke it reverts to the slow but general verison of this method.

4. OverlayLayer: repaint(): Getting the damage region repaint is wrong, won't get the right region.

   Same fix as above.

5. OverlayLayer: setVisible() should repaint.

   Fixed. Also fixed in FigureLayer.

6. AbstractFigure.getLayer(): should check if _parent is actually an instance of Figure before casting and throw a run-time exception if not.

   It will throw a ClassCastException. I don't think there's any need to check that the type of _parent is correct, as if it is not, then whoever called setParent() had a bug, and that method is clearly documented as "not for general use."

7. AbstractFigure.repaint(): should document the role of this method in derived classes.

   I added

   This default implementation creates a damage region that is the same size as this figure's bounding box, and forwards that to the parent. Subclasses only need to override this method if they need to damage a region that is different from the bounding box.

8. AbstractFigure.setParent(): why is this method public if not intended for public use?

   So that classes that implement FigureContainer in other packages can call it. I changed the last line of the comment to this: This method is intended only for use by classes that implement FigureContainer.

9. FigureContainer.pick(): consider returning an Iterator or Set.

   That's actually fairly difficult and expensive if it's done the way it should be done (iterate all figures under the mouse even if in different subtrees), so is not a fundamental operation. There is an attempt at something that does this in CanvasUtilities but it won't work. This is definitely something that is needed, but should be done as a separate method somewhere else that uses FigureContainer.figuresFromFront() to recursively find intersecting figures, and then the client can filter them with hit().

10. AbstractFigureContainer.decorate(): should issue better exception message.

    Fixed.

11. AbstractFigureContainer.swapChild(): should the method name starts with _ since it is protected?

    No, the Diva convention is not to use underscores for method names. There are actually a few methods that break this convention, which appear to be used as a "this is kind of weird so be careful with this one" flag. No change.

12. AbstractFigureContainer.pick(): method out of order (also in other clases, especially in FigureLayer.java)

    Fixed in AbstractFigureContainer and FigureLayer.

13. AbstractFigureContainer.swapChild(): The 2nd Figure must not be a child, but not checked.

    Not needed. I changed the comment to clarify:

    Replace the first figure with the second. This is a hook method for the decorate() and undecorate() methods, and should not be called by other methods. Implementors can assume that the first figure is a child of this container, and that the second is not.

14. AbstractFigureContainer.swapChild(): better name for this method?

    I went with "replaceChild," unless anyone can think of a better one that will have to do.

15. AbstractFigureContainer.undecorate(): should issue better exception message.

    Fixed.

16. CompositeFigure._children: There is no simple mechanism provided by super class, so the comment needs work?

    Deleted comment.

17. CompositeFigure. The call to super() in the constructor is not necessary.

    Deleted.

18. CompositeFigure.add(): why cast to AbstractFigure?

    Removed.

19. CompositeFigure.contains(): why the comment says the implementation is wrong, but still implemented in the wrong way?

    Well, it's not wrong, it's just slower. I updated the comment to say so. I'm a bit leery of changing it to use if (child.getParent() != this) because that only is correct if the pointers are set up correctly... I dunno, somehow it seems to me that having the method do what it says it does and providing a documented faster way is better than hiding stuff.

20. CompositeFigure.some methods taken an index can be removed.

    See response to the same issue in the last review.

21. CompositeFigure.getBounds(): not clear why a FIXME is there.

    I replaced the FIXME with this comment

                // This could be made faster by optimizing for orthogonal
                // transforms. Since it's cached though, don't worry about it.
    

22. CompositeFigure.getBounds()(?): should use validate(), invalidate() methods.

    There's only one variable being cached, and it isn't visible outside this class. Adding validate() and invalidate() to do this is overkill.

23. CompositeFigure.paint(): the else{...} part should use canvasUtility.transform().

    Actually, I think I raised this issue. The code as it is is correct.

24. CompositeFigure.paint(): FIXME: need to be fixed (code should go back in.)

    Fixed.

25. CompositeFigure.pick(): not clear why a FIXME is there.

    Removed.

26. CompositeFigure.repaint(): better name for checkCacheValid()?

    This issue is in DamageRegion, not CompositeFigure. Maybe we could have a better name...

    [FIXME]

27. CompositeFigure.repaint(): why need to call checkCacheValid().

    [FIXME]

28. FigureDecorator.getFigureCount(): comment: when return 0? when 1?

    Fixed comment.

29. FigureDecorator.getContainer(): in the else{...} part, should not assume the parent is a FigureContainer.

    If the parent is not a FigureContainer, you have much more serious problems than getting a run-time exception here...

30. FigureDecorator.getShape(): should call child.getShape().

    Fixed.

31. FigureDecorator.newInstance(): improve class comments.

    [FIXME]

32. FigureDecorator.newInstance(): better method name?

    [FIXME]

33. TransformContext._cacheValid: which data is cached?

    [FIXME]

34. TransformContext.checkCacheValid(): what happens if TransformContext is not an ancestor?

    [FIXME]

35. TransformContext.checkCacheValid(): what is this supposed to do?

    [FIXME]

36. TransformContext.getInverseTransform(): how to do noninvertible transforms?

    [FIXME]

37. TransformContext.getInverseTransform(): better document for _cacheValid flag and _inverseTransform.

    [FIXME]

38. TransformContext.constructor should check if the component is null.

    [FIXME]

39. TransformContext._version is an int, should it be a long? what happens if it rolls over?

    [FIXME]

40. TransformContext.. what happens if push() and pop() are not called in the right order?

    [FIXME]

41. TransformContext.setTransform(): should make a copy of the AffineTransform.

    [FIXME]

Related issues

StructureFigure and subclasses could be explained better somewhere.

Concluding notes