Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9985 closed task (fixed)

dijit._Contained._getSibling() code reduction

Reported by: Les Owned by: bill
Priority: low Milestone: 1.4
Component: Dijit Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

I didn't test it, but the code reduction is quite simple. The last three lines of _getSibling() can be reduced to a single line of code.

Current code:

_getSibling: function(which){
	// summary:
	//      Returns next or previous sibling
	// which:
	//      Either "next" or "previous"
	// tags:
	//      private
	var node = this.domNode;
	do{
		node = node[which+"Sibling"];
	}while(node && node.nodeType != 1);
	if(!node){ return null; } // null
	var id = node.getAttribute("widgetId");
	return dijit.byId(id);
}

New:

_getSibling: function(which){
	// summary:
	//      Returns next or previous sibling
	// which:
	//      Either "next" or "previous"
	// tags:
	//      private
	var node = this.domNode;
	do{
		node = node[which+"Sibling"];
	}while(node && node.nodeType != 1);
	return node ? dijit.byNode(node) : null;
}

Change History (18)

comment:1 Changed 10 years ago by bill

Milestone: tbd1.4
Owner: set to bill
Status: newassigned

Makes sense, return node && dijit.byNode(node); is even a few bytes shorter. I'll change that.

comment:2 Changed 10 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [20280]) dijit._Contained and dijit._Container code reduction from Les (CLA on file) and me.

Seems like there is still a lot of redundancy though between _Container (_getSiblingOfChild, _nextElement) and _Contained (_getSibling, _getPrevSibling, _getNextSibling). The only difference I see is that _Container._getSiblingOfChild() skips over non-Widget siblings; not sure if/why that code is needed. Also might make sense to just use dojo.query() for these methods.

Also updating API doc for return codes.

Fixes #9985, #9986 !strict.

comment:3 Changed 10 years ago by bill

(In [20282]) Removing more cruft from _Container, although leaving hasChildren() optimized for speed, rather than the shorter "return getChildren().length > 0;". Refs #9985 !strict.

comment:4 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

I'll reopen this ticket.

I see in two places in dijit._Container code like this:

dojo.forEach(this.getChildren()...

...but a shorter form could be used:

this.getChildren().forEach(...

Also, I'd suggest returning the widget in both removeChild() and addChild() to enable chaining.

removeChild: function(/*Widget or int*/ widget){
   ....
   return widget;
}

comment:5 Changed 10 years ago by Les

I think getParent() can also be simplified.

Current code:

getParent: function(){
	// summary:
	//		Returns the parent widget of this widget, assuming the parent
	//		implements dijit._Container
	for(var p=this.domNode.parentNode; p; p=p.parentNode){
		var id = p.getAttribute && p.getAttribute("widgetId");
		if(id){
			var parent = dijit.byId(id);
			return parent.isContainer ? parent : null;
		}
	}
	return null;
}

New:

getParent: function(){	
	var parent = dijit.getEnclosingWidget(this.domNode.parentNode);	
	return parent && parent.isContainer ? parent : null;		
}

comment:6 Changed 10 years ago by Les

Removing more cruft from _Container, although leaving hasChildren() optimized for speed.

So hasChildren() is optimized for speed, but is it correct? I don't see that hasChildren() looks for widgets in this.containerNode.

comment:7 Changed 10 years ago by Les

I'd use this for hasChildren();

hasChildren: function(){
	// summary:
	//		Returns true if widget has children, i.e. if this.containerNode contains widgets.
	return dojo.query("> [widgetId]", this.containerNode).length > 1;
}

comment:8 Changed 10 years ago by Les

Correction: length > 0

comment:9 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

this.getChildren().forEach(... won't work because getChildren() returns an array, and arrays don't have forEach() methods on all browsers.

As for hasChildren(), it's working as before. The trick to realize is that this.containerNode only contains widgets, it doesn't have plain DOM nodes. (This is part of the contract for _Container.) There's admittedly some weirdness because half of the code in _Container (like getChildren()) checks specifically for widgetId and half doesn't, will worry about that at a later time.

comment:10 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

this.getChildren().forEach(... won't work because getChildren() returns an array

Hmm... I don't understand this. I see that getChildren() returns a dojo.NodeList? b/c getChildren() uses dojo.query().map(), which returns NodeList?.

I tried this in IE6 (there's no [].forEach in IE6):
http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Tree.html

Run:

dijit.byId('dijit__TreeNode_7').getChildren().forEach

You will see that a function is returned.

What about the getParent() simplification? Do you like it?

comment:11 Changed 10 years ago by bill

I see that getChildren() returns a dojo.NodeList b/c getChildren() uses dojo.query().map(), which returns NodeList.

Ah OK I stand corrected. I was assuming it wasn't a NodeList since it isn't a list of nodes (but rather a list of widgets).

What about the getParent() simplification? Do you like it?

That looks good too, didn't notice that comment before.

About hasChildren(), I was thinking it needed to be performant but I just reviewed the dijit code base and only Tree calls that function, and just for keyboard navigation (ie, not during page load), so I guess we can use dojo.query(). I think it would be better if we could use the :first-child selector though.

comment:12 Changed 10 years ago by bill

Oh actually, I can't depend on getChildren() returning a NodeList after all, because that's not the spec of getChildren(), and thus (even though _Container.getChildren() return a NodeList), subclasses may not.

For example, BorderContainer.getChildren() and AccordionContainer.getChildren() do not return a NodeList. Even if those were fixed though, user defined subclasses might still be returning plain arrays.

For 2.0 we could of course consider changing the spec and returning an augmented array like the NodeList code does.

comment:13 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [20296]) More code size reductions, thanks Les (CLA on file), fixes #9985 again, !strict.

comment:14 Changed 10 years ago by Les

Resolution: fixed
Status: closedreopened

Bill, thank you. I think we want to return null if the widget can't be found. Otherwise, the return value will be undefined.

removeChild: function(/*Widget or int*/ widget){
		...

		// If we cannot find the widget, return null
		if(!widget || !widget.domNode){ return null; }
	   
		...
	   
		return widget;          // Widget
}

comment:15 Changed 10 years ago by bill

We aren't really strict about differentiating between null and undefined (or false and undefined) for most functions. For example, dijit.byId("foo") returns undefined, not null.

I do see a slight reduction though to get rid of that second return statement, which I'll make.

comment:16 Changed 10 years ago by bill

Resolution: fixed
Status: reopenedclosed

(In [20302]) More code size reductions, fixes #9985 again, !strict.

comment:17 Changed 10 years ago by bill

(In [20351]) Returning widget from _Container.addChild()/removeChild() is inconsistent with subclasses' addChild()/removeChild() which don't return anything. It's better to be consistent to avoid confusion from our users. Refs #9985 !strict.

comment:18 Changed 10 years ago by bill

Type: enhancementtask

seems like these aren't really enhancements from the users' point of view, labeling as tasks instead

Note: See TracTickets for help on using tickets.