pipi7817 / mad-components

Automatically exported from code.google.com/p/mad-components
0 stars 0 forks source link

MadComponents 0.7.6 #36

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
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>=0; i--) {

                var child:DisplayObject = DisplayObject(item.getChildAt(i));

                if (child.hasOwnProperty("destructor")) {

                    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>=0; i--) {

                var child:DisplayObject = DisplayObject(item.getChildAt(i));

                if (child.hasOwnProperty("destructor")) {

                    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 < 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 < 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.

Original issue reported on code.google.com by neverbi...@gmail.com on 7 Aug 2012 at 2:27

GoogleCodeExporter commented 9 years ago
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.

Original comment by neverbi...@gmail.com on 7 Aug 2012 at 3:52

GoogleCodeExporter commented 9 years ago
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.

Original comment by neverbi...@gmail.com on 8 Aug 2012 at 12:41

GoogleCodeExporter commented 9 years ago
"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"

Original comment by neverbi...@gmail.com on 8 Aug 2012 at 12:43

GoogleCodeExporter commented 9 years ago
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...

Original comment by neverbi...@gmail.com on 13 Aug 2012 at 11:20

GoogleCodeExporter commented 9 years ago
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;

Original comment by neverbi...@gmail.com on 13 Aug 2012 at 11:23

GoogleCodeExporter commented 9 years ago

Original comment by doc.andr...@gmail.com on 3 Sep 2012 at 2:50

GoogleCodeExporter commented 9 years ago
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.

Original comment by neverbi...@gmail.com on 9 Sep 2012 at 11:45

GoogleCodeExporter commented 9 years ago
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?

Original comment by neverbi...@gmail.com on 9 Sep 2012 at 12:12