Todd Volkert wrote: > - in getPreferredWidth() and in getPreferredHeight(), take care to account > for the case when you've been passed a constraint value of -1 > (unconstrained). The code right now assumes a positive value. Also, you'll > probably want to run a Math.max(..., 0) on your clientWidth/clientHeight > values. > > Will do. > - getPreferredHeight() seems to be missing some code. It's reporting the > sum of the preferred *widths* of the cells, when it should calculate the > maximum preferred cell height and sum that. > > Aah, missed that when I fixed getPreferredWidth(). Thanks. > - Can you implement the newly added TODO in getBaseline()? It should amount > to a distillation of TablePaneSkin.getBaseline(). > > I can, but it's not going to be very accurate. To be accurate I'd need to repeat most of layout(), and then do the "find the first visible component and get it's baseline.
> - The layout() method could be a lot simpler. I think we should define > GridPane as divvying up the width and height of the cell evenly in > layout(). GridPane would *report* a preferred size based on the maximum > preferred sizes of its cells, but by the time it's given a size and told to > lay out, it would ignore the preferred sizes of its cells and would simply > divide them evenly (after accounting for padding/spacing and collapsing > non-visible rows/columns). If we do this, then layout() implementation > becomes a lot simpler, and it should run faster as well. > > Funny, it was already doing that, the preferredSize calculations were completely superfluous. Taken them out. > - In paint(), you don't need to copy the graphics - you can just use it. > The graphics objects that are passed to skins are already copies and are > guaranteed to not affect any other component, so you can modify them and not > ever worry about having to restore them to their previous state. The only > times we ever create them in paint() methods is when we have a sub-routine > within paint() that we want to be isolated from the rest of the method (like > calling a renderer's paint() method). > > Removed copying. > - In paint(), I think you can safely remove the setting of the anti-alias > hint. We're only drawing lines here, so anti-aliasing shouldn't really buy > you anything. You can also remove the setStroke() call, since that's > abstracted from you in the call to GraphicsUtilities.drawLine(). > > OK. > - In paint(), once you make the two changes above, the outer 'if' statement > will become superfluous :) > > OK. > - Finally, do you mind signing up for the GridPane tutorial app and web > page? Thus far, the person who authored the component has been responsible > for writing up the tutorial on it. > > Will do. -- Noel.
