Code review of the mature parts of the Diva canvas. This is the first of a series of reviews on the canvas code.
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.
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.
Done.
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.
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]
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.
Fixed.
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.
Fixed, here and in setParent().
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.
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.
Hm. I guess the point is that either: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.
- set/getXXX() methods can be overridden, in which case the methods should be consistently used even in the defining class; or
- 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.
Changed to use an array.
Fixed. Use a constant now.
The array is now just a cache of the individual variables so the indexing constants no longer exist.
Dubious benefit. No change.
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.
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.
Oops... Fixed.
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.
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.
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.
I don't understand, there are only two lines of code that could be factored out.
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.
Yes, it should! Fixed.
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:
- 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.
- 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.
Fixed. processLayerEvent() now calls grabPointer().
[FIXME]
Added Yellow to classes that are recognizable from earlier design reviews, Red to other classes.
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.
[FIXME]
I have made all exception messages more informative.
Comments to:
johnr@eecs.berkeley.edu