We can discuss the class names in more depth...
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#newcode185 samples/showcase/src/com/google/gwt/sample/showcase/client/content/cell/CwCustomDataGrid.java:185: tr.startTH().colSpan(2).rowSpan(2).className(headerStyle + " " + firstColumnStyle); On 2011/08/01 17:53:54, rjrjr wrote:
Didn't you already prepend this space to firstColumnStyle in the
constructor? Done. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java File user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java#newcode44 user/src/com/google/gwt/dom/builder/shared/StylesBuilder.java:44: public interface StylesBuilder { On 2011/08/01 17:53:54, rjrjr wrote:
I know this isn't the right patch for this conversation, but have you
consider
the api evolution issue here? At the very least these interfaces need
comments
warning that they are subject to change, and that custom
implementations should
expect to get broken, yes?
Agreed. I'll add a warning in a separate patch to avoid bloating this patch. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/client/Style.java File user/src/com/google/gwt/dom/client/Style.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/dom/client/Style.java#newcode798 user/src/com/google/gwt/dom/client/Style.java:798: private static final String STYLE_LINE_HEIGHT = "lineHeight"; On 2011/08/01 17:53:54, rjrjr wrote:
Huge amount of redundancy between these strings and those in c.g.g.dom.client.Style, etc. Do I not worry about that because they
all get
inlined?
The compiler maps string literals that are equal to the same variable. For example, "border" in Style and SafeStylesUtils will map to the same variable in compiled JS.
Also, Style is missing this lineHeight attribute. Should it be added
there at
the same time? Is there anything we can do to tie the maintenance of
the two
packages together?
Missing from where? This patch adds lineHeight to Style and SafeStylesUtils. We could maintain them together by making the static strings public in Style, then converting to hyphenated form for CSS. The problem is that we now have a runtime conversion, or at the least a map lookup to go from camelCase to hyphenated. Alternatively, they could live together in some kind of StyleName object, but it would have to be public to be visible to both classes. I think the current implementation is a cleaner API. Also, they don't have to be completely in sync. If we add a change to the builder, we would naturally add the change to SafeStylesUtils and Style because the builder depends on those. If we only add the change to Style or SafeStylesUtils, it doesn't break the builder.
Confused by camel case here but hyphens in SafeStyleUtils (third
redundant
bag-o-strings). Are they actually different in css and js?
Yes, js uses camelCase, but css uses hyphenated. You cannot use one in the other. http://gwt-code-reviews.appspot.com/1499808/diff/1/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/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode368 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:368: * columns. On 2011/08/01 17:53:54, rjrjr wrote:
Should be in its own file. AbstractCellTable is huge.
Done. Also moved DefaultCellTableBuilder to its own file. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode428 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:428: // Cache the styles for faster access. On 2011/08/01 17:53:54, rjrjr wrote:
Here and in the showcase app, did you actually measure improved speed
from this
caching?
No, and I doubt it makes any difference for headers which have a limited number of Cells. I copied the code from CellTableBuilder, which always cached the style class names. Very early on in CellTable, I did a performance pass and found that reducing the number of string concatenations did make a noticeable performance improvement. I inlined most of the styles, except where it was convenient not to do so because the style name is used in multiple places. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode826 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:826: private class HeaderUtilityImpl extends CellTableHeaderBuilder.Utility<T> { On 2011/08/01 17:53:54, rjrjr wrote:
Does Utility really need the param type T? What happens if it deals
with
Column<?, ?> instead of Column<T, ?>?
associateColumn(builder, column) needs to take a column of type <T> to ensure its compatible with the CellTable itself. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode896 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:896: while (section.getDepth() > 0) { On 2011/08/01 17:53:54, rjrjr wrote:
Should this loop be added to the builders as endAll()?
Its actually already included in asSafeHtml(), so this loop isn't even necessary. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode1296 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:1296: // Strip the table section tags off of the tbody. On 2011/08/01 17:53:54, rjrjr wrote:
It seems like you have to do this kind of stripping a lot. Can the
builders
learn to do this for you? Render the inner build only?
Maybe as a future patch? It would take some testing to make sure we don't break the existing builder constraints. http://gwt-code-reviews.appspot.com/1499808/diff/1/user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java#newcode2972 user/src/com/google/gwt/user/cellview/client/AbstractCellTable.java:2972: if (elem == null) { On 2011/08/01 17:53:54, rjrjr wrote:
You sure you want this to mask NPEs?
Done. This was copied from isCellParent(Element), which could accept null in certain circumstances. That is no longer the case, so I removed the check here and from isCellParent. http://gwt-code-reviews.appspot.com/1499808/diff/1/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/1/user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java#newcode34 user/src/com/google/gwt/user/cellview/client/CellTableHeaderBuilder.java:34: public abstract class Utility<T> { On 2011/08/01 17:53:54, rjrjr wrote:
"Utility" is right up there with "Misc" in the pantheon of
non-descriptive
names. Even "Helper" would be better. Or Worker? Really, it seems like
this
thing is the builder, and CellTableHeaderBuilder is the
CellTableHeaderDelegate. "Delegate" sounds like its handling events or presenter logic. In this case, CellTableHeaderBuilder is responsible for building the structure of the table header, so I think "Builder" is the right term. As far as the Utility, its a set of methods defined by the CellTable implementation that allow it to associate rendered headers with events, and associate elements with Columns that may be sortable. Since the Utility is implemented by the CellTable, what about "TableConfiguration". The name to "Helper" sounds as ambiguous as Utility, and I think Worker refers to a specific pattern associated with threads.
Can you document just where implementations and instances of this are
found? Added some JavaDoc explaining that the CellTable implementation will define the Utility class.
Why is this an abstract class while the builder is an interface? How
are you
making these choices?
I made Utility abstract so we can add methods with default implementations in the future. I was copying the Cell/Context approach where we have a Cell interface, but each method takes a Context concrete class. It works well for Cells because we can add more methods to the Context without changing the Cell interface. In this case, the CellTableHeaderBuilder interface is straightforward and unlikely to change, but the Utility class is a way to let the CellTable know that certain elements are "special", each because they are Headers or they are sortable. As we add features to CellTable, we may need more methods in Utility that let the user configure the CellTable. The Utility can certainly be an interface with a warning that the API may change. With an abstract interface, we can add an method that throws an UnsupportedOperationException by default, but at least its not a breaking API change. I think its also useful to pass the CellTable instance with the Utility. Would you object to adding a getCellTable() method and taking the CellTable as a constructor arg? http://gwt-code-reviews.appspot.com/1499808/diff/1/user/test/com/google/gwt/user/cellview/client/CellTableTest.java File user/test/com/google/gwt/user/cellview/client/CellTableTest.java (right): http://gwt-code-reviews.appspot.com/1499808/diff/1/user/test/com/google/gwt/user/cellview/client/CellTableTest.java#newcode195 user/test/com/google/gwt/user/cellview/client/CellTableTest.java:195: table.getPresenter().flush(); On 2011/08/01 17:53:54, rjrjr wrote:
Why are these calls becoming necessary?
Currently, CellTable refreshes column widths synchronously every time a column is added or removed. I made a change to CellTable so it would defer setting the column width until the redraw loop, then reverted the change to avoid overcomplicating this change. So it isn't necessary now, but I may need to readd it in the future if we do defer updating column widths. http://gwt-code-reviews.appspot.com/1499808/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
