I must agree with that - since onAttach is most often used to modify component's state (visibility, new children, ...) and not to attach any resources (as it was probably intended to) it should be refactored to a method which should be invoked after the listener phase and before the render phase. Users can create they own onAttach mehtod and call it if the need it. IModel also defines only detach() and no attach().
Resources (like loading of models) should be done lazy anyway, never in an attach method Why load everything if you maybe only need 10% of it?
this means that all code that is in onattach needs to move to onbeforerender > and onbeforerender needs to be relaxed to allow changes in the component > hierarchy, while onattach should not allow modification of hierarchy. Moreover, the new onBeforeRender should be invoked on all components that could be rendered, not only on visible components as it is now...
this would be the case of onBeforeRender would just become what onAttach is now. onAttach is now also just before render after the listener interface. We have 2 methods for it which are called right after each other. I don't see the big Pro for that. Finally I'd like to suggest one more refactorization: To make
Component#isVisible final becase overriding this method will - in most cases - silently break setVisible and can be IMHO quite harmful. Overridden isVisible could be always replaced by calling setVisible(...) in the new onBeforeRender method.
isVisible (and isEnabled) will never be made final. That will break many things. and sometimes isVisible must be completely dynamic. and can't be done by a setVisible method (which can only be called in the event phase) for example if i just do a refresh in the browser of the current page. and i have there a listview that shows it self depending on the size of the list that it has. This can be done in the isVisible method of that listview. I never can set this myself anywhere because i just rerendered the page. johan
