Opened 9 years ago

Closed 3 years ago

#11762 closed defect (patchwelcome)

Calling this.inherited (arguments) on the Expando results in currentSize not defined message.

Reported by: spidey2099 Owned by: dante
Priority: high Milestone: 1.13
Component: DojoX Layout Version: 1.5
Keywords: Cc:
Blocked By: Blocking:

Description

There is a use case in my application where a resize is called on an expando. When this occurs - an error message to the effect that currentSize is not defined occurs.

Looking at the code - it was found that the resize method has changed it's arguments!!! So now there is a second argument currentSize which must be provided.

And if this argument is not provided, the expando is not created completely and things like splitters become detached from it and the page in which the expando exists cannot be built.

Attachments (1)

ExpandoPane.js (7.3 KB) - added by spidey2099 9 years ago.
Fix for currentSize being null when children of the expando call resize()

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by spidey2099

P.S. Maybe I missed it but if a fundamental method's arguments change, shouldn't we wrap it with the original method and invoke the new method call from the old one. Basically we can use the wrapper pattern to be backward compatible.

comment:2 Changed 9 years ago by Adam Peller

Component: GeneralDojox
Owner: changed from anonymous to dante

are we talking about ExpandoPane? or the base classes? Aren't those arguments marked as optional?

comment:3 Changed 9 years ago by bill

Component: DojoxDojoX Layout

comment:4 in reply to:  2 Changed 9 years ago by spidey2099

Replying to peller:

are we talking about ExpandoPane? or the base classes? Aren't those arguments marked as optional?

Not as far as I can tell. Anyway here is the resize method from dojo/dojox/layout/ExpandoPane.js

*

resize: function(/* Object? */newSize, /*Object?*/ currentSize){

summary: we aren't a layout widget, but need to act like one: newSize: Object The size object to resize to currentSize: Object The size of my domNode that has already been set (by BorderCon?

tainer)

if(!this._hasSizes){ this._startupSizes(newSize); }

compute size of container (ie, size left over after title bar) this._contentBox = {

w: newSize && "w" in newSize ? newSize.w : currentSize.w, h: (newSize && "h" in newSize ? newSize.h : currentSize.h) - this._tit

leHeight

}; dojo.style(this.containerNode, "height", this._contentBox.h + "px");

if(newSize){

dojo.marginBox(this.domNode, newSize);

}

this._layoutChildren();

},

*

comment:5 Changed 9 years ago by spidey2099

Sorry for the split response. Here is the resize method from dojo/dojox/layout/ExpandoPane.js in release 1.4.2

 resize: function(/* Object? */psize){
                // summary: we aren't a layout widget, but need to act like one:
                //
                // psize: Object
                //              The size object optionally passed to us by our parent.

                if(!this._hasSizes){ this._startupSizes(psize); }

                // compute size of container (ie, size left over after title bar)
                // it looks like two marginBox calls, but sometimes psize comes in with only one member
                var     size = (psize && psize.h) ? psize : dojo.marginBox(this.domNode);
                this._contentBox = {
                        w: size.w || dojo.marginBox(this.domNode).w,
                        h: size.h - this._titleHeight
                };

                dojo.style(this.containerNode, "height", this._contentBox.h + "px");
                this._layoutChildren();
        },
Last edited 3 years ago by dylan (previous) (diff)

comment:6 Changed 9 years ago by Adam Peller

Given the ambiguity of our documentation format, it's hard to tell since it's there twice, but the "?" in Object? means it's optional. Where's the inherited call and how do you reproduce the problem?

comment:7 Changed 9 years ago by spidey2099

Okay - I see what you are saying. The call I am making is: this.inherited(arguments) However, I suspect that something is adding the first arg(newSize) to the arguments and the code looks for the newSize parameter and if present it will process both the newSize and the currentSize - which in this case is undefined.

Question is: if I pass an empty arguments list to the expando, will it resize correctly?

Changed 9 years ago by spidey2099

Attachment: ExpandoPane.js added

Fix for currentSize being null when children of the expando call resize()

comment:8 Changed 9 years ago by spidey2099

I have found that when children of the expando call resize, the currentSize can be undefined. The code is not defensive about this: it assumes that currentSize will always be there. I have tried passing in an empty arguments list and the method does not handle it.

So I added a defensive measure to protect against currentSize being undefined:

		// compute size of container (ie, size left over after title bar)
		if (currentSize === undefined || currentSize === null) {
			currentSize =  dojo.marginBox(this.domNode);
		}

The complete Expando.js with the change is attached.

This change works and the problem is fixed in my application. Please let me know what you think.

comment:9 Changed 9 years ago by bill

Since ExpandoPane is always a child of BorderContainer, it's assuming that it's always passed in a non-null currentSize parameter. I'll update the contradictory documentation. FWIW this currentSize stuff is more trouble than it's worth and will be removed in the future.

I don't understand what you wrote about about "when children of the expando call resize()". Is ExpandoPane's child a ContentPane? And the ContentPane is calling resize on it's children? And that's when the problem happens?

comment:10 Changed 9 years ago by bill

(In [22900]) Fix contradictory documentation about currentSize being optional/not optional, and stop using ? on newSize to indicate "possibly null". Refs #11762 !strict.

comment:11 Changed 3 years ago by dylan

Milestone: tbd1.12
Resolution: patchwelcome
Status: newclosed

If there's interest in making a change to this, it needs an updated pull request, see our contributing guidelines at https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md

I'm marking this as patchwelcome for now, given that it's been 5 years.

Note: See TracTickets for help on using tickets.