Code review of the mature parts of the Diva canvas. This is the second of a series of reviews on the canvas code.
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
Done.
It's only two lines of code, there's no need.
Fixed. Now if it doesn't have a BasicStroke it reverts to the slow but general verison of this method.
Same fix as above.
Fixed. Also fixed in FigureLayer.
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."
I addedThis 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.
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.
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().
Fixed.
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.
Fixed in AbstractFigureContainer and FigureLayer.
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.
I went with "replaceChild," unless anyone can think of a better one that will have to do.
Fixed.
Deleted comment.
Deleted.
Removed.
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.
See response to the same issue in the last review.
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.
There's only one variable being cached, and it isn't visible outside this class. Adding validate() and invalidate() to do this is overkill.
Actually, I think I raised this issue. The code as it is is correct.
Fixed.
Removed.
This issue is in DamageRegion, not CompositeFigure. Maybe we could have a better name...[FIXME]
[FIXME]
Fixed comment.
If the parent is not a FigureContainer, you have much more serious problems than getting a run-time exception here...
Fixed.
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
[FIXME]
Comments to:
johnr@eecs.berkeley.edu