PDA

View Full Version : Unnecessary doLayout() calls by Ext.layout.ContainerLayout.configureItem()?



pinecone_web
11 Aug 2009, 7:38 AM
Hi,

I used Ext 2.2.1 before. After playing with 3.0.0 for a while, I notice that the doLayout() of a Component is called much frequently than before. Reading the code, I found that the Ext.layout.ContainerLayout.configureItem() will call doLayout() of an item. This makes each call to Ext.layout.ContainerLayout.renderAll() (at the time either when rendering a container, or when resizing a container) goes through a downward rendering/layout propagation along the Container->Component hierarchy. For example, an container "A" which is is contained by a viewport, say "B", will experience 3 rendering/layout propagations during the rendering of B, one at the time when rendering A (called B.layout.renderAll()), one at the time when resizing A (called by B.layout.layout()), and one at the time when B.doLayout() recursively calls each item's (including A's) doLayout().

However, as we can see, the first two propagations are unnecessary, as when rendering A, its doLayout() will be called by B if B is not contained by any other container. The endering/layout propagate automatically. Furthermore, when resizing A, the resize event will be fired by items whose sizes are changed. Again, the layout propagates downward automatically.

Is it really necessary to call item.doLayout() in Ext.layout.ContainerLayout.configureItem()? For what reason it is added in 3.0 (I didn't see it in 2.2)? This may seriously effect the performance of a complex container where forward rendering/layout propagation is costly.

tryanDLS
11 Aug 2009, 8:13 AM
Please don't post the same thing in multiple places - duplicate post removed.

Animal
11 Aug 2009, 10:39 AM
Was the thread removed from Bugs?

Because I think this is a bug. doLayout should not be called at render time of every contained Container.

pinecone_web
11 Aug 2009, 4:13 PM
Sorry for the multiple posts as I don't know this is a bug or an "update."

I tried commenting out the doLayout() call in Ext.layout.ContainerLayout.configureItem() in a simple example where two panels (one embedded with google map) are contained in a viewport. Observations: No error happens; nearly 2 times faster performance (and a dump of Container->Component rendering now looks the same with Ext 2.2.1).

However, I believe it must be added for some reason. Could someone tell me why please. Thanks in advance.

Animal
12 Aug 2009, 12:21 AM
I would have thought it was added for a reason. I'll go through the commit log to see who did it.

but I'm pretty sure this is a critical bug, and if it's in the grand 3.0.0 release, there will have to be a patch revision immediately.

evant
12 Aug 2009, 1:42 AM
Possibly, the code should be:



if(c.doLayout && this.forceLayout){
c.doLayout(false, true);
}

Animal
12 Aug 2009, 2:56 AM
Yes, that flag is used by CardLayout to implement deferredRender: false.

There's also this code in CheckboxGroup



doLayout: function(){
//ugly method required to layout hidden items
if(this.rendered){
this.panel.forceLayout = this.ownerCt.forceLayout;
this.panel.doLayout();
}
},


forceLayout in a Panel is not used. Was the intention there to force a layout even if hidden? I can't see why. But if so, it should use the second parameter to doLayout.

The doLayout in Container also sets the property on child Containers instead of passing it into the Container's doLayout call.

Code is now



doLayout: function(shallow, force){
var rendered = this.rendered,
forceLayout = this.forceLayout;

if(!this.canLayout() || this.collapsed){
this.deferLayout = this.deferLayout || !shallow;
if(!(force || forceLayout)){
return;
}
shallow = shallow && !this.deferLayout;
} else {
delete this.deferLayout;
}
if(rendered && this.layout){
this.layout.layout();
}
if(shallow !== true && this.items){
var cs = this.items.items;
for(var i = 0, len = cs.length; i < len; i++){
var c = cs[i];
if(c.doLayout){
c.forceLayout = forceLayout; // <--- does nothing
c.doLayout();
}
}
}
if(rendered){
this.onLayout(shallow, force);
}
delete this.forceLayout;
},


I think it should use that second parameter to propagate any forced layout down to all child Containers:



doLayout: function(shallow, forceLayout){
var rendered = this.rendered,
forceLayout = forceLayout || this.forceLayout;

if(!this.canLayout() || this.collapsed){
this.deferLayout = this.deferLayout || !shallow;
if(!forceLayout){
return;
}
shallow = shallow && !this.deferLayout;
} else {
delete this.deferLayout;
}
if(rendered && this.layout){
this.layout.layout();
}
if(shallow !== true && this.items){
var cs = this.items.items;
for(var i = 0, len = cs.length; i < len; i++){
var c = cs[i];
if(c.doLayout){
c.doLayout(false, forceLayout);
}
}
}
if(rendered){
this.onLayout(shallow, force);
}
delete this.forceLayout;
},

Animal
12 Aug 2009, 2:58 AM
But I still don't see why doLayout should be called at render time in a configureItem call.

Animal
12 Aug 2009, 3:00 AM
No. It should not be called there.

If you are adding Components to a deferredRender: false Container, then it is still necessary to call doLayout on it after adding. And it will lay itself out even if hidden.

pinecone_web
12 Aug 2009, 3:32 AM
Possibly, the code should be:



if(c.doLayout && this.forceLayout){
c.doLayout(false, true);
}


Evant,

Your modification makes the code respect the "forceLayout" flag, but my point is that the code should NOT appear there.

Animal,

Do you imply that the doLayout() was added in Ext.layout.ContainerLayout.configureItem() because of the bug in Ext.Container.doLayout() which ignores the "forceLayout" flag? Is there any other reason from the commit log?

Animal
12 Aug 2009, 4:30 AM
I think it was to do with ensuring that Components which are rendered in an initially-hidden state (eg Menu and Window) had their components layed out on first render.

But this has been fixed. So I think that code should not be there at all now.

pinecone_web
12 Aug 2009, 6:19 AM
Thx man. You guys are doing a great library, and service too.

pinecone_web
12 Aug 2009, 4:40 PM
By the way, are you going to release a new version soon? Because I think this may impact the performance of a complex container a lot.