Hi Noel, I've been meaning to review GridPane, but I hadn't had the chance until now. All-in-all, it looks good, but there are some issues to address in the skin. It'd be nice if the ASF had something like ReviewBoard set up, where I could annotate the code, but instead, here's an old-fashioned review :)
- 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. - 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. - Can you implement the newly added TODO in getBaseline()? It should amount to a distillation of TablePaneSkin.getBaseline(). - 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. - 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). - 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(). - In paint(), once you make the two changes above, the outer 'if' statement will become superfluous :) - 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. Cheers, -T
