On Tue, Aug 4, 2009 at 11:00 AM, Sami Jaber <[email protected]> wrote:
> This work is really awesome Joel. > I know that the code stills at early stage but if I can give you a first > feedback ... > Glad to have all the feedback I can get. Better now than later :) -> 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(); > We considered doing something like this originally, and I think it's the right approach for Swing/SWT. However, when you have to implement these layouts in terms of the HTML DOM, the Layout that you choose for a panel can have a huge effect on the DOM structure you have to create, affecting pretty much every method, so that the common "Panel" class ends up being nothing but a simple façade for its associated Layout. To make matters worse, it's damned near impossible to change a panel's layout after it's constructed, because it forces you to reconstruct the entire DOM. -> 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 > LayoutData has always been there, but it was just package-protected. The doc states explicitly that it may only be used by the panel that owns the widget, and it really does help avoid lots of extra maps, which can get pretty expensive. The userData on Layer should probably have been called userObject (like in TreeItem), and I should have made it invariant rather than having a setter (the next patch will include this change). I admit it's a bit odd, but the "user data" pattern (to the extent one can call it a pattern) can really make things a lot easier in this case. It's also worth pointing out that most users won't really be exposed to Layer.userObject, since they'll mostly be using panels and widgets. 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'm not a huge fan of attach() and detach() either, now that I think of it (actually, I *really* wish I could get rid of them, but this appears to be impossible because of requirements imposed by the IE6 implementation). What about calling them something like onElement[De A]ttached()? Would that make it a bit clearer? -> 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 ? > Actually, if you're using a LayoutPanel (or similar widget, more are on their way, such as DockLayoutPanel, StackLayoutPanel, and the like), you shouldn't ever have to use the Layout class directly. The above code could simply be: LayoutPanel leftTopLayout = new LayoutPanel(); leftTopLayout.add(tableHorizPanel); Layer layer = leftTopLayout.getLayer(tableHorizPanel); layer.setLeftRight(...); layer.setTopBottom(...); leftTopLayout.layout(); Does that make sense? I would prefer to modify add() to return the Layer directly, but it's inherited from HasWidgets (unfortunately, it was a bad idea to have add() there in the first place, but it's too late to fix now). -> 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 > I haven't gone through this code completely, but at first glance it appears to be using the Layout class directly on the RootPanel's element, which is unlikely to work in practice (you end up trying to wrap a Layout around the <body> element -- I think I need to simply require that the Layout class take a DivElement explicitly). The RootLayoutPanel is meant to solve this problem in a pretty simple way -- please let me know if it's not sufficient for some reason I'm not seeing. Also, I can't quite follow all the logic here, but do remember that you have to explicitly call layout() on these LayoutPanel (and RootLayoutPanel) instances once all the children are added and their layers' constraints set. Once I get the patch updated to take this (and other) feedback into account, I'll have a deeper look at the code to provide a simple example that I believe captures what it's trying to do. I realize it's probably not immediately obvious quite how all these classes fit together, so please let me know if you have any ideas on how I can make it clearer. Once it's all done, we'll be updating the samples, which should help, but I'm open to other ideas as well. Hope this helps, > Ça aide beaucoup, merci :) --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
