diva.canvas: Design review, June 8th, 1998
John Reekie, 20 Jul 2001

diva.canvas: Design review, June 8th, 1998

• Preliminary notes
• Identified defect
• Related issues
• Concluding notes

Preliminary notes

Review called by John Reekie for the core package of his Diva canvas. Some of the issues raised seemed to be due to John transferring some of the code from Java2D based to Swing based.

• Moderator: galicia
• Scribe: nsmyth
• Author: johnr
• Reader: johnr
• Reviewers: none

Review started: 2.15 PM
Review ended: 3.30 PM

Identified defects

1. Figure Canvas should perhaps extend SwingComponent instead of java.awt.Component

   I renamed FigureCanvas to JCanvas, and made it a subclass of JComponent (the Swing root class). The internal architecture of the canvas is now more complicated, in order to provide Swing compatibility but keep performance "high."

2. Should re-ordering among layers in Figure canvas be supported?

   The new internal architecture of the canvas has the JCanvas containing a CanvasPane, which in turn contains CanvasLayers. Each layer is assigned to an index within a canvas pane, thus giving clients a simple way of rearranging layers. Layer indices don't have to be continguous. Lower-numbered layers are drawn on top of higher-numbered layers.

   Note: this revision has been superseded as a result of a subsequent re-review.

3. Double buffering is currently allowed to be turned on & off. The reasons why it might want to be turned off are unclear.

   Now that JCanvas inherits from JComponent, it also inherits the isDoubleBuffered and setDoubleBuffered methods. (It doesn't need to override them.)

4. In FigureContainer there is no description of what a Figure is, perhaps a short description would be useful.

   There's a description at the start of the specification.

5. The method damage() in FigureContainer is the only one that refers to the geometry of the container so it might be misplaced.

   I removed it.

6. In the class/interface diagram under Container & layers, Figure is not an interface.

   Redrew the class diagram (see next item)

7. It would be very helpful to have one big diagram of all the classes and  interfaces in the package showing  how they interrelate.

   Drew two class diagrams -- one for canvas, panes, and layers, and one for figures and associated classes.

8. The naming of FigureContainer and Layer interfaces could perhaps reflect better what their roles are.

   I can't think of any better ones, really. Container is used in AWT, so FigureContainer seems reasonable. I renamed Layer to CanvasLayer in the hope that that might be clearer. Swing uses layers in a similar way, so I think the term Layer is consistent with Swing.

9. In FigureContainer, the methods lower() and lower(Figure) might need to be renamed to reflect exactly the tasks they perform. e.g. lowerToBottom(), lowerBelow(Figure) ?

   I ended up with the following resolution. In the new interface ZList, which abstracts the notion of an ordered list of figures, I added methods

   void add (int, Figure)
   int indexOf (Figure)
   void setIndex (int,Figure)
   

   Combined with a well-defined meaning of what indexes mean (lower indexes are drawn above higher indexes, and -1 signifies the end of the list), this allows all of this functionality to be implemented without using awkward method names.

10. The same goes for raise(), raise(Figure)

    See above.

11. The issue of offset(relative) Vs. absolute coordinates needs to be looked at further. In particular, the issues raised when there is more than one root layer.

    I resolved this issue by making all figure drawing take place in "logical" coordinates in a canvas layer. Canvas layers can be stacked into canvas panes, and ultimately, the top-level canvas pane is part of a JCanvas -- this is the point where logical coordinates can be mapped to screen coordinates.

12. Does the outline of a figure need to be closed?

    Um. I'm not sure I understand this. If it means does the shape of a figure need to be closed, then the answer is no: Java2D will draw whatever shape you specify, closed or not.

13. StrokedFigure should have a shape.

    Done.

14. StrokedFigure and ShapedFigure might need to be renamed to convey exactly what they do. Suggestion: BorderedFigure and FilledFigure

    I renamed ShapedFigure to FilledFigure, since that's what it's really about. I decided to keep StrokedFigure as it is, partly because the purpose of the interface is to give the figure a java.awt.Stroke object that draws its outline, and partly because BorderedFigure would be misleading for figures such as lines, which are stroked but do not have a border.

15. Figure Classes: perhaps add a comment to getFeatureSet() saying exactly what a Feature point is.

    There's a description at the start of the document that we skipped over in the review.

16. should method hit() be called hits()?

    This is an awkward one, since it really should, but java.awt.Graphics2D has hit() for a method with similar purpose, so I elected to leave it as-is (and added a comment to the code).

17. isContainedBy() might need to be renamed to avoid confusion with parent container. This issue arose in several places. It might be worthwhile looking at how AWT differentiates between the container and the parent of an object.

    A similar comment applies to contains(), but since this is in java.awt.Shape, I kept contains(). On examination, it turns out that isContainedBy() is completely unncessary, as

    figure.isContainedBy(rect)
    

    would be the same as

    rect.contains(figure.getBounds())
    

    So I removed isContainedBy().

18. repaint() method needs to be worked out further i.e. exactly what it does and when it needs to be called.

    The repaint() methods of all classes are now consistent with awt/awing and each other.

19. Should the Glass classes be a sub package of the core package?

    Probably not... With the new layered architecture, I need to define a wrapper class that extends Figure and contains a CanvasPane. This will be in the main package, but won't be called Glass.

Related issues

none

Concluding notes

Most of the issues raised at the review were concerned with naming and placement of methods. An overall class diagram as suggested above would help clarify what is going on a lot, and perhaps raise issues we couldn't see today. In general for design reviews, a combination of an overall class diagram and the javadoc output might be the easiest way to proceed.