View
  By topic
  As outline
  Fully expanded

Forum topics
  diva-website
  diva-bugs
  sketch-and
  Design reviews
Dynamic Interactive Visualization

Forum

Welcome to the diva Forum.

Diva website redesign
We need to fix our site! This is a topic on how to fix the long-nagging bugs on our site, and how to make it fit into the GSRC web site as well. Diva kicks butt! Woo-hoo!

First cut
Michael and I met and came up with the following design. The high-level picture is that what is currently a single set of documentation needs to be split into two:
  1. The actual Web site itself
  2. Developer documentation for each individual release.

These two sets of documentation will be mutually exclusive (kiss...). The Web docs will point to copies of the developer docs for i) the current release and ii) the current snapshot (and possibly past ones as well?)

The documentation downloaded as part of the release is very simple in layout and formatting. Ideally, this documentation is clearly marked with the release or snapshot version on every page to highlight the fact that it is version-specific. The index.html page contains links to the following sections:

  • A link to the website itself!
  • Installation
  • Installing and running demos
  • Trouble-shooting
  • List of changes since last snapshot/release
  • Package documentation -- canvas, graph, sketch etc
  • API docs (by default the index.html is just a page that says "build the docs")
  • Acknowledgements and copyright notice (also duplicated on the website)

The website has the following sections:

  • Home
  • About
  • Demos, eventually with applets. Initially some of this will be duplicated with the developer documentation.
  • Downloads. Tne download page has links for the current release and snapshot to:
    1. Individual zip and tar files
    2. A copy of the developer docs for that release or snapshot
Also, it would be nice to integrate the navigation links for the database driven pages:
  • Forum
  • FAQ
  • Private forum
  • Mailing lists
  • Task list
  • Bug database
(Perhaps this could be both linked on the index page text, and also appear on the footer of individual pages?)

Progress so far

I have made a first stab at implementing the changes we discussed. I have created two new directories, newdoc and newweb which will eventually replace the doc and web directories. The HTML files in these directories are devoid of headers/footers/navigation, since this will all be generated using PHP. The structure is my proposed structure for the site. I think it's an amazing improvement over what we had before.

The following changes still need to be made:

  • The headers/footers/navigation still needs to be added to the web page using PHP. Eventually the JavaScripted navigation buttons will be put back in... =)
  • The download page needs to point to the right download files and the right browseable documentation.
  • The package directory in the developer documentation needs to be filled in. Right now there is an index.html but no subdirectories. I think each of us should be responsible for updating our own subdirectories once everything else on this page has been figured out. Divide up the work a little bit.

I think that's it for now.

Outstanding issues
I think things are pretty much there. There are several things that need to get done for the new web site:
  1. downloads page needs to be finished
    • format files nicely
    • online docs point to where?
  2. javascript needs to be updated to point to absolute URL's or to somehow understand relative URL's. right now if you go to "about" page the images don't show up properly.
  3. release scripts need to be finalized. must generate both release tarballs and also online documentation.
  4. style-sheets must not be stripped from the html (!!!)
  5. fancy images for "main" pages
I think that's it. Please let me know if there are any other issues that you can think of.

More Web-site fixes needed!!!
  • API docs get passed through (for frames) but the BODY tag is getting lost!
  • Need visual indicator on snapshot documentation to indicate that is a snapshot. Generalize using a "banner" option?
  • Document special cases: eg /api/ doesn't process text.
  • Fancy HTML option for "logo".
Web-site generally...
  • Add email address/s to bottom of group pages!
  • Add sequence generation to FancyHTML.

Diva bugs and fixes
Discuss bugs in and fixes for Diva, as a precursor to release 0.3.

Graph/connector bugs
Assigned, assigned to Michael Shilman
While trying to fix problems in the Ptolemy schematic editor, Steve and I identified the following outstanding bugs in the Diva graph and connector code.
  1. Grab handles don't understand transform contexts, so when a connector spans two sites that are located in different transform contexts, and is then selected, the grab handles show up in the wrong places. The buggy line is commented in BasicGrabHandle.java.
  2. None of the ConnectorTarget implementatios understand hierarchy, so when you drag an edge over a node nested within a composite node, the edge does not snap to it. I'm not sure if this worked in the previous version, but I think the fix is to make ConnectorTargets understand hierarchy... Comments?
John Reekie ·  Status changed to Open. Task assigned to John Reekie.
Stephen Neuendorffer, PhD ·  Task assigned to Michael Shilman. Michael and I are going to work on this task this morning.
Michael Shilman · 
Michael Shilman ·  Category changed to Canvas. Status changed to Assigned. Thigs is way cool... Now let's see if it actually makes me more motivated to fix bugs. =)

Sketch revision
Not started, assigned to Heloise Hse
test
Heloise Hse ·  Category changed to Sketch.

Desktop manager
Not started, assigned to John Reekie
Write an implementation of DesktopManager that will allow us to implement internal frame management more sensibly than the current hacked-up approach with JPseudoFrame.

Fix the bug where the icons don't show up in JPSeudoFrame in JDK1.3.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

diva.canvas KeyEvents and MotionEvents
Not started, assigned to John Reekie
Make KeyEvent and MotionEvent propagate down the canvas hierarchy properly.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

JPalette sucks
Not started, assigned to John Reekie
Both implementations of drag and drop suck. One because it uses icons not figures, and the other (Steve's) because it depends on Ptolemy. Write one that's useful.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

Finish diva.graph.schematic
Not started, assigned to John Reekie
Whatever that means. Steve has a working (just) schematic editor but it's tied into Ptolemy.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

Finish SwingWrapper
Not started, assigned to John Reekie
Actually, my inclination is to just dump this. Maybe Michael wants to have a crack at finishing it.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

diva.canvas.AttachableFigure
Not started, assigned to John Reekie
Steve's idea. Implemented by LabelFigure, StraightTerminal, and AttachableWrapper. See the current StraightArc (?) and LabelWrapper for an example of what could be cleaned up.

Not a high priority, really.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

SketchGraphController
Not started, assigned to John Reekie
I was going to rewrite this cleanly and not inheriting from BasicSketchController. Realistically, this is not likely to happen.

Plus, I'm tired of wading through reams of incomprehensible code that barely does anything useful but is defended on vacuous "what if" grounds. I vote we dump it.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

diva.graph.editor
Not started, assigned to John Reekie
Need to be finished. I was working on this but ran out of time.
John Reekie ·  Created. Status changed to Not started. Task assigned to John Reekie.

Sketch and whiteboard
Issues related to the diva.sketch and diva.whiteboard packages.

Reactive vs. call semantics
I have some design issues for the sketch package. In the current design (and implementation) communication between a low-level recognizer and a high-level recognizer (and also the high-level => application) is achieved through an event-listener construction. Mouse events are passed to the low level recognizer, which in turn generates recognition events, etc. This flow of control and information gets passed up the tree in a "reactive" fashion.

It seemed like the right thing to do before, but the more I think about it, the more I think it can be done using a standard call semantics. Control would flow down the tree, and information would be passed back up the tree. The application would get a mouse event, ask its high-level recognizer to recognize the event, which in turn would ask a low-level recognizer, etc. The return values of these method calls would propagate information back up the tree.

I'm contemplating a redesign of the package and it seems like the call semantics are more intuitive/natural and require much fewer lines of code to achieve. On the other hand, the reactive semantics seem to be more flexible (fanout is possible, for example, though I can't think of where it would be useful in the case of recognition--debugging perhaps?). Some pros to the reactive approach:

  1. Consistent with the Java event model--e.g. a client would simply "register" with a recognizer to be updated with a much higher level of event than the mouse press.
  2. Consistent with the Beans model, which is could be a plus.
  3. Asynchronous recognition is a better fit with this model, though we are planning not to support that at this point in time.

I a discussion with Ali, he compared the two approaches to an OS "service" versus "event" model. He supposed that if the application was going to be dealing primarily with events, that it would be more consistent to have the recognition also provide an event-based interface. From my perspective, if the application is dealing with low-level mouse events anyway, it might be more consistent to abstract the recognition into a service so that the application doesn't have to deal with so many types of events.

I don't want this to be a distraction, and I think there are higher priority items to deal with, but I want to put this forward as a potential discussion topic if anybody has thoughts on this.

Sketch todo list
Completed, assigned to Heloise Hse
1 architecture revision (hard-hat to beta1, interpreters)
  We want to make the architecture less complicated,
  especially the intersection between application and
  high-level recognition.  So that people can easily 
  use our infrastructure to build sketch-based UI for
  their applications.  Right now this line is kind
  of blurry.
2 file formats
  - XML
  - GIF
3 liveboard maintenance
  Get a new pen for the liveboard in the CAD lounge
4 John's tablet
  Try out John's tablet.  We might want to buy
  different kinds of pen and tablets (on a return
  policy), try them out to see which ones we like 
  to use.  Eventually, we might want to supply CAD
  group students with pens and tablets and have them
  be our whiteboard users.
5 whiteboard v1.0
  - save/load (xml)
  - print?
  - export to GIF
  - pen width/color
  - in John's GUI framework
  - on liveboard
  - graffiti support
  - selection/move/resize
Heloise Hse ·  Status changed to Completed. Whiteboard application supports export to JPEG instead of GIF, because only JPEG is in JDK1.2. For now I think exporting to JPEG is sufficient. I will look into Java Advanced Imaging for saving to different formats.

graffiti support is not provided in whiteboard because the recognition didn't work that well. Also I don't think it's that important for whiteboard to have character recognition capability. It's probably more useful to provide some structural interpretation.

Other than the two points mentioned above, the rest of the tasks has been completed.

recognition/type class redesign
The recognition package currently employs 
a Recognition class that encapsulates multiple
interpretations of a single stroke as a set of
<Type, confidence> pairs, where confidence is a
double value.  I designed the classes Recognition
and Type with single-stroke recognition in mind,
with the intent of redesigning them once I added
multi-stroke/composite recognition.  Now that the
Parser2D package is complete, I think it's time
for a redesign.

A little background on the Recognition class: a
Recognition object is returned by a Recognizer in
response to some change in a TimedStroke.
For example:

public Recognition strokeCompleted(TimedStroke s);

So the Recognition class currently pertains to that
one stroke.  Now that multi-stroke recognition is
supported, the Recognition object must pertain to
multiple strokes.  I propose that the Recognition
object be a pair of <Type, confidence> values, where
the Type object is now either a scalar Type (single
stroke) or a CompositeType (multiple stroke, i.e.
scrollbar from the Parser2D user manual).  If it is
a composite Recognition, then the confidence values
will be confidences not of the individual stroke but
of the entire "world state".  This will involve another
change, which is how the stroke object is 'linked' to
Type.  Currently, all the types in a Recognition object
are implicitly linked to the TimedStroke that initiated
the recognition.  If there are composite types, then
the type objects will have to be modified to have
explicit links to the strokes that they represent.

Example:

Input to Recognizer: (stroke4 completed)
Output from LLR: [[downTri .95] [upTri .8] ...]
Input1 to non-incremental Parser2D: [vrect square upTri downTri]
Output1 from Parser2D: [scrollbar [boundary vrect] [handle square] 
                         [upArrow upTri] [downArrow downTri]]
Input2 to non-incremental Parser2D: [vrect square upTri upTri]
Output2 from Parser2D: []
Recognition: [scrollbar [boundary [vrect stroke1]] [handle [square stroke2]] [upArrow [upTri stroke3]] [downArrow [downTri stroke4]]

Open issues:
1) Deltas between world states.  In previous e-mails I
have advocated the need for representing mutual
exclusion in the recogniton objects.  So if a re-recognition precludes a world state from a previous
recognition, this should be expressed as a delta from
the previous recognition.  [more here]

Design reviews
Design review materials and notes

diva.sketch.parser2d Design Review, 10/99
This is a design review of the package diva.sketch.parser2d, a 2D parsing implementation for sketch recognition applications and beyond. The review materials (user manual, design document, and API documentation) are attached to this article as a ZIP file.

diva.canvas: comments, May 1st, 1998

diva.canvas: comments, May 1st, 1998

Email comments received after posting preliminary spec to the Java2D mailing list, and responses.

Shawn Rutledge 


Date: Fri, 01 May 1998 19:09:00 -0700
From: "Shawn T. Rutledge" 
To: John Reekie 
Subject: Re: Structured graphics

Java2D is part of JDK 1.2, which also integrates Swing, so I'm surprised you didn't base your components on JComponent. As is they would be heavyweight wouldn't they? Also, "damage" is redundant; you can use repaint. In Swing, "glasses" aren't special, because you can call setOpaque(false) on any JComponent, and the result of that is that paint() will not automatically paint the background before calling paintComponent(), therefore it will be transparent. JFrames have a layered architecture, including a "glass pane" that is on top of everything, and you can add other layers. You can also build layered architectures inside JComponents, using things like OverlayLayout and JLayeredPane (but I'm not an expert on that yet). I don't see a lot of value in your event dispatching changes either; it's really not that big a deal to have to implement two listeners to get all the mouse events (I wondered why too, but it's not worth much nitpicking either).

Then there's the "figure", which is kindof a neat idea and many people think that they need such a thing, although typically when following the MVC pattern, you'd separate the view from the model anyhow. You can store a Shape in the model, and the view (subclassing JComponent) can have a paintComponent which calls draw(Shape) on its given Graphics2D. "Features" are a good idea; so far I don't know of any of this kind of support in Java2D. Again, you need to separate model from view and from control; if you allow the user to drag a feature with the mouse, that is control functionality, and should change the location of the feature within the "figure" model, not the view. The model then notifies the view(s). Always assume there will be more than one view/control combination for each model (users might want to see two different perspectives or zoom levels on the same drawing at the same time, and be able to edit in both windows). Animation is a good idea and I don't think is supported much in 1.2. Interactors appear to be a really good idea. This area is a very common weakness in many existing frameworks, and I haven't worked with any really complete designs for this sort of thing yet so I'm not quite as opinionated about what is the right way to do it. :-) But Taligent's CommonPoint seemed to have a decent design. BTW have you found any way to move the mouse cursor programmatically? This seems to be a Java shortcoming, and severely limits the kind of user feedback you can give in situations where you want to constrain the movement; you can constrain the movement of what is being dragged, but not the cursor itself.

A drawing package is a "poster child" for MVC; I'd consider it a must to separate the model, view, and control. There is a lot less blurring of those lines than there is in most "business" applications.

So in summary building a framework to make it easier to build drawing packages is a good idea, but yours doesn't have broad enough coverage of the needed features, yet, as well as arbitrarily transmogrifying things that already work well enough. 

-- 
  _______                 KB7PWD @ KC7Y.AZ.US.NOAM   ecloud@bigfoot.com
 (_  | |_)  Shawn T. Rutledge            http://www.bigfoot.com/~ecloud
 __) | | \_____________________

John Reekie 


From: John Reekie 
To: "Shawn T. Rutledge" 
Cc: johnr@kahn.eecs.berkeley.edu
Subject: Re: Structured graphics
Date: Mon, 04 May 1998 13:24:36 -0700
Thanks for your comments -- very thought-provoking! There is currently a compatibility problem with Swing and 2D, but I do intend to use Swing eventually. I wasn't aware of the layers support, so this is definitely something I will use. Thanks for the tip, I'm still gqetting up to speed on the 2D API, Swing is a little down the track... I oscillated back and forth on the new listeners -- I wanted a conceptual separation between entering and leaving, and dragging, especially for the interactors. But maybe you're right...

One thing I wasn't clear on: do you think every layer should be a JComponent? I am planning on using layers as figures embedded within a larger figure, for doing things like level of detail, zooming into different visualization spaces, and so on. I have been somewhat disconcerted by the complexity and performance (i.e. dismal) of Swing...

On MVC: the canvas is purely part of the view (except for the interactors sub-package). Each Figure has setModel and getModel methods to access its model. I think that it's important not to have things like Shapes in the model, since each view might render the model in a completely different way. The location, maybe, but not the appearance. Same deal with features: these support manipulation of the on-screen representation, but where that comes from is entirely up to the programmer. In an MVC-style architecture, it's coming via the model. I am trying to be careful to keep the scope of this package purely to supporting on-screen representation, without assuming particular applications other than "interactive 2D."

Shawn Rutledge 

Date: Mon, 04 May 1998 19:56:35 -0700
From: "Shawn T. Rutledge" 
To: John Reekie 
Subject: Re: Structured graphics
I oscillated back and forth on the new listeners -- I wanted a conceptual separation between entering and leaving, and dragging, especially for the interactors. But maybe you're right...
If nothing else having two different models is extra clutter in a programmer's mind, because he or she will never be able to escape the official Sun event model anyway.
One thing I wasn't clear on: do you think every layer should be a JComponent? I am planning on using layers as figures
Well there isn't any higher base class for lightweight components, unless you impart the lightweightness yourself (I did this once, forget how...) I guess you could just have an object for each layer which stores Shapes to be drawn, and then in the layered component's paintComponent just draw all the layers' Shapes in order, but that seems like re-inventing the wheel to me. Containment isn't that expensive with Swing (but with peered components, each peer consumes a Windows resource, and painting the various layers involves a lot of going back and forth between Java code and native code; so that's why I think complex arrangements of components should be lightweight).
On MVC: the canvas is purely part of the view (except for the interactors sub-package). Each Figure has setModel and getModel methods to access its model. I think that it's important not to have things like Shapes in the model, since each view might render the model in a completely different way.
Aha, good point.

diva.canvas: comments, May 6th, 1998

diva.canvas: comments, May 6th, 1998

Email comments received after posting preliminary spec to ptdesign.

Michael Shilman 


From: Michael Shilman 
To: "'John Reekie'" 
Subject: RE: Java canvas spec
Date: Wed, 6 May 1998 01:11:20 -0700
Hi John,

I didn't have time to scrutinize your API, but I did take a quick look. It's very polished and looks very good, though it'd really take some serious thinking to give it a proper critique. I don't have time for serious thinking these days, so you'll get a half-assed critique. =)

I think there is a lot of useful functionality in the API you've spec'ed. The question I have is how much of that functionality is beneficial to me as the user, and how much of it is just going to make my GUI run slower? What are the abstraction layers in your API, if any? Does the API simply provide structure, or does it also provide an abstraction by which common operations can be optimized behind an abstraction layer?

Related: which API pieces build on each other? Can you reuse code in this way to reduce the number of distinct functions in your spec? Seems like, e.g. Layer is just a special type of Group--does it need its own class?

About specific features: I think the predicates look very elegant. However it'd be nice if you could give some situations in which you'd use them. Is this a Tcl thing?

Is there any way to make the event API more like the AWT1.1?

Write some "case studies"; things that would push the limits of your model. For example, I have a graph editor and I'd like to draw edges between nodes in two different coordinate frames. How would I do that in your system? I'd like to drag a node out of its parent and into it's parent's parent--I'd like the coordinate frame to change, but not the size/aspect ratio of the node wrt the user. How do I do event handling for this?

Finally, would this extend to 3D at all? It'd be nice if it did, though I know that's asking a lot...

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

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

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.

diva.canvas: Design re-review, June 22nd, 1998

diva.canvas: Design re-review, June 22nd, 1998

Preliminary notes

Re-review called by John Reekie for the top-level architecture of diva.canvas, redesigned to deal with problems notes in the last review with coordinate systems and Swing compatibility. There was only one reviewer (Neil Smyth) but sometimes you just have to work with what you can get...

The reviewed file is tagged version-0-3 in the diva repository.

  • Moderator: nsmyth
  • Scribe: johnr
  • Author: johnr
  • Reader: johnr
  • Reviewers: none
Review started: 4.10 PM
Review ended: 4:50 PM

Identified defects

  1. CanvasPane would be simpler if it just kept an order list of layers, instead of the current non-contiguous set of integers.
    It would be! I made CanvasPane into an abstract class, so that subclasses are required to provide the storage and access methods for layers. This is better anyway, I think: rather than having a complicated API that allow clients to manipulate layers in every way possible, just let them define how they want the layers and provide access to them. In addition, I defined the class BasicCanvasPane, which contains just a simple linear list of layers with a couple of access methods.
  2. CanvasLayer needs to be made consistent with CanvasPane in the paint() and repaint() sets of methods.
    Done.
  3. FigureContainer.children() and ZList.overlapping() each need to be made into two iterator methods, instead of returning an array.
    I created the following methods: 
    Iterator FigureContainer.figures ()
Iterator FigureContainer.figuresFromFront ()
Iterator ZList.figuresOverlapping (Rectangle2D region)
Iterator ZList.figuresOverlappingFromFront (Rectangle2D region)
    Note the change from "children" to "figures," which I think is more consistent with the meaning of the methods.
  4. CanvasPane.addLayer(): order of arguments is wrong.
    Fixed.
  5. None of the methods in FigureContainer or ZList define the behaviour in anomolous cases eg adding a figure that already exists, and so on.
    Decided and documented.

Related issues

none

Concluding notes

Rework is still being done following the previous review. In particular, the class diagrams need work.

Now that I've starting writing this code, I am converting the specification into the programmer's guide. It's too tedious keeping the spec in sync with the code, as there are lots of small changes, so refer to the API javadocs to verify the above changes.

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

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

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

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

    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

    		•
    Send feedback to cxh at eecs berkeley edu
    You are not logged in 
    ©2002-2009 U.C. Regents