Ray and I hashed out the names a bit. I don't mind shortening CellTableHeaderCreator to HeaderCreator. I don't think it'll cause any conflicts down the road.
@rjrjr - any objections to the shorter name? http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java File samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java#newcode82 samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java:82: public class CwCustomDataGrid extends ContentWidget { On 2011/08/02 03:47:57, skybrian wrote:
This example doesn't really hang together. On the one hand it's
showing friends
and birthdays (like a social app or a contacts database) and on the
other it's
showing who's eligible for retirement benefits (like an HR app). It
seems like
we should pick one or the other? Perhaps take out "retirement
benefits" and
replace that with something that makes more sense for a contacts
database? I removed the "retirement benefits" row completely. We only need one example of a spanning row, so the "birthday this month" is fine.
I also wonder if we should simplify it or split into two or more
examples where
each demonstrates one thing. For someone reading documentation, 800
lines of
sample code seems really big and I'd much prefer to read a smaller
example. This is a sample that is meant to illustrate what you can do with GWT. We show source code for people who want to delve into it, but the primary purpose of Showcase is to show off GWT, not to provide example code. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java File user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode493 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:493: rawHtml = rawHtml.substring(7, rawHtml.length() - 8); On 2011/08/02 03:47:57, skybrian wrote:
Perhaps add: assert tag.size() == 5
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java File user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode32 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:32: public interface CellTableHeaderBuilder<T> { On 2011/08/02 03:47:57, skybrian wrote:
How about just HeaderBuilder?
Its specific to CellTable/DataGrid. Other CellWidgets might have a different header builder. Ray and I came up with CellTableHeaderCreator. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode35 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:35: * A utility for building the header. On 2011/08/02 03:47:57, skybrian wrote:
"(or footer)"
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode44 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:44: public abstract class Utility<T> { On 2011/08/02 03:47:57, skybrian wrote:
Hmm, why isn't this an interface?
It seems like "Callbacks" would be a better name, although it looks
like you're
using "Utility" elsewhere.
Ray and I spent some time hashing out names, and we settled on CellTableHeaderCreator and Helper (instead of Utility). I don't like Callback because it isn't used in response to an event or an action, its just a helper class used to build the table. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode47 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:47: * Associate a {@link Column} with the specified element. If a column is On 2011/08/02 03:47:57, skybrian wrote:
"Enables column-specific event handling for the specified element."
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode52 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:52: * The element builder must be in a state where an attribute can be added. On 2011/08/02 03:47:57, skybrian wrote:
Could be moved to the "builder" parameter. Also, we could say something about where to get the builder. "The
builder should
be a child element of a row returned by {@link #startRow}."
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode58 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:58: public abstract void associateColumn(ElementBuilderBase<?> builder, Column<T, ?> column); On 2011/08/02 03:47:57, skybrian wrote:
Perhaps this should be named enableColumnHandlers?
Done. I like that better. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode72 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:72: * @param builder the {@link ElementBuilderBase} to render into On 2011/08/02 03:47:57, skybrian wrote:
Is this true? "This will normally a cell in a row returned by startRow()."
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode75 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:75: public abstract <H> void renderHeader(ElementBuilderBase<?> builder, Context context, On 2011/08/02 03:47:57, skybrian wrote:
I don't know how this fits into your existing conventions, but perhaps
rename
"builder" to "out"? (For render methods I like to put the thing being
rendered
into first, and name it "out".)
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode79 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:79: * Add a row to the table. On 2011/08/02 03:47:57, skybrian wrote:
Should this be something like:
"Adds a header (or footer) row to the table, below any rows previously
added." Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java File user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java#newcode114 user/src/com/google/gwt/user/cellview/client/DefaultCellTableBuilder.java:114: // Builder the cell. On 2011/08/02 03:47:57, skybrian wrote:
"Build"
Done. http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java File user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java#newcode214 user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java:214: * @param index the index On 2011/08/02 03:47:57, skybrian wrote:
What is this an index into?
Done. Specified that the index is the column index of the header http://gwt-code-reviews.appspot.com/1499808/diff/6001/user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java#newcode235 user/src/com/google/gwt/user/cellview/client/DefaultCellTableHeaderBuilder.java:235: protected void renderHeader(Utility<T> utility, ElementBuilderBase<?> builder, Header<?> header, On 2011/08/02 03:47:57, skybrian wrote:
Could we make the builder the first argument?
Done. http://gwt-code-reviews.appspot.com/1499808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors