This work is really awesome Joel.
I know that the code stills at early stage but if I can give you a first
feedback ...
-> Using CSS positioning is really a nice idea, I definitively buy it.
-> I have always been dubious about how GWT mix the concept of Layout and
the concept of Panel. If we take a look at Swing/AWT or Windows Forms/WPF,
layout are clearly separated from Panels (sure DOM doesn't help to do that).
wouldn't it be better to do :
DockLayout dl = new DockLayout(constraints)
SimplePanel sp = new SimplePanel();
sp.setLayout(dl);
Instead of :
DockPanel d = new DockPanel();
-> In the modified Widget class, I'm not a big fan of the circular reference
between layer and widget in the add(widget) method :
// Physical attach.
Layer layer = layout.attachChild(widget.getElement());
layer.setUserData(widget);
widget.setLayoutData(layer);
I understand that widget class should know the nature of the layout
constraints, but there are probably too much concepts, UserData, LayoutData,
etc ... a bit confusing
You have also layout.attach() and layout.layout() and layout.attachChild()
... also a bit confusing, we have to dig into the source code to really
understand that layout.attach() is about mem leak and layout.layout()
updates the layout children
-> I have tried to mix LayoutPanel and Panels that don't hold any
positioning styles (HorizontalPanels, FlexTable, TabPanel, ...). The way you
inject positioning attributes on any div element with attachChild() is
really cool. Please correct me if I'm wrong but say that you want to add a
FlexTable to an already attached parent, one have to write :
LayoutPanel leftTopLayout = new LayoutPanel()
leftTopLayout.add(tableHorizPanel); // This a normal HorizontalPanel we wrap
it with a Layout
// Want to inject a basic table div
// We should be able to get the parent underlying layout created previously
instead of re-creating a new one
Layout layout = new Layout(tableHorizPanel.getElement());
layout.attachChild(myflexTable.getElement());
tableHorizPanel.add(myflexTable);
We have to DOM add and Layout attach, don't you find the code redundant ?
-> and this would be my last remark ;-)
The following demo doesn't run in a consistent manner across all the
browsers, the page is in standards mode, you will find a link pointing to
the source code.
http://www.dng-consulting.com/blogs/media/users/sami/demogwt/MyGWTDemo.html
Hope this helps,
Sami
On Mon, Aug 3, 2009 at 10:14 PM, <jlaba...@google.com> wrote:
>
> Looks good. A couple of nitpicks, and I want to see it in action, but
> no problems so far.
>
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/2
> File layout/Layout.gwt.xml (right):
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/2#newcode17
> Line 17: <inherits name="com.google.gwt.user.UserAgent"/>
> Fix indent spacing
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/2#newcode19
> Line 19: <inherits name="com.google.gwt.animation.Animation"/>
> Fix indent spacing
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/5
> File layout/client/Layout.java (right):
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/5#newcode68
> Line 68: * <code>
> This code should be moved to the examples directory.
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/5#newcode102
> Line 102: * warning. Use at your own risk.
> Do we need a TODO to remove this when we release? Or are we going to
> change the layout after we release?
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/5#newcode154
> Line 154: Object userData;
> Should this be a parameterized type?
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/5#newcode177
> Line 177: * This is the element that sits between the parent and child
> elements. It
> typo - "It is normally", not "It normally"
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/3
> File layout/client/LayoutImpl.java (right):
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/3#newcode2
> Line 2: * Copyright 2008 Google Inc.
> 2009
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/4
> File layout/client/LayoutImplIE6.java (right):
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/4#newcode2
> Line 2: * Copyright 2008 Google Inc.
> 2009
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/11
> File user/client/ui/LayoutPanel.java (right):
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/11#newcode41
> Line 41: * // Attach two child widgets, laying them out horizontally,
> splitting at 50%.
> This code should be moved to the examples directory.
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/11#newcode142
> Line 142: assert child.getParent() == this : "TODO";
> You should put a message here
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/12
> File user/client/ui/RootLayoutPanel.java (right):
>
> http://gwt-code-reviews.appspot.com/51830/diff/1/12#newcode48
> Line 48: * // Attaches a pair of widgets to the RootLayoutPanel
> Move to examples directory
>
> http://gwt-code-reviews.appspot.com/51830
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
