Opened 11 years ago

Closed 7 years ago

#5950 closed enhancement (invalid)

[patch][cla] dojox.dtl : extending widgetInTemplate support

Reported by: guest Owned by: Neil Roberts
Priority: low Milestone: future
Component: DojoX DTL Version: 1.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

The attached patch is a proposal to realize what is described in http://www.dojotoolkit.org/forum/dojox-dojox/dojox-development-discussion/dojox-dtl-extending-widgetintemplate-support

As far as I know Ticket #5121 patch and Ticket #5949 patch are needed for full support.

Attachments (4)

dtlWidgetInTemplateDemo.patch (3.7 KB) - added by guest 11 years ago.
Demo update to demostrate enhancements. Provided by Alessandro Ferrari
dojoParserStartup.patch (1.6 KB) - added by guest 11 years ago.
Add to instantiate method of dojo.parser a second optional parameter that if is present and is false don't startup instances. Needed for delayed startup. Provided by Alessandro Ferrari
dtlWidgetInTemplate.patch (7.3 KB) - added by guest 11 years ago.
Provided by Alessandro Ferrari
dijit.js (7.8 KB) - added by guest 11 years ago.
Don't send a patch but full file so there's no problem with yours ongoing changes. Provided by Alessandro Ferrari

Download all attachments as: .zip

Change History (31)

Changed 11 years ago by guest

Demo update to demostrate enhancements. Provided by Alessandro Ferrari

comment:1 Changed 11 years ago by Adam Peller

Owner: changed from Adam Peller to Neil Roberts

comment:2 Changed 11 years ago by dylan

Milestone: 1.1

comment:3 in reply to:  description Changed 11 years ago by guest

Last dtlWidgetInTemplate.patch add optional second parameter "parsed"/"unparsed" or if not present autodetect correct behavior.
Fix an IE bug too.

Thanks,
Alessandro

comment:4 in reply to:  description Changed 11 years ago by guest

In IE6 cloneNode(true) don't clone content of script tags.
Last dtlWidgetInTemplate.patch version fix the problem.
Now demo fully tested on IE6, FF, Opera and Safari.

Changed 11 years ago by guest

Attachment: dojoParserStartup.patch added

Add to instantiate method of dojo.parser a second optional parameter that if is present and is false don't startup instances. Needed for delayed startup. Provided by Alessandro Ferrari

comment:5 in reply to:  description Changed 11 years ago by guest

Last patch's version has substantial improvements:

  • moved much code from DojoTypeNode? constructor to dojoType function to speedup cycles
  • in parsed mode on re-render destroy and re-create widget only if something is changed (big performance improvement)
  • on re-render if needed and supported call widget's reset method (needed for formwidgets)
  • delay startup of created widgets to end of render process so it's really "Called after a widget's children, and other widgets on the page, have been created."

Tested on IE6, FF, Opera and Safari.

Changed 11 years ago by guest

Attachment: dtlWidgetInTemplate.patch added

Provided by Alessandro Ferrari

comment:6 in reply to:  description Changed 11 years ago by guest

Last patch's version fix a sneaky bug on re-rendering of some widget-in-widget case.
Tested on IE6, FF, Opera and Safari.

comment:7 Changed 11 years ago by Neil Roberts

Resolution: fixed
Status: newclosed

(In [13126]) Fixes #5950.

  • Adds parsed keyword to widgetsInTemplate
  • Only rebuilds widget if underlying template changes
  • Thanks to Alessandro for noting various techniques in making this work
  • !strict

comment:8 in reply to:  7 Changed 11 years ago by guest

First of all thanks for making this part of official dojox.dtl

Looking at [13126] I noticed some different behavior:

  • no parsed auto-detection when not explicitly set
  • no reset() support (needed for textbox and other form widgets)
  • no delayed startup

I think some can be useful and better match dijit behavior (parsed auto-detection and delayed startup) without big performance issue and, more important, reset() support is needed if you want a correct form widgets support.

I'm testing [13126] in some quite complex case for an application I'm developing and will let you know if some issue arise.

Last note: #5121 is needed for full support under IE.

Sincerely,
Alessandro

comment:9 Changed 11 years ago by Neil Roberts

  • How were you hoping for parsed auto-detection to work?
  • I'll look into the reset stuff.
  • It does have delayed startup. At least it starts up as late as possible.
  • There are IE script fixes

comment:10 in reply to:  9 Changed 11 years ago by guest

Resolution: fixed
Status: closedreopened

Parsed auto-detection simply do

var div = document.createElement("div");
div.appendChild(node);
parsed = /{(%|{)/.test(div.innerHTML);
div.removeChild(node);

See dtlWidgetInTemplate.patch attachment.

Startup can be delayed to end of HtmlTemplate? rendering when all nodes and widgets are present. See dtlWidgetInTemplate.patch and dojoParserStartup.patch attachments.

Sorry, I didn't noticed IE script fixes. I think #5121 can be changed to fixed.

I'm testing the new code and I found a little bug in EventNode?.render:
line 90 must be changed from fn = fn[0]; to fn = fn.split(" ")[0];

More important I've found that a template with 2 parsed widgets, one inside the other, don't get inner widget instantiated.
This is because this._template is created with ddcd.widgetsInTemplate = false but the solution is not simply set it to true.

I'm actually trying to solve the problem and probably tomorrow I'll attach a new proposed patch to solve it, fix EventNode?.render, add auto-detection, reset support and delayed startup.

Sincerely,
Alessandro

comment:11 Changed 11 years ago by Neil Roberts

Auto-detection will ALWAYS be off. There are 2 extremely important reasons for it. The main reason is that it's EXTREMELY expensive to to do this widget construction. The second reason is that widgets determine how they're templated, and I can't "guess" the way they're templated. It's up to you. And even then, it's hard to guess what mixin they've used for templating.

I can see now why startup needs to be delayed. But I don't think the solution is changing the parser code. I'll make sure this gets in.

Thanks for the EventNode? stuff, I'll have a fix in.

The inner widget should get instantiated by the parser. ddcd.widgetsInTemplate = true is unrelated to a widget that has widgetsInTemplate = true.

comment:12 in reply to:  11 Changed 11 years ago by guest

Replying to pottedmeat:

Auto-detection will ALWAYS be off. There are 2 extremely important reasons for it. The main reason is that it's EXTREMELY expensive to to do this widget construction. The second reason is that widgets determine how they're templated, and I can't "guess" the way they're templated. It's up to you. And even then, it's hard to guess what mixin they've used for templating.

Using the example code in dojox.dtl.contrib.dijit.dojoType method only if second parameter parsed or unparsed is not declared is not so expensive (if a developer think is too expensive can declare using optional second parameter) and mimic behavior of normal dijit templates where no second parameter is needed. For second reason sorry but I can't understand what kind of mixin you are writing about.

The inner widget should get instantiated by the parser. ddcd.widgetsInTemplate = true is unrelated to a widget that has widgetsInTemplate = true.

An example can explain better what I mean
<div dojoType="acme.Widget parsed">{% for item in items %}<div dojoType="acme.Widget2 parsed">{{ item }}</div>{% endfor %}</div>
the problem is that acme.Widget2 aren't instantiated.

Sincerely,
Alessandro

comment:13 Changed 11 years ago by Neil Roberts

Okay, crossed wires. You're saying that auto-detection would only happen, if they _did_ have parsed set to true. Totally makes sense, and I'll add that to my updates.

I see what you're saying about the second part. I'll need to look into how the Dojo parser handles nested widgets. I'm wondering if it has anything to do with _Container/_Contained mixins.

Finally, it dawned on me tonight that I've already programmed it so that if a variable is passed with a certain API, it will be rendered into the template. Using this method, I can build a simple wrapper so that a widget can be assigned to a variable. This way, stuff like for loops would be much less expensive.

comment:14 in reply to:  13 ; Changed 11 years ago by guest

Replying to pottedmeat:

Okay, crossed wires. You're saying that auto-detection would only happen, if they _did_ have parsed set to true. Totally makes sense, and I'll add that to my updates.

Not exactly, what I mean is

dojoType: function(parser, text){
	if(ddcd.widgetsInTemplate){
		var node = parser.swallowNode();
		var parsed = false;
		if(text.slice(-7) == " parsed"){
			parsed = true;
			node.setAttribute("dojoType", dojo.trim(text).slice(0, -7));
		}else if(text.slice(-9) == " unparsed"){
			parsed = false;
			node.setAttribute("dojoType", dojo.trim(text).slice(0, -9));
		}else{
			var div = document.createElement("div");
			div.appendChild(node);
			parsed = /{(%|{)/.test(div.innerHTML);
			div.removeChild(node);
		}
		return new ddcd.DojoTypeNode(node, parsed);
	}
	return dd._noOpNode;
}

This mimic the behavior of dtl rendering on server a page that use dojo and dijit on client (browser): no second parameter needed.
Moving server template to client require no change but developer can optionally add "parsed" and "unparsed" optional parameter to force a behavior and for performance reason.

I see what you're saying about the second part. I'll need to look into how the Dojo parser handles nested widgets. I'm wondering if it has anything to do with _Container/_Contained mixins.

Even if we solve it we need to create a dtl DojoTypeNode? node for inner widget otherwise we have the following problem:

<div dojoAttachPoint="outerWidget" dojoType="acme.Widget1 parsed">
{% for item in items %}
<div dojoAttachPoint="innerWidgets" dojoType="acme.Widget2 parsed">{{ item }}</div>
{% endfor %}
</div>

If "wgt" is the instance of dojox.dtl._HtmlTemplated with the above template after rendering
wgt.outerWidget is a widget
but
wgt.innerWidgets is an array of domNode
and if acme.Widget2 contains dijit._Templated mixin domNodes in wgt.innerWidgets are detached from page.
Same problem for dojoAttachEvent.

Finally, it dawned on me tonight that I've already programmed it so that if a variable is passed with a certain API, it will be rendered into the template. Using this method, I can build a simple wrapper so that a widget can be assigned to a variable. This way, stuff like for loops would be much less expensive.

If I understand correctly it can give a big speedup in loops reusing widgets even if order is changed. Great improvement!

For reset() support just replace lines 161 and 162 with

	}else{
		if(this._dijit.reset){ this._dijit.reset(); }
	}
}else{
	if(this._dijit.reset){ this._dijit.reset(); }
}

An other useful improvement is add, just after above code

this._dijit.dtlContextThis = context.getThis();

so it's possible something like this

<div dojoAttachPoint="main_container" style="display:none;">
<label>Selected code:<div dojoAttachPoint="main" dojoType="dijit.form.TextBox"></div></label>
</div>
{% for item in items %}
<div dojoType="dijit.form.Button">{{ item.name }}
<script type='dojo/connect' event='onClick' args='e'>
  this.dtlContextThis.main.setValue("{{ item.code }}");
  this.dtlContextThis.main_container.style.display = "";
</script>
</div>
{% endfor %}

this is a very simple example but this improvement can make possible much more complex interactions (I'm currently using it in my application in quite complex situations).
It can be done with jsId but only if you use the template only once in the page and anyway you add unneeded dirt to global namespace.

Sincerely,
Alessandro

Changed 11 years ago by guest

Attachment: dijit.js added

Don't send a patch but full file so there's no problem with yours ongoing changes. Provided by Alessandro Ferrari

comment:15 Changed 11 years ago by guest

"dijit.js" attachment contains a new version of dojox/dtl/contrib/dijit.js with:

  • widget-in-widget fix (this is the most important)
  • fixed EventNode?.render
  • auto-detection
  • reset support
  • delayed startup

and some comments and questions in code.

It works for me.

Sincerely,
Alessandro

comment:16 in reply to:  14 ; Changed 11 years ago by Neil Roberts

This mimic the behavior of dtl rendering on server a page that use dojo and dijit on client (browser): no second parameter needed.

Adding the unparsed option to this makes a lot of sense, very nice!

Even if we solve it we need to create a dtl DojoTypeNode? node for inner widget otherwise we have the following problem:

Actually, the first time the parser encounters a dojoType attribute, it removes the node from the page and stops processing. When I run the parser on that node, it's run with widgetsInTemplate set to false, so that HTML is built as normal. There was a bug earlier where these nodes were still removed from DOM, but that's been fixed.

The one thing I do see as an issue here is if they've set "unparsed" on one of these inner widgets (eg if one of the inner widgets is DTL templated independently. I think that, in that case, I would make sure that the contents of that node aren't parsed, which should be farily easy.

An other useful improvement is add, just after above code

this._dijit.dtlContextThis = context.getThis();

I'd like to have this obey more of a _Container/_Contained API. I'll try to make sure that if the Dijit obeys the _Contained API that I set it up accordingly, and if it doesn't, to add the appropriate stuff so that this.getParent() works properly

comment:17 in reply to:  16 Changed 11 years ago by guest

Actually, the first time the parser encounters a dojoType attribute, it removes the node from the page and stops processing. When I run the parser on that node, it's run with widgetsInTemplate set to false, so that HTML is built as normal. There was a bug earlier where these nodes were still removed from DOM, but that's been fixed.

Rendering inner html as normal let the dojoAttachPoint/dojoAttachEvent problem open:
when in inner html there's a node with both dojoType and (dojoAttachPoint or dojoAttachEvent) the attach process is performed on domNode and not on widget.
An other issue is that this way we can't have widget reuse for inner widget.
I think both problems can be solved with widgetsInTemplate set to true only.

An other useful improvement is add, just after above code

this._dijit.dtlContextThis = context.getThis();

I'd like to have this obey more of a _Container/_Contained API. I'll try to make sure that if the Dijit obeys the _Contained API that I set it up accordingly, and if it doesn't, to add the appropriate stuff so that this.getParent() works properly

dtlContextThis has mainly an other purpose: write a big part of widgets interaction logic in template without having to define a new dojox.dtl._HtmlTemplated derived class for any template. Otherwise a widget in template can't interact with an other widget in the same template (other than parent/child) but can only "dojoAttachEvent" to a method that contains the logic.
This is a big limit in dojox.dtl._HtmlTemplated derived classed reuse.

Sincerely,
Alessando

comment:18 Changed 11 years ago by bill

Milestone: 1.21.3

bump enhancements to next milestone, as we prepare to close out 1.2

comment:19 Changed 10 years ago by bill

Description: modified (diff)
Milestone: 1.3future

comment:20 Changed 10 years ago by Adam Peller

Component: DojoxDojoX DTL

comment:21 Changed 10 years ago by Neil Roberts

Status: reopenednew

comment:22 Changed 10 years ago by Neil Roberts

Status: newassigned

comment:23 Changed 7 years ago by ben hockey

Resolution: patchwelcome
Status: assignedclosed

dtl is unsupported. any further patches are welcome.

comment:24 Changed 7 years ago by ben hockey

Priority: highlow

comment:25 Changed 7 years ago by bill

Description: modified (diff)
Resolution: patchwelcome
Status: closedreopened

Was this marked "patchwelcome" by mistake? It looks like there is a patch.

comment:26 Changed 7 years ago by ben hockey

bill - yeah i guess i didn't know what to do with this. the link in the description is 404 so i didn't know what this ticket was trying to achieve and i didn't spend any time looking to sort through which of these patches still needed to be applied or even if they could still be applied cleanly and nobody had done anything in 4 years so i thought i'd close it somehow. if you're willing to try to make sense of the state of this ticket then feel free to take it. if not, i guess we could assign it back to "guest" and ask what they believe to be the current state of this ticket and then let it close if we don't hear back.

comment:27 Changed 7 years ago by bill

Resolution: invalid
Status: reopenedclosed

Yeah, it's a mess. OK, I'll close it again and if anyone wants to they can just file a new ticket with a patch.

Note: See TracTickets for help on using tickets.