diva.canvas: code review, February 1st, 1999
John Reekie, 20 Jul 2001
Last updated: 20 Nov 2005

diva.canvas: code review, February 1st, 1999

• Preliminary notes
• Review materials
• Identified defects
• Related issues
• Concluding notes

Preliminary notes

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

• Moderator: neurendor
• Scribe: mudit
• Author: johnr
• Reader: michaels
• Reviewers:

Review started: 1.12 pm
Review ended: 2.15 pm

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. To get all of the source code for this and the next review, download this zip file.

JCanvas architecture

1. CanvasComponent.java
2. VisibleComponent.java

Canvas and panes

1. JCanvas.java
2. CanvasPane.java
3. GraphicsPane.java

Layers and Figures

1. CanvasLayer.java
2. FigureLayer.java
3. OverlayLayer.java
4. Figure.java

Identified defects

1. rewrite constructors so that it calls different constructor of the same class rather than repeat the same code. Supports code reuse.

   Done in JCanvas, not for any other classes. There's a tradeoff between "reuse" and obscurity. In some of these constructors, I would be replacing two lines including a call to super() into one line that calls another constructor.

2. JCanvas#paint()code for setting background to white is repeated in both branches. Maybe call a private method to set the color.

   It's only a few lines of code. Generally, I find that code that wraps up bits of code in obscure private methods is harder to understand, and in this case I think creating another method would fall into that category.

3. JCanvas#paint() A better comment explaining why we have offscreen == null (for GC?)

   Done.

4. JCanvas#setCanvasPane() Doubly referenced stuff. Comment it so that _canvasPane.setCanvas() is not called directly as that will lead to bugs.

   I don't understand the first sentence. I added this comment to CanvasPane.setCanvas(): This method is not intended for general use, only by JCanvas or subclasses.

5. JCanvas#setPreferredSize setSize and getSize to refer to each other. Probably should not call _canvasPane.setSize as that looks at the between JCanvas.setSize is still setting size, etc. Neurendor volunteered to explain this point better.

   Actually, I think this is correct, because these are two cases: one when the CanvasPane is contained by a JCanvas and one when it is not. I will draw an interaction diagram.

   [FIXME]

6. CanvasPane#private variables Comments should be more informative if there are comments at all.

   Firstly, I think it's good to consistently have a comment placeholder for all variables. Secondly, that doesn't mean that you have to duplicate information that's already in the set... method corresponding to the variable.

7. CanvasPane#dispatchEventSystem.out.println and FIXME should go. Bad message anyways.

   Fixed.

8. CanvasPane#processLayerEvent() Maybe layer should set the source itself rather than calling setLayerSource() on the event separately.

   The intention is that application-specific subclasses of CanvasLayer can be created, and so placing this in the CanvasPane relieves subclass writers of this particular detail. In some cases the call to setLayerSource() might be made twice buy this is not a problem.

9. CanvasPane#setCanvas() Message in exception seems sort of bogus. More clear message needed.

   Fixed, here and in setParent().

10. CanvasPane#setCanvas() Is "this._canvas" needed or should it just be _canvas?

    It's not needed but I think that where you have both "_canvas" and "canvas" visible in the same scope it's good practice to add the "this" qualifier to clearly distinguish them.

11. CanvasPane# Probably clone the transform

    I don't think this is necessary. In general, set...() methods do not clone the object being set, and in this case it's clearly documented that the transform will be remembered by the pane. I added a comment that any changes to the transform will affect the pane.

12. CanvasPane Wherever we have methods, use them rather than accessing flags directly. Maybe these methods should be final. Perhaps not final though so you can overwrite them later in derived classes.

    Hm. I guess the point is that either:

    1. set/getXXX() methods can be overridden, in which case the methods should be consistently used even in the defining class; or
    2. set/getXXX() methods cannot be overridden, in which case the defining class is free to access the private variables as it wishes, sice it completely controls access to them.

    I have chosen the latter for the Beans-style setter and getter methods, and made these methods final in CanvasPane, JCanvas, CanvasLayer, and FigureLayer. Interestingly, this strategy showed up an unneeded overridden method in FigureLayer, which I removed.

13. GraphicsPane#private variables ArrayList maybe an overhead. Probably an array will do.

    Changed to use an array.

14. GraphicsPane#private variables Should be 5 not 6 in arrayList

    Fixed. Use a constant now.

15. GraphicsPane#Constructor reference the final static variables rather than numbers directly.

    The array is now just a cache of the individual variables so the indexing constants no longer exist.

16. GraphicsPane#setBackGroundLayer Factor out setLayers methods

    Dubious benefit. No change.

17. CanvasLayer#constructor() Should this even exist. If yes, then should it be Setting the _containingPane to null or throw an exception.

    It is used by GraphicsPane. GraphicsPane constructs its layers without giving them itself as the parent, then it calls a common method on each layer to initialize it. Rather than try and change the way GraphicsPane works, I think it's better to leave this constructor in.

    _containingPane is already set to null. You can't throw an exception or the GraphicsPane constructor will fail.

18. FigureLayer# Comment about indices going from 0 to n ?

    I don't know if this applied to a specific method. At any rate, I added the following line to the class comment: Figures are also stored at contiguous integer indexes, with index zero being the "topmost" figure and the highest index being the "lowest" figure.

19. FigureLayer#contains() and dispatch alpahabetic order of methods

    Oops... Fixed.

20. FigureLayer#get Comments about how indices are arranged to get figure count, etc.

    I added the line: Indexes are contiguous from zero to getFigureCount()-1, with figures at lower indexes being displayed "on top of" figures with higher indexes.

21. FigureLayer#grabPointer Clear documentation about how this should be used.

    This is a very specialized method so it requires some detailed understanding of how the canvas works. I tried to clarify with this comment:

    Typically, this method will be called from an Interactor class in response to a mousePressed() event. The effect of calling this method is to make the series of mouse events up to and including the mouseReleased() event appear to be originating from the figure f rather than the actual figure that was clicked on. For example, if clicking and dragging on a graph node creates and the drags a new edge, this method will be called after constructing the edge to make the edge itself handle the mouse drag events instead of the node.

22. FigureLayer#get() All the index related methods should be removed as the zlist is anyway accessible.

    The problem with this proposal is that, if, for example, setIndex() is removed, clients will need to remember to call repaint() or they will get strange results. eg:

       layer.getFigures().setIndex(index, figure);
       figure.repaint();
    

    Calling setIndex() is a reasonably common operation, for example, with index zero it "raises" the figure to the top of the display list. I have left the index-related methods as-is, I don't recall any specific reason why they should be removed anyway.

23. FigureLayer#remove Factor out the common code from the methods.

    I don't understand, there are only two lines of code that could be factored out.

24. FigureLayer#repaint appears that damage region is being modified by TransformContext while everywhere else we assume that the pane deals with that.(neurendor)

    The factory method createDamageRegion requires a transform context argument, which is the context of the given coordinates. In this case, the transform context that is passed to the factory method will actually be the transform context of the containing pane, but that's fine. No change.

25. FigureLayer#undecorate Should it be child.repaint()

    Yes, it should! Fixed.

26. FigureLayer#dispatchEventUpTree and dispatchMotionEvent Should uptree transform the event?

    Since this review, Michael and I had a long discussion about how/whether to transform the coordinates contained within events as they were passed up and down the tree. I started to implement it, but had second thoughts after I realized that it introduced more complications for clients when dealing with transforms. After some back and forth, I realized that every possible choice seems to have about the same number of awkwardness dealing with transforms, and so decided to stick with the current strategy. Specifically:

    1. Layer events always contain coordinates in the transform context of the layer on which they originated. This means that event coordinates change when recursing into a nested pane, but not when merely changing contexts between nested figures.
    2. The responsibility is on the client to transform layer coordinates (as contained in the event) into the coordinates of the context of a particular figure where the event might have hit.

27. FigureLayer#processLayerEvent looks like code got pulled out into grabPointer but doesn't really call that method, instead repeats the code.

    Fixed. processLayerEvent() now calls grabPointer().

28. FigureLayer#processLayerMotionEvent doesn't have any implementation.

    [FIXME]

Related issues

coderating flags should be in files

Added Yellow to classes that are recognizable from earlier design reviews, Red to other classes.

Comments at some places do not completely explain the meanings. This is apparently to reduce duplication of documentation. Maybe what needs to be done is to refer the user from one method to another that has detailed comments. In specific, CanvasPane#isEnabled comments are not very informative.

The standard Java programming conventions say that setEnabled() and isEnabled() are a pair, why does each need to explicitly refer to the other? In general, I think it is adequate that setXXX() says what the effect of setting the variable is. No change.

FIXMEs need to be fixed.

[FIXME]

Exception messages should be more informative. They are quite terse.

I have made all exception messages more informative.

Concluding notes