Export to GitHub

mad-components - issue #36

MadComponents 0.7.6


Posted on Aug 7, 2012 by Swift Kangaroo

Looking at latest version I've noticed several things that could be improved.

  • UIPages.as:

Why the change from:

                var page:DisplayObject = new UI.FormClass(this, child, newAttributes);

to

                var page:* = new UI.FormClass(this, child, newAttributes);

?

Is there any case in which it may not be a DisplayObject? Because if so, I see several places where it could fail, like attachPages().

Also:

                _pages[_pages.length] = page;

Is faster than:

                _pages.push(page);

You could always declare some:

var i:int = _pages.length;

Begore for each, and then change:

                _pages[_pages.length] = page;

Into

                _pages[i++] = page;

Also _pages could be changed from a simple Array to a Vector.<DisplayObject> or Vector.<IContainerUI>. I do not know which one would be more appropriate...

  • UILabel:

Missed the report from here?

https://code.google.com/p/mad-components/issues/detail?id=33&can=1

  • UI.as (and general):

    public static function clear(item:Sprite = null):void {
    
        if (!item) item = _root;
    
        for (var i:int = item.numChildren-1; i&gt;=0; i--) {
    
            var child:DisplayObject = DisplayObject(item.getChildAt(i));
    
            if (child.hasOwnProperty(&quot;destructor&quot;)) {
    
                Object(child).destructor();
    
            }
    
            item.removeChildAt(i);
    
        }
    
    }
    
    public static function clear(item:Sprite = null):void {
    
        if (!item) item = _root;
    
        for (var i:int = item.numChildren-1; i&gt;=0; i--) {
    
            var child:DisplayObject = DisplayObject(item.getChildAt(i));
    
            if (child.hasOwnProperty(&quot;destructor&quot;)) {
    
                Object(child).destructor();
    
            }
    
        }
    
        item.removeChildren();
    
    }
    

removeChildren() can be from 3 to 6 (or even more) times faster depending of the number of children. It may not be noticeable when there are just a few children, but it's better.

  • UIForm.as (and general)

        public function clear():void {
            for (var i:int = 0; i &lt; numChildren; i++) {
                var item:DisplayObject = getChildAt(i);
                if (item is IContainerUI) {
                    IContainerUI(item).clear();
                    IContainerUI(item).destructor();
                }
                removeChild(item);
            }
        }
    
    
        public function clear():void {
            for (var i:int = 0; i &lt; numChildren; i++) {
                var item:IContainerUI = getChildAt(i) as IContainerUI;
                if (item != null) {
                    item.clear();
                    item.destructor();
                }
            }
            removeChildren();
        }
    

Using as is often better that doing an explicit cast, the gain is ridiculous, but in this case I think it also looks better.

I may have missed more places where these can be applied. Also, sorry for all my issues with the ugly formatting, I don't know if there is a way for formatting code.

Comment #1

Posted on Aug 7, 2012 by Swift Kangaroo

Heh, the clear method from UIForm was from the previous version, sorry, maybe I swapped windows or something and thought it was the newer one.

Comment #2

Posted on Aug 8, 2012 by Swift Kangaroo

I made a patch with several changes:

https://gist.github.com/3294732

  • Added some of the changes commented above.
  • Commented some lines in layout as looking at the code it seems the behaviour is already applied several lines below (childAttributes.position(child, _inGroup && !_row);).
  • Fixed some issues with parseLabel. defaultTextFormat setter doesn't format already set text, and checking @height for fixWidth doesn't seem right.
  • I added an updateAfterEvent() call in page transitions... I use it for smoother transitions, but because of performance you may want to remove it.
  • Several changes to UIScrollVertical, first, why the addChildAt(0)? it causes the content to stay behind behind other components, for example background images. Also, changed width and height usage to widthH and heightV, it seems right, as it is also used in the UI and in my case it improved its usage inside other containers.
  • Added x, y, right and bottom properties when placing controls inside a frame container. This allows for anchoring, and placing controls in random positions without relying to code. Just using the UI XML.

I don't know if my changes cause any regression, I made a few tests and didn't see any. The app I'm working on saw several improvements thanks to these changes, like code reduction and dynamic placement of some controls without relying to more complex column or row layouts.

Comment #3

Posted on Aug 8, 2012 by Swift Kangaroo

"it seems right, as it is also used in the UI and" meant "it seems right, as it is also used in the constructor and"

Comment #4

Posted on Aug 13, 2012 by Swift Kangaroo

Here is another patch to apply over the previous one:

https://gist.github.com/3344754

  • Fixed some copy & paste errors.
  • Looking at the code it seems attributes.height and attributes.width should be used for the size of control containers, if it isn't so, it looks OK to me. Seeing widthH and heightH are read-only maybe it would be more logical the other way around tho. Changed UIList and UIGroupedList accordingly, made some tests, and some cases where previous code resulted in wrongly sized lists now look OK.
  • Changed how Attributes.parse() works, I think it is a bit clearer and consistent with the rest of the code this way, also, it fixes a problem with gapH and gapV in my previous patch.
  • There is a line in original MadComponents code that looks like a copy & paste error:

            _rendererAttributes.height=attributes.width - 2*attributes.paddingH;
    

I ran some regression tests, and didn't see any problem with my changes, but who knows...

Comment #5

Posted on Aug 13, 2012 by Swift Kangaroo

On the line that looks like a copy & paste error I changed:

  • _rendererAttributes.height=attributes.width - 2*attributes.paddingH;

To

  • _rendererAttributes.height=attributes.heightV - 2*attributes.paddingH;

It should be instead:

  • _rendererAttributes.height=attributes.heightV - 2*attributes.paddingV;

Comment #6

Posted on Sep 3, 2012 by Grumpy Bird

(No comment was entered for this change.)

Comment #7

Posted on Sep 9, 2012 by Swift Kangaroo

When using the bottom and right properties alone a null reference error can be got in parseBlock because it tries to access child and it hasn't be set yet... quite obvious.

It depends of where you use these properties, since not always that parseBlock part is accessed called.

Haven't decided how to fix it yet.

Comment #8

Posted on Sep 9, 2012 by Swift Kangaroo

Just to mention: Haven't decided how to fix it, but it may be possible to completely remove it. The layout function is always called at first after parseBlock, isn't it?

Status: Started

Labels:
Type-Defect Priority-Medium