RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
So to summarize my rant: -1 for removing the ability to use add inside a constructor. +0 for improving the handling of oninitialize +1 for improving the documentation on the lifecycle of components and the event chain called during processing [2] I assume that means you don't care if 3218 is marked as won't fix and onInitialize remains overridable by those that choose to use it. It depends if the current code calls onInitialize as a side effect of calling page#add. If so then it would be good to change that so that onInitialize is called by the framework after page contruction has completed. The operation of onInitialize would then be nice a 'pure': that is, it is never invoked during construction - even if someone calls page#add in a constructor. Documentation is a good enough alternative when there is an unresolved issue that only occurs in rare cases. So yes, document it, and let those that want to use onInitialize do so. I never claimed using constructors will make your webapps eat your young. I simply outlined the pros and cons of each approach and argued the design advantages of not touching your components from outside wicket lifecycle methods. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 10 Mar 2011, at 00:42, Igor Vaynberg wrote: i am +0 for refactoring the code so that oninitialize() cascade is not started from page#add() but from somewhere before page#onconfigure(). this way oninitialize() is safe to override in pages because it will not be invoked from page's constructor but from some framework code at a later time. Then I think that's the only actual solution that has positive votes. Without any further feedback, can we agree to leave constructors as they are, make onInitialize overridable and do it this way? - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Oh the horror I have been through the last 6 years where I could randomly call add() inside my page's and panel's constructor. Over 1500 hand crafted pages by about 4 hands of developers and they are all ready for anti-depressants because of the troubles the component constructor has caused. Yes, things are really bad when you execute code in a constructor. Seriously, in my 6+ years of developing Wicket applications, adding components to the component hierarchy, I have yet to encounter the problems you say are so detrimental to developing with Wicket. Similarly calling methods from constructors that may be overridden. Those things are easily discovered with a debugger. At my company I have not encountered anything bad happening because of this. Perhaps in 5 years one or two days in total lost on a team with 25 developers. whooptidoo. not. a. big. deal. Using Subversion, Eclipse, Hibernate, Maven and Spring has caused much more lost hours than any problem you mentioned. There are just a couple of components, or rather models that require the component path in order to work properly. However those are typically better rewritten to evaluate their stuff at render time rather than construction time. If that doesn't work, those usually were better added in onBeforeRender, or more recently onInitialize or onConfigure. Given that in all my Wicket time I have not encountered unsurmountable problems with using constructors for what they are intended for, I will not be voting favorable for something that will throw away 6 years of code, libraries, books, blogs, articles and presentations, just because one person dislikes constructors. If I wanted such an experience, I'd used Tapestry instead. Yes, we break compatibility between releases. We do this to improve our framework and the way applications can be built. We strive to minimize compatibility breaks and try to keep upgrades of medium sized applications to a couple of days. We learnt our valuable lesson with the constructor change—and we're not about to repeat that fiasco again [1]. We try to balance the improvements we can achieve with the pain it will cause for upgrading between versions. In this instance, the pain is huge and community wide. The benefits are slim at best, as evidenced by 6 years of successful application development. The balance in this issue is severely off. So to summarize my rant: -1 for removing the ability to use add inside a constructor. +0 for improving the handling of oninitialize +1 for improving the documentation on the lifecycle of components and the event chain called during processing [2] Martijn [1] we had to rewrite Wicket in Action twice, and still discovered old 2.0 code in the examples days before going to the press [2] I'll have a go at that soonish if no-one beats me to the punch - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
[drama] So to summarize my rant: -1 for removing the ability to use add inside a constructor. +0 for improving the handling of oninitialize +1 for improving the documentation on the lifecycle of components and the event chain called during processing [2] I assume that means you don't care if 3218 is marked as won't fix and onInitialize remains overridable by those that choose to use it. Documentation is a good enough alternative when there is an unresolved issue that only occurs in rare cases. So yes, document it, and let those that want to use onInitialize do so. I never claimed using constructors will make your webapps eat your young. I simply outlined the pros and cons of each approach and argued the design advantages of not touching your components from outside wicket lifecycle methods. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
-1 on removing add() from ctor, too it will piss of a huge load of wicket developers for a tiny and questionable benefit. things like this made me drop tapestry btw (thanks for the really big laugh, martijn :-) we don't want users to leave because we make decisions from within the ivory tower. Am 10.03.2011 um 14:07 schrieb Maarten Billemont: [drama] So to summarize my rant: -1 for removing the ability to use add inside a constructor. +0 for improving the handling of oninitialize +1 for improving the documentation on the lifecycle of components and the event chain called during processing [2] I assume that means you don't care if 3218 is marked as won't fix and onInitialize remains overridable by those that choose to use it. Documentation is a good enough alternative when there is an unresolved issue that only occurs in rare cases. So yes, document it, and let those that want to use onInitialize do so. I never claimed using constructors will make your webapps eat your young. I simply outlined the pros and cons of each approach and argued the design advantages of not touching your components from outside wicket lifecycle methods. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
+1 for the approach you mention where onInitialize is called from the framework and not as a surprise side effect of calling page#add(). From: Maarten Billemont [mailto:lhun...@gmail.com] On 10 Mar 2011, at 00:42, Igor Vaynberg wrote: i am +0 for refactoring the code so that oninitialize() cascade is not started from page#add() but from somewhere before page#onconfigure(). this way oninitialize() is safe to override in pages because it will not be invoked from page's constructor but from some framework code at a later time. Then I think that's the only actual solution that has positive votes. Without any further feedback, can we agree to leave constructors as they are, make onInitialize overridable and do it this way? - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org DISCLAIMER:--- This e-mail transmission and any documents, files and previous e-mail messages attached to it are private and confidential. They may contain proprietary or copyright material or information that is subject to legal professional privilege. They are for the use of the intended recipient only. Any unauthorised viewing, use, disclosure, copying, alteration, storage or distribution of, or reliance on, this message is strictly prohibited. No part may be reproduced, adapted or transmitted without the written permission of the owner. If you have received this transmission in error, or are not an authorised recipient, please immediately notify the sender by return email, delete this message and all copies from your e-mail system, and destroy any printed copies. Receipt by anyone other than the intended recipient should not be deemed a waiver of any privilege or protection. Thales Australia does not warrant or represent that this e-mail or any documents, files and previous e-mail messages attached are error or virus free. -- - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
+1 for keeping calls to add() in the Component constructor. This makes upgrading as painless as possible. +1 also for having onInitialize() mimic: private boolean initialized = false; /// etc protected void onBeforeRender() { if (!initialized) { onInitialize(); initialized = true; } // other onBeforeRender stuff } Thank you all, Scott On Thu, Mar 10, 2011 at 2:54 PM, Coleman, Chris chris.cole...@thalesgroup.com.au wrote: +1 for the approach you mention where onInitialize is called from the framework and not as a surprise side effect of calling page#add(). From: Maarten Billemont [mailto:lhun...@gmail.com] On 10 Mar 2011, at 00:42, Igor Vaynberg wrote: i am +0 for refactoring the code so that oninitialize() cascade is not started from page#add() but from somewhere before page#onconfigure(). this way oninitialize() is safe to override in pages because it will not be invoked from page's constructor but from some framework code at a later time. Then I think that's the only actual solution that has positive votes. Without any further feedback, can we agree to leave constructors as they are, make onInitialize overridable and do it this way? - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org DISCLAIMER:--- This e-mail transmission and any documents, files and previous e-mail messages attached to it are private and confidential. They may contain proprietary or copyright material or information that is subject to legal professional privilege. They are for the use of the intended recipient only. Any unauthorised viewing, use, disclosure, copying, alteration, storage or distribution of, or reliance on, this message is strictly prohibited. No part may be reproduced, adapted or transmitted without the written permission of the owner. If you have received this transmission in error, or are not an authorised recipient, please immediately notify the sender by return email, delete this message and all copies from your e-mail system, and destroy any printed copies. Receipt by anyone other than the intended recipient should not be deemed a waiver of any privilege or protection. Thales Australia does not warrant or represent that this e-mail or any documents, files and previous e-mail messages attached are error or virus free. -- - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Tue, Mar 8, 2011 at 11:22 PM, Maarten Billemont lhun...@gmail.com wrote: Just like EJBs, you should be careful about how much interaction you do beyond your object's scope within the constructor. Your component doesn't have a hierarchy, getPage() cannot be accessed, none of your subclass constructors have been invoked and therefore your instance is not properly initialized and ready for use. not really sure what you mean by subclass constructors or how they come into play when constructing an instance... If I understand correctly, here is an example of what he means : Exactly. If B extends A and I do something in A's constructor that goes beyond the scope of setting up A's instance variables, then I risk a side-effect that relies on the instance to be constructed at B's level as well. There's nothing in the JVM that prevents this and these bugs are very hard to see and debug. They should be avoided by good coding practices. yep, calling overridable methods from constructors is bad - you just made the case for making page.oninitialize() final... On 08 Mar 2011, at 22:07, Igor Vaynberg wrote: i think code like this should be possible: NameEditor e=new NameEditor(name, firstNameModel, lastNameModel); e.getFirstNameEditor().setRequired(true); e.getLastNameEditor().setEnabled(false); taking the previous example of a name editor, with constructor we have: class nameeditor extends panel { public nameeditor(...) { add(new textfield(first,..); add(new textifled(last, ...); } public textfield getfirst() { return get(first); } public textfield getlast() { return get(last); } } Firstly, it's my opinion that you really shouldn't be doing anything to components directly, especially not from outside your class. As for why, see Encapsulation and Law of Demeter. neither of the two apply because nameeditor explicitly exposes the two components as part of its contract via public getters. But if you really wanted to use this pattern, it really wouldn't have to be as cumbersome as you make it out to be. Also, I really don't like condoning get(String), it's nasty and very type-unfriendly. It also breaks as soon as you do any refactoring in your component hierarchy. yes, i traded off some refactorability for better memory footprint. imho a perfectly valid trade off when memory and space are costly. class NameEditor extends Panel { TextFieldString firstField; TextFieldString lastField; @Override protected void onInitialize() { super.onInitialize(); add(getFirst(), getLast()); } public textfield getFirst() { if (firstField == null) firstField = new TextFieldString(); return firstField; } public textfield getLast() { if (lastField == null) lastField = new TextFieldString(); return lastField; } } there are still problems with this: *you still have the two extra memory slots that are a complete waste here - component references *the lazy init code is cumbersome to write *you are missing the two slots needed to pass models from constructor to the two textfields - so thats four extra slots total that you have now forced on users * the components i get from the getters are not added to their parent - for more complex components that need access to their markup this is a problem, eg if (text.equals(getFirst().getMarkupTag().get(type))...not necessarily a good example taking this particular custom component as context, but a valid usecase in plenty of other situations. -igor - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
yep, calling overridable methods from constructors is bad - Yes I agree... you just made the case for making page.oninitialize() final... But isn't that the very thing that the whole overridable onInitialize method was intended to avoid as it gets called after construction by the framework. Any post construction code can simply be put in the onInitialize method. DISCLAIMER:--- This e-mail transmission and any documents, files and previous e-mail messages attached to it are private and confidential. They may contain proprietary or copyright material or information that is subject to legal professional privilege. They are for the use of the intended recipient only. Any unauthorised viewing, use, disclosure, copying, alteration, storage or distribution of, or reliance on, this message is strictly prohibited. No part may be reproduced, adapted or transmitted without the written permission of the owner. If you have received this transmission in error, or are not an authorised recipient, please immediately notify the sender by return email, delete this message and all copies from your e-mail system, and destroy any printed copies. Receipt by anyone other than the intended recipient should not be deemed a waiver of any privilege or protection. Thales Australia does not warrant or represent that this e-mail or any documents, files and previous e-mail messages attached are error or virus free. -- - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Wed, Mar 9, 2011 at 12:22 AM, Coleman, Chris chris.cole...@thalesgroup.com.au wrote: yep, calling overridable methods from constructors is bad - Yes I agree... you just made the case for making page.oninitialize() final... But isn't that the very thing that the whole overridable onInitialize method was intended to avoid as it gets called after construction by the framework. Any post construction code can simply be put in the onInitialize method. the way the code is implemented page's oninitialize() would get called on first page#add() invocation and since the add() invocation is most likely inside page's constructor... -igor DISCLAIMER:--- This e-mail transmission and any documents, files and previous e-mail messages attached to it are private and confidential. They may contain proprietary or copyright material or information that is subject to legal professional privilege. They are for the use of the intended recipient only. Any unauthorised viewing, use, disclosure, copying, alteration, storage or distribution of, or reliance on, this message is strictly prohibited. No part may be reproduced, adapted or transmitted without the written permission of the owner. If you have received this transmission in error, or are not an authorised recipient, please immediately notify the sender by return email, delete this message and all copies from your e-mail system, and destroy any printed copies. Receipt by anyone other than the intended recipient should not be deemed a waiver of any privilege or protection. Thales Australia does not warrant or represent that this e-mail or any documents, files and previous e-mail messages attached are error or virus free. -- - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
yep, calling overridable methods from constructors is bad - you just made the case for making page.oninitialize() final... No, I've made the case for this issue. Either add() should be disallowed from the constructor (meaning onInitialize won't get called from it), onInitialize should be made final (meaning onInitialize will do nothing when it gets called), or Page should stop calling initialize() as a result of a component being added, and behave like any other component does. Would you mind addressing this third option? What exactly is the reason for Page behaving in this special way? On 09 Mar 2011, at 09:15, Igor Vaynberg wrote: Firstly, it's my opinion that you really shouldn't be doing anything to components directly, especially not from outside your class. As for why, see Encapsulation and Law of Demeter. neither of the two apply because nameeditor explicitly exposes the two components as part of its contract via public getters. Allow me to remind you: More formally, the Law of Demeter for functions requires that a method M of an object O may only invoke the methods of the following kinds of objects: • O itself • M's parameters • any objects created/instantiated within M • O's direct component objects • a global variable, accessible by O, in the scope of M Making your components available via getters does not excuse anything. The list does not include invoke methods on any objects created by any objects created within M, for good reason. Also, I really don't like condoning get(String), it's nasty and very type-unfriendly. It also breaks as soon as you do any refactoring in your component hierarchy. yes, i traded off some refactorability for better memory footprint. imho a perfectly valid trade off when memory and space are costly. there are still problems with this: *you still have the two extra memory slots that are a complete waste here - component references You're talking about the cost of a reference to a component. It's not like you've got a COPY of the component in your field. And no, it's not a good trade-off. You're writing Java applications, you'll be deploying in Java application containers, you're working with a web framework that goes to great lengths to help the container provide solid clustering. Your memory footprint is absolutely subordinate to decently maintainable and compile-time checked code. If you're a developer that doesn't appreciate the latter, you'll find that you can be a lot more flexible using a language with weak type checking, at the cost of maintainable code. Any Java developer should not be willing to make this trade-off, especially when it's as light as this one. Your component gets added to your page anyway! *the lazy init code is cumbersome to write Hardly, but you shouldn't be doing it anyway except for special cases where you can forgive yourself the bad design. *you are missing the two slots needed to pass models from constructor to the two textfields - so thats four extra slots total that you have now forced on users You really shouldn't be having references to your components and have only two slots for your models, but if you want to provide getters for your components, yes, you need instance fields for them. Because get(String) is nasty. This really doesn't have anything to do with the difference between constructor and onInitialize initialization, except for the added bonus that you can't do nasty get(String) in the latter case before your component is initialized. The second option is looking better and better to me. * the components i get from the getters are not added to their parent - for more complex components that need access to their markup this is a problem, eg if (text.equals(getFirst().getMarkupTag().get(type))...not necessarily a good example taking this particular custom component as context, but a valid usecase in plenty of other situations. Irrelevant, even if you added them in your constructor, you don't have access to your markup before you add your newly created NameEditor to a page hierarchy. This example does help to confirm the need for an onInitialize: Don't touch your components until onInitialize is called indicating that the page hierarchy and markup are available. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 3/8/11 12:33 PM, Martijn Dashorst wrote: On Tue, Mar 8, 2011 at 6:03 PM, GOODWIN, MATTHEW (ATTCORP) mg0...@att.com wrote: +1 for clear documentation/Javadoc explaining proper use of onInitialize. Does this exist somewhere? As someone new to Wicket I'm trying to learn as fast as I can and a lot of examples (almost exclusively) I see out there show the add(..) from within the constructor - which is apparently an anti-pattern from the sound of this thread. It is most certainly not an antipattern in my book. I find the reaction of the anti-constructor folks too strong and am trying to formulate a civil reaction to this whole anti constructor rant. Martijn *Please* don't deprecate add() from constructor. While a minority use-case, this allows for very elegant Wicket code when using Scala: add(new Form[Person](form, model) { add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) etc. Since the body of a class is the constructor in Scala, this is a perfect fit for closure-like structures when using Wicket. Thanks, Gary - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 09 Mar 2011, at 10:44, Gary Thomas wrote: While a minority use-case, this allows for very elegant Wicket code when using Scala: add(new Form[Person](form, model) { add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) Style is great when there are no issues involved with doing it. If it turns out that adding components from the constructor is indeed dangerous when combined with other components (eg. in libraries) that use onInitialize then we should reconsider. I think the best solution so far would be to not invoke onInitialize when adding components to a page, that would allow the constructor to be used still if the developer really wants to. With regards to your Scala code, while I don't know any Scala whatsoever, wouldn't it be just as easy to do something like this: add(new Form[Person](form, model) { override def onInitialize: add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) Which would give you the added lazy component construction bonus that onInitialize provides, as well as your elegance. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont lhun...@gmail.com wrote: yep, calling overridable methods from constructors is bad - you just made the case for making page.oninitialize() final... No, I've made the case for this issue. Either add() should be disallowed from the constructor (meaning onInitialize won't get called from it), onInitialize should be made final (meaning onInitialize will do nothing when it gets called), or Page should stop calling initialize() as a result of a component being added, and behave like any other component does. Would you mind addressing this third option? What exactly is the reason for Page behaving in this special way? page.add() is where the oninitialize cascade is initiated because that is the earliest point at which the page instance is known. doing it from there ensures that the page is more or less fully constructed after it is instantiated allowing you to call methods on it. eg MyPage page=new MyPage(); page.foo() { where foo manipulates state of page's child components } i know that is not the style you prefer to use in your applications, but that doesnt mean no one should be able to use it. On 09 Mar 2011, at 09:15, Igor Vaynberg wrote: Firstly, it's my opinion that you really shouldn't be doing anything to components directly, especially not from outside your class. As for why, see Encapsulation and Law of Demeter. neither of the two apply because nameeditor explicitly exposes the two components as part of its contract via public getters. Allow me to remind you: More formally, the Law of Demeter for functions requires that a method M of an object O may only invoke the methods of the following kinds of objects: • O itself • M's parameters • any objects created/instantiated within M • O's direct component objects • a global variable, accessible by O, in the scope of M Making your components available via getters does not excuse anything. The list does not include invoke methods on any objects created by any objects created within M, for good reason. yes, and if we follow this good reason then i would have to add the following methods: nameditor { setfirstnamevisible() {} setlastnamevisible() {} setfirstnameenabled() {} setlastnameenabled() {} setfirstnamerequired() {} setlastnamerequired() {} addfirstnamevalidator() {} addlastnamevalidator() {} setfirstnamelabel() {} setlastnamelabel() {} } just to name a few, and all they do is forward stuff to the two inner components. sounds like a lot of fun... Also, I really don't like condoning get(String), it's nasty and very type-unfriendly. It also breaks as soon as you do any refactoring in your component hierarchy. yes, i traded off some refactorability for better memory footprint. imho a perfectly valid trade off when memory and space are costly. there are still problems with this: *you still have the two extra memory slots that are a complete waste here - component references You're talking about the cost of a reference to a component. It's not like you've got a COPY of the component in your field. And no, it's not a good trade-off. You're writing Java applications, you'll be deploying in Java application containers, you're working with a web framework that goes to great lengths to help the container provide solid clustering. Your memory footprint is absolutely subordinate to decently maintainable and compile-time checked code. If you're a developer that doesn't appreciate the latter, you'll find that you can be a lot more flexible using a language with weak type checking, at the cost of maintainable code. Any Java developer should not be willing to make this trade-off, especially when it's as light as this one. Your component gets added to your page anyway! in a perfect world where your servers have unlimited amounts of memory and your cluster's backplane is infinitely fast that would be true. apparently youve missed some recent threads on this list about people trying to optimize state. when you have a table with 100-200 rows and each row has 2-3 instances of such editors, all of a sudden those two extra slots add up. if you dont need to optimize your state thats great, but we should leave the door open for when you do. *the lazy init code is cumbersome to write Hardly, but you shouldn't be doing it anyway except for special cases where you can forgive yourself the bad design. well, in that case you shouldnt mind writing code like this: class mypage extends page { private boolean initialized=false; protected void onconfigure() { if (!initialized) { initialized=true; myOneTimeInitializationCode(); }} } *you are missing the two slots needed to pass models from constructor to the two textfields - so thats four extra slots total that you have now forced on users You really shouldn't be having references to your components and have only
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
I have a c++ background and this kind of problem is even more dangerous in c++ (virtual calls don't work as normal in constructors). In Java also, I think making this known to the outside world before it is fully constructed is unsafe, as illustrated above. It's a potential problem we've been aware of and consciously allowed to exist in Wicket for a long time after our failed attempt to address it on Wicket 2.0 (years ago). In the end I don't think it is a concrete very often, and the difficulties that get introduced trying to fix it are simply not worth it in our case. Eelco - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 3/9/11 2:18 AM, Maarten Billemont wrote: On 09 Mar 2011, at 10:44, Gary Thomas wrote: While a minority use-case, this allows for very elegant Wicket code when using Scala: add(new Form[Person](form, model) { add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) Style is great when there are no issues involved with doing it. If it turns out that adding components from the constructor is indeed dangerous when combined with other components (eg. in libraries) that use onInitialize then we should reconsider. I think the best solution so far would be to not invoke onInitialize when adding components to a page, that would allow the constructor to be used still if the developer really wants to. With regards to your Scala code, while I don't know any Scala whatsoever, wouldn't it be just as easy to do something like this: add(new Form[Person](form, model) { override def onInitialize: add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) Thanks for the reply - Yes it would be as simple as that - but again, not quite as elegant (imho). I admit my argument is pretty minor, but elegant code is a hallmark of an API or library that is designed well, and I think Wicket is great in that regard. Scala aside, the current usage of add() from the constructor just makes sense to me, since you really are constructing the component. Fully understood there are valid concerns about the side effects of this design, but would love to see that resolved transparently if at all possible. I'm not an expert on the internals, but am a very happy user of Wicket :-) Thanks, Gary - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 09 Mar 2011, at 19:15, Gary Thomas wrote: On 3/9/11 2:18 AM, Maarten Billemont wrote: On 09 Mar 2011, at 10:44, Gary Thomas wrote: While a minority use-case, this allows for very elegant Wicket code when using Scala: add(new Form[Person](form, model) { add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) Style is great when there are no issues involved with doing it. If it turns out that adding components from the constructor is indeed dangerous when combined with other components (eg. in libraries) that use onInitialize then we should reconsider. I think the best solution so far would be to not invoke onInitialize when adding components to a page, that would allow the constructor to be used still if the developer really wants to. With regards to your Scala code, while I don't know any Scala whatsoever, wouldn't it be just as easy to do something like this: add(new Form[Person](form, model) { override def onInitialize: add (new TextArea[String](text, (getModel - text)) { override def isVisible: Boolean = false }) }) Thanks for the reply - Yes it would be as simple as that - but again, not quite as elegant (imho). I actually think that it's more elegant to be explicit about the fact that your components are constructed lazily during initialization rather than object construction. Given the recent feedback, cede that forcing onInitialize on everybody may not be necessary, although I am still very convinced that it is the right thing to do design-wise. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 09 Mar 2011, at 18:56, Igor Vaynberg wrote: On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont lhun...@gmail.com wrote: yep, calling overridable methods from constructors is bad - you just made the case for making page.oninitialize() final... No, I've made the case for this issue. Either add() should be disallowed from the constructor (meaning onInitialize won't get called from it), onInitialize should be made final (meaning onInitialize will do nothing when it gets called), or Page should stop calling initialize() as a result of a component being added, and behave like any other component does. Would you mind addressing this third option? What exactly is the reason for Page behaving in this special way? page.add() is where the oninitialize cascade is initiated because that is the earliest point at which the page instance is known. doing it from there ensures that the page is more or less fully constructed after it is instantiated allowing you to call methods on it. Why should Page be a special case? Why not for Panels too? In fact, if you take away this special casing for Page, this issue probably wouldn't exist at all and we could go back to letting the constructor-favored users use constructors and the others use onInitialize. eg MyPage page=new MyPage(); page.foo() { where foo manipulates state of page's child components } i know that is not the style you prefer to use in your applications, but that doesnt mean no one should be able to use it. This issue is about the fact that this style, which I've argued is flawed, is basically causing you to finalize onInitialize, meaning no one will be able to use the alternative. It cuts both ways, and I really don't see why onInitialize should come out as the looser when it condones safer code. On 09 Mar 2011, at 09:15, Igor Vaynberg wrote: Firstly, it's my opinion that you really shouldn't be doing anything to components directly, especially not from outside your class. As for why, see Encapsulation and Law of Demeter. neither of the two apply because nameeditor explicitly exposes the two components as part of its contract via public getters. Allow me to remind you: More formally, the Law of Demeter for functions requires that a method M of an object O may only invoke the methods of the following kinds of objects: • O itself • M's parameters • any objects created/instantiated within M • O's direct component objects • a global variable, accessible by O, in the scope of M Making your components available via getters does not excuse anything. The list does not include invoke methods on any objects created by any objects created within M, for good reason. yes, and if we follow this good reason then i would have to add the following methods: nameditor { setfirstnamevisible() {} setlastnamevisible() {} setfirstnameenabled() {} setlastnameenabled() {} setfirstnamerequired() {} setlastnamerequired() {} addfirstnamevalidator() {} addlastnamevalidator() {} setfirstnamelabel() {} setlastnamelabel() {} } just to name a few, and all they do is forward stuff to the two inner components. sounds like a lot of fun... Actually, that would be a workaround to hack away a warning your IDE might give you about the Law of Demeter. The point of the practice is that outside of NameEditor, you shouldn't be doing anything that requires knowledge of how NameEditor works. Which means you shouldn't be exposing any of its components, and definitely none of its components' properties. If you want to do something to your NameEditor that changes the way it shows your name, you should express that in terms that make sense for a name editor. new NameEditor(NameEditorMode.FIRST_NAME_ONLY); Or whatever API makes sense to you. Just don't expose NameEditor's guts. Also, I really don't like condoning get(String), it's nasty and very type-unfriendly. It also breaks as soon as you do any refactoring in your component hierarchy. yes, i traded off some refactorability for better memory footprint. imho a perfectly valid trade off when memory and space are costly. there are still problems with this: *you still have the two extra memory slots that are a complete waste here - component references You're talking about the cost of a reference to a component. It's not like you've got a COPY of the component in your field. And no, it's not a good trade-off. You're writing Java applications, you'll be deploying in Java application containers, you're working with a web framework that goes to great lengths to help the container provide solid clustering. Your memory footprint is absolutely subordinate to decently maintainable and compile-time checked code. If you're a developer that doesn't appreciate the latter, you'll find that you can be a lot more flexible using a language with weak type checking, at the
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Wed, Mar 9, 2011 at 11:54 AM, Maarten Billemont lhun...@gmail.com wrote: On 09 Mar 2011, at 18:56, Igor Vaynberg wrote: On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont lhun...@gmail.com wrote: yep, calling overridable methods from constructors is bad - you just made the case for making page.oninitialize() final... No, I've made the case for this issue. Either add() should be disallowed from the constructor (meaning onInitialize won't get called from it), onInitialize should be made final (meaning onInitialize will do nothing when it gets called), or Page should stop calling initialize() as a result of a component being added, and behave like any other component does. Would you mind addressing this third option? What exactly is the reason for Page behaving in this special way? page.add() is where the oninitialize cascade is initiated because that is the earliest point at which the page instance is known. doing it from there ensures that the page is more or less fully constructed after it is instantiated allowing you to call methods on it. Why should Page be a special case? Why not for Panels too? oninitialize() is an atomic callback that notifies components when the page becomes available. the page is special because since a page is always available to itself it doesnt need such a callback. how is that for a rationalization... :) you are mixing a generic post-construct callback with a more specific usecase of oninitialize() In fact, if you take away this special casing for Page, this issue probably wouldn't exist at all and we could go back to letting the constructor-favored users use constructors and the others use onInitialize. that may very well be true. but weve come a long way from the beginning of this thread which was: forbid add() in constructors. eg MyPage page=new MyPage(); page.foo() { where foo manipulates state of page's child components } i know that is not the style you prefer to use in your applications, but that doesnt mean no one should be able to use it. This issue is about the fact that this style, which I've argued is flawed, is basically causing you to finalize onInitialize, meaning no one will be able to use the alternative. It cuts both ways, and I really don't see why onInitialize should come out as the looser when it condones safer code. On 09 Mar 2011, at 09:15, Igor Vaynberg wrote: Firstly, it's my opinion that you really shouldn't be doing anything to components directly, especially not from outside your class. As for why, see Encapsulation and Law of Demeter. neither of the two apply because nameeditor explicitly exposes the two components as part of its contract via public getters. Allow me to remind you: More formally, the Law of Demeter for functions requires that a method M of an object O may only invoke the methods of the following kinds of objects: • O itself • M's parameters • any objects created/instantiated within M • O's direct component objects • a global variable, accessible by O, in the scope of M Making your components available via getters does not excuse anything. The list does not include invoke methods on any objects created by any objects created within M, for good reason. yes, and if we follow this good reason then i would have to add the following methods: nameditor { setfirstnamevisible() {} setlastnamevisible() {} setfirstnameenabled() {} setlastnameenabled() {} setfirstnamerequired() {} setlastnamerequired() {} addfirstnamevalidator() {} addlastnamevalidator() {} setfirstnamelabel() {} setlastnamelabel() {} } just to name a few, and all they do is forward stuff to the two inner components. sounds like a lot of fun... Actually, that would be a workaround to hack away a warning your IDE might give you about the Law of Demeter. The point of the practice is that outside of NameEditor, you shouldn't be doing anything that requires knowledge of how NameEditor works. Which means you shouldn't be exposing any of its components, and definitely none of its components' properties. If you want to do something to your NameEditor that changes the way it shows your name, you should express that in terms that make sense for a name editor. new NameEditor(NameEditorMode.FIRST_NAME_ONLY); Or whatever API makes sense to you. Just don't expose NameEditor's guts. the two components are not considered guts. the nameeditor encapsulates the markup used to render the two components in my application, as well as a good set of defaults. however, it leaves room open for overriding those defaults. there is nothing wrong with this design. Also, I really don't like condoning get(String), it's nasty and very type-unfriendly. It also breaks as soon as you do any refactoring in your component hierarchy. yes, i traded off some refactorability for better memory footprint. imho a perfectly valid trade
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 09 Mar 2011, at 21:42, Igor Vaynberg wrote: oninitialize() is an atomic callback that notifies components when the page becomes available. the page is special because since a page is always available to itself it doesnt need such a callback. how is that for a rationalization... :) Only, you're special-casing initialize() in Pages so that onInitialize() gets called as soon as the page is available, only to then disallow developers from using onInitialize() by making it final. Kind of defeats the whole point, doesn't it. And I can't see initialize() doing anything else that the developer might notice. So really, just don't do it. In fact, if you take away this special casing for Page, this issue probably wouldn't exist at all and we could go back to letting the constructor-favored users use constructors and the others use onInitialize. that may very well be true. but weve come a long way from the beginning of this thread which was: forbid add() in constructors. We have come a long way, but if onInitialize didn't behave this way for pages, that would be an ideal solution to the issue that wouldn't force onInitialize or the constructor (or a lame onConfigure hack). interesting because in the beginning of this thread you wanted to blindly castrate constructors :) I explained the issue and proposed solutions. One of them involved not creating component hierarchies from the constructors. I wouldn't call that castrating: You still get to use them for what they're supposed to do: initialize your instance's state. i have proposed a way to deal with the issue. you can create your own one-time callback in the page from inside onconfigure(). So to round up, the proposed solutions are: 1. Forbid add() in constructor. 2. Forbid onInitialize() in pages (and fake one with onConfigure code that really has nothing to do with configuring but is onInitialize code in a block that requires a test of an instance field that takes up a whole boolean field in your page state, and do that for each of your pages). 3. Don't initialize() on add() in pages. * -1 to forbid add() from constructors * +0 to delay oninitialize() cascade until right before the first call to onconfigure() So that's [1] and [3] out the window, does that mean you vote +1 for [2]? Because as I mentioned before, you can't in all honesty argue the case of leaving the door open for add() in constructors if the developer want to do that and slam the door shut for add() in onInitialize() if the developer wants to do that. Perhaps we can add a [4]: 4. Allow both and ignore the fact that mixing them will break things in pages. Heck, if we can allow doing potentially bad things in constructors because the bad is rare anyway, why not, right? My vote, in order of preference, would be [1], [3], [4], [2]. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Just a thought: Surely there must be a way to support both styles of 'construction' (I use that term loosely) within the one framework. Perhaps a runtime switch in the properties could dictate the 'construction' mode. You could leave the default as 'traditional wicket' mode but in 'two phase' construction mode a clear two phase constructor/init strategy is used whereby init is called only after parent object construction is complete so that all virtual method calls will be made from within the init method and so guatrateed to be operating on a 'fully constructed object' instead of the current situation where it can be operating on a partically constructed parent object - nasty! (I admit it is more dangerous in languages like C++ but it can still cause significant problems in Java). In this two phase mode exceptions could be thrown if an attempt is made to do something which is dangerous or would go against the strategy. That means everybody used to other frameworks where a separate 'init' phase takes place after parent object construction is complete will be happy because they can switch to 'two phase' mode but at the same time all existing wicket code will continue to work unchanged. -Original Message- From: Maarten Billemont [mailto:lhun...@gmail.com] Sent: Thursday, 10 March 2011 8:28 AM To: Igor Vaynberg Cc: users@wicket.apache.org Subject: Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages On 09 Mar 2011, at 21:42, Igor Vaynberg wrote: oninitialize() is an atomic callback that notifies components when the page becomes available. the page is special because since a page is always available to itself it doesnt need such a callback. how is that for a rationalization... :) Only, you're special-casing initialize() in Pages so that onInitialize() gets called as soon as the page is available, only to then disallow developers from using onInitialize() by making it final. Kind of defeats the whole point, doesn't it. And I can't see initialize() doing anything else that the developer might notice. So really, just don't do it. In fact, if you take away this special casing for Page, this issue probably wouldn't exist at all and we could go back to letting the constructor-favored users use constructors and the others use onInitialize. that may very well be true. but weve come a long way from the beginning of this thread which was: forbid add() in constructors. We have come a long way, but if onInitialize didn't behave this way for pages, that would be an ideal solution to the issue that wouldn't force onInitialize or the constructor (or a lame onConfigure hack). interesting because in the beginning of this thread you wanted to blindly castrate constructors :) I explained the issue and proposed solutions. One of them involved not creating component hierarchies from the constructors. I wouldn't call that castrating: You still get to use them for what they're supposed to do: initialize your instance's state. i have proposed a way to deal with the issue. you can create your own one-time callback in the page from inside onconfigure(). So to round up, the proposed solutions are: 1. Forbid add() in constructor. 2. Forbid onInitialize() in pages (and fake one with onConfigure code that really has nothing to do with configuring but is onInitialize code in a block that requires a test of an instance field that takes up a whole boolean field in your page state, and do that for each of your pages). 3. Don't initialize() on add() in pages. * -1 to forbid add() from constructors * +0 to delay oninitialize() cascade until right before the first call to onconfigure() So that's [1] and [3] out the window, does that mean you vote +1 for [2]? Because as I mentioned before, you can't in all honesty argue the case of leaving the door open for add() in constructors if the developer want to do that and slam the door shut for add() in onInitialize() if the developer wants to do that. Perhaps we can add a [4]: 4. Allow both and ignore the fact that mixing them will break things in pages. Heck, if we can allow doing potentially bad things in constructors because the bad is rare anyway, why not, right? My vote, in order of preference, would be [1], [3], [4], [2]. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 09 Mar 2011, at 23:31, Chris Colman wrote: Surely there must be a way to support both styles of 'construction' (I use that term loosely) within the one framework. My proposed [3] and [4] already does that without an external configuration option that would confuse things about as much as JSF's faces-config.xml and all that good stuff. But sure, you could implement [1] and allow the developer to turn off the RTE from an Application setting. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Perhaps one question: Why is onInitialize invoked immediately after a component is added to a page? Page#componentAdded(final Component component) (line 1603 in 1.4.15). Is it really necessary to initialize the page here, and if so, why? The only possibly useful thing I can see it doing is fireComponentInitializationListeners, but why can't that just happen later, before the first render? - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Interesting problem. It helped me visualize the issue when I tried to suggest: myPage() { add(new Thing(this,...)); // 'this' doesn't exist yet } I think building component hierarchy from constructors feels natural and it's going to be a hard sell to change to support this use case where some components might need to know their parents. I thought I was keeping pretty much up-to-speed on 1.5 changes, but I'm kind of surprised this is the first I'm noticing this fairly substantial API change. Sorry if something like this has already been shot down - I didn't look - but just off the top of my head, seems like there could be a way to have the framework (as it is about to add a page to it's map or at some other convenient early point) go back down thru the hierarchy and visit all the components to offer them a parent reference? Maybe guarantee it would happen before the first onBeforeRender or something like that? Then, we could let these visitors 'vote' on when the hierarchy is finished changing - (i.e. components could use this information to make additional changes to the hierarchy) by returning a value. Perhaps call this new component visitor something like int onParentChanged(Component parent) - return flags that would let the framework interrupt and restart, repeat after finish, or just continue visiting (or maybe just return boolean repeat flag). -- Jim Pinkham TogetherAuction.com On Tue, Mar 8, 2011 at 8:33 AM, Zoltán Nagy zberke...@gmail.com wrote: Hi! My idea is this: Somewhere in the future (for example in wicket 1.6) adding copmonents from the constructor will be prohibited, but till than it should be allowed. -- Zoltán Nagy - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
I vote for solution 3: postpone the onInitialize call, possible to the first Component#configure execution. Then the problem of initialization code being executed for not fully constructed components go away. If it is really important to have a callback method for path cognizant components, than the API can be improved adding one more method like the awt Component#addNotify, but with one change: this method needs to be called again if the component is remove / re added and it will not be a good place to add children. - 1 for solution 2, it is up to user decide how to code objects, the related problems in this thread can be addressed in different way, like I voted before. On Tue, Mar 8, 2011 at 8:11 AM, Maarten Billemont lhun...@gmail.com wrote: Dear fellow wicket users, Following is a call for your opinion and a vote on how Wicket should behave with regards to post-constructing components in 1.5. The final solution that is chosen will affect the way you construct your components and your component hierarchies. First, some background on what onInitialize is and why you should be using it. THE PROBLEM Historically, we've generally used our component constructors to build our component's hierarchy. We call add() to put new components in our custom component from our component's constructor: public class MyPanel extends Panel { private final IModelString name = Model.of(); public MyPanel(String id) { add(new TextFieldString(name, name)); } } This can cause problems, however, when initializing a component requires knowledge of the component's markup, or requires that the component's path in the hierarchy is known (during construction, the component is not yet added to any parent component). THE SOLUTION onInitialize was introduced to Component. This gives us a place to initialize our component's hierarchy after our component has been added to its parent. As a result, our component has a path to its page, making markup, parent and sibling components accessible. THE BAD The downside of initializing your component hierarchy in onInitialize as opposed to from the constructor, are: 1. You need to move all your component hierarchy initialization code into a separate method. Usually, this is trivial work (cut/paste), but it is work that needs to be done nonetheless. 2. You cannot put your components in final instance fields anymore. 3. You should never do anything with components from custom methods. You should only ever reference your components directly from overridden wicket methods, such as onConfigure, onBeforeRender, onSubmit, etc. A little more on #3: If you add custom methods to your component, eg. void makeRequired() { nameField.setRequired(true); } You allow things like: new MyPanel().makeRequired(); That would throw a NullPointerException, since nameField is no longer initialized in the constructor, but much later, before the component first gets rendered. I would argue, though, that any custom methods that touch components are to be avoided at all cost. Component behaviors should never be touched except from wicket lifecycle methods (you probably want onConfigure) or right after you construct it: add(new TextFieldString(name, name).setRequired(true)); If you need a custom method such as makeRequired, it shouldn't touch components, it should set state that gets used by onConfigure: void makeRequired() { this.required = true; } void onConfigure() { setRequired(required); } (Obviously, the example is rather silly..) Fast-forward to https://issues.apache.org/jira/browse/WICKET-3218, it seems onInitialize has a problem. THE PROBLEM With regard to pages, if you add a component to a page from the constructor, onInitialize is invoked immediately. As a result, onInitialize code is ran even though your instance hasn't been fully constructed yet. This is bad. The real issue here is that we can combine component hierarchy initialization with constructors and onInitialize in a single class. However, doing so causes dangerous situations. We really should do only one, not both at the same time. THE SOLUTIONS This is where we need your vote. Currently, two proposed solutions exist: 1. Make onInitialize final for all Pages. As a result, onInitialize will still get called when the page is constructed and components are added, but you won't be able to use it, avoiding the risk of you using it in a manner that requires your instance to be constructed. 2. Make adding components from the constructor illegal. This would have to be by means of a runtime exception thrown whenever you try to add components to the hierarchy from the constructor. THE GOOD 1. This will be least painful for existing code. In all components, you can still mix constructor an onInitialize use, in pages, you can't use onInitialize, but in all likelihood, the largest amount of existing
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
+1 for making Page.onInitialize() non-final. Since 1.4.12 we have migrated a lot of code moving construction of the component hierarchies to onInitialize. We are really very happy with the way it is now in 1.4. +1 for clear documentation/Javadoc explaining proper use of onInitialize. (Developers who don't want to follow that explanation do also not have to be protected agains themselves) -1 for throwing an exception if add(..) is called from within a constructor (but maybe logging a warning instead?) I would also not have a problem with an alternative onInitialize()-method for the Pages. - -- Regards, Hans http://cantaa.de -- View this message in context: http://apache-wicket.1842946.n4.nabble.com/VOTE-WICKET-3218-Component-onInitialize-is-broken-for-Pages-tp3341090p3341741.html Sent from the Users forum mailing list archive at Nabble.com. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 08 Mar 2011, at 16:39, Pedro Santos wrote: I vote for solution 3: postpone the onInitialize call, possible to the first Component#configure execution. Then the problem of initialization code being executed for not fully constructed components go away. I actually also thought of that, which is why I asked for the reason as to why initialize() is invoked as a result of components being added to a page. If it ends up not being necessary (delay the onInitialize to when it's called for any other component) then perhaps that's a fair middle ground. People can still use constructors if they like (though really, the only single *advantage* I can think of for this is that it means you don't have to go back and fix your existing codebase), and others are free to use onInitialize. Doing your work post-construct is a very common thing in Java. That's the whole point of there being a javax.annotation.PostConstruct, for example. Anyone that claims doing this is bad or counter-intuitive probably hasn't seen enough Java code yet. We're stuck with a legacy of how we used to do things in Wicket. And yes, that is a burden we should consider. But at some point, in favor of a clean API and consistency, you need to be able to give up legacy. Perhaps 1.5 is that time, perhaps it should be 1.6, but kid yourself not: it will be all that much harder the longer we wait. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 08 Mar 2011, at 17:38, Hans Lesmeister 2 wrote: +1 for making Page.onInitialize() non-final. Since 1.4.12 we have migrated a lot of code moving construction of the component hierarchies to onInitialize. We are really very happy with the way it is now in 1.4. -1 for throwing an exception if add(..) is called from within a constructor (but maybe logging a warning instead?) This is a valid vote if we can come up with a way of not having random failure side-effects from mixing the two (which is the whole reason the the issue exists in the first place), without a final onInitialize and an exception in add(). If such a solution is not suggested in this thread, we're gonna need either a final onInitialize or an exception in add(). You can't have your cake and eat it too. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Tue, 8 Mar 2011 17:43:29 +0100 Maarten Billemont lhun...@gmail.com wrote: On 08 Mar 2011, at 16:39, Pedro Santos wrote: I vote for solution 3: postpone the onInitialize call, possible to the first Component#configure execution. Then the problem of initialization code being executed for not fully constructed components go away. Fun fact: There is already a patch for that... I actually also thought of that, which is why I asked for the reason as to why initialize() is invoked as a result of components being added to a page. If it ends up not being necessary (delay the onInitialize to when it's called for any other component) then perhaps that's a fair middle ground. People can still use constructors if they like (though really, the only single *advantage* I can think of for this is that it means you don't have to go back and fix your existing codebase), and others are free to use onInitialize. I would like to use onInitialize for things that *need* to be done outside of the constructor. However, there are much better arguments than existing codebase for still allowing constructors. First, constructors are natural places to do stuff like that. Yes, they have limitations, and yes, PostConstruct exists for a reason, but that doesn't render constructors obsolete. Second, if you need to use any kind of parameters for stuff you do in onInitialize, you *must* store them in instance variables, even if you need them just once and could have long let them be garbage collected. Going purely post-construct would be a very bad idea for this reason. Carl-Eric www.wicketbuch.de - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Tue, 8 Mar 2011 17:46:26 +0100 Maarten Billemont lhun...@gmail.com wrote: This is a valid vote if we can come up with a way of not having random failure side-effects from mixing the two (which is the whole reason the the issue exists in the first place), without a final onInitialize and an exception in add(). If such a solution is not suggested in this thread, we're gonna need either a final onInitialize or an exception in add(). You can't have your cake and eat it too. I think there are good arguments for both constructors and onInitialize. But the developer needs to know what he/she is doing. I think that both options are useful and valid. This is certainly not a case of having the cake and eating it too. Constructor-only makes some initializations more difficult (though of course you can still use onBeforeRender or onConfigure, as we do now, after my patch had been rejected originally). onInitialize-only increases memory footprint and introduces conceptually unnecessary state by forcing you to carry *every* parameter via an instance field. Having both lets the developer choose the most appropriate strategy for the component at hand. Carl-Eric www.wicketbuch.de - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
+1 for clear documentation/Javadoc explaining proper use of onInitialize. Does this exist somewhere? As someone new to Wicket I'm trying to learn as fast as I can and a lot of examples (almost exclusively) I see out there show the add(..) from within the constructor - which is apparently an anti-pattern from the sound of this thread. Thanks, Matt -Original Message- From: Hans Lesmeister 2 [mailto:hans.lesmeis...@lessy-software.de] Sent: Tuesday, March 08, 2011 11:38 AM To: users@wicket.apache.org Subject: Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages +1 for making Page.onInitialize() non-final. Since 1.4.12 we have migrated a lot of code moving construction of the component hierarchies to onInitialize. We are really very happy with the way it is now in 1.4. +1 for clear documentation/Javadoc explaining proper use of onInitialize. (Developers who don't want to follow that explanation do also not have to be protected agains themselves) -1 for throwing an exception if add(..) is called from within a constructor (but maybe logging a warning instead?) I would also not have a problem with an alternative onInitialize()-method for the Pages. - -- Regards, Hans http://cantaa.de -- View this message in context: http://apache-wicket.1842946.n4.nabble.com/VOTE-WICKET-3218-Component-on Initialize-is-broken-for-Pages-tp3341090p3341741.html Sent from the Users forum mailing list archive at Nabble.com. - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On 08 Mar 2011, at 17:54, Carl-Eric Menzel wrote: However, there are much better arguments than existing codebase for still allowing constructors. First, constructors are natural places to do stuff like that. Yes, they have limitations, and yes, PostConstruct exists for a reason, but that doesn't render constructors obsolete. Your constructors are the perfect place to initialize your instance, at least at the level of your constructor's type. Build all your instance variables there, handle your page parameters, and that stuff. Just like EJBs, you should be careful about how much interaction you do beyond your object's scope within the constructor. Your component doesn't have a hierarchy, getPage() cannot be accessed, none of your subclass constructors have been invoked and therefore your instance is not properly initialized and ready for use. You really shouldn't be doing anything in your constructor that is NOT related to initializing your constructor type's instance variables. Touch anything else, invoke any methods, and you are opening pandora's box. It's important to understand *what* constructors are a natural place for. Second, if you need to use any kind of parameters for stuff you do in onInitialize, you *must* store them in instance variables, even if you need them just once and could have long let them be garbage collected. Going purely post-construct would be a very bad idea for this reason. Sorry? Parameters? You mean arguments to your components' constructor? Of course, but you really shouldn't ever need these in onInitialize if they're not state. And yes, your component's state belongs in its instance variables. If this is an issue, could you enlighten me? - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Tue, Mar 8, 2011 at 6:03 PM, GOODWIN, MATTHEW (ATTCORP) mg0...@att.com wrote: +1 for clear documentation/Javadoc explaining proper use of onInitialize. Does this exist somewhere? As someone new to Wicket I'm trying to learn as fast as I can and a lot of examples (almost exclusively) I see out there show the add(..) from within the constructor - which is apparently an anti-pattern from the sound of this thread. It is most certainly not an antipattern in my book. I find the reaction of the anti-constructor folks too strong and am trying to formulate a civil reaction to this whole anti constructor rant. Martijn - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
On Tue, Mar 8, 2011 at 11:42 AM, Maarten Billemont lhun...@gmail.com wrote: On 08 Mar 2011, at 17:54, Carl-Eric Menzel wrote: However, there are much better arguments than existing codebase for still allowing constructors. First, constructors are natural places to do stuff like that. Yes, they have limitations, and yes, PostConstruct exists for a reason, but that doesn't render constructors obsolete. Your constructors are the perfect place to initialize your instance, at least at the level of your constructor's type. Build all your instance variables there, handle your page parameters, and that stuff. Just like EJBs, you should be careful about how much interaction you do beyond your object's scope within the constructor. Your component doesn't have a hierarchy, getPage() cannot be accessed, none of your subclass constructors have been invoked and therefore your instance is not properly initialized and ready for use. not really sure what you mean by subclass constructors or how they come into play when constructing an instance... You really shouldn't be doing anything in your constructor that is NOT related to initializing your constructor type's instance variables. Touch anything else, invoke any methods, and you are opening pandora's box. It's important to understand *what* constructors are a natural place for. i think code like this should be possible: NameEditor e=new NameEditor(name, firstNameModel, lastNameModel); e.getFirstNameEditor().setRequired(true); e.getLastNameEditor().setEnabled(false); its simply the good citizen pattern where once you have an instance that instance is fully constructed and is ready to use. i think this applies to most components. i think there are much fewer components that need a page or their environment to work, and most of them are application-specific. wicket is all about reusability, and we should not hinder how easy and convenient it is to create and use reusable components. Second, if you need to use any kind of parameters for stuff you do in onInitialize, you *must* store them in instance variables, even if you need them just once and could have long let them be garbage collected. Going purely post-construct would be a very bad idea for this reason. Sorry? Parameters? You mean arguments to your components' constructor? Of course, but you really shouldn't ever need these in onInitialize if they're not state. And yes, your component's state belongs in its instance variables. If this is an issue, could you enlighten me? taking the previous example of a name editor, with constructor we have: class nameeditor extends panel { public nameeditor(...) { add(new textfield(first,..); add(new textifled(last, ...); } public textfield getfirst() { return get(first); } public textfield getlast() { return get(last); } } without constructing components i would have to do this: class nameeditor extends panel { private boolean firstNameRequired, lastNameRequired; private boolean firstNameEnabled, lastNameEnabled; protected void oninitialize() { add(new textfield(first,..); add(new textifled(last, ...); } protected void onconfigure() { first.setrequired(firstNameRequired).setEnabled(firstNameEnabled); last.setrequired(lastNameRequired).setEnabled(lastNameEnabled); } public void setFirstNameRequired(value) { firstNameRequired=value; } public void setLastNameRequired(value) { lastNameRequired=value; } public void setFirstNameEnabled(value) { firstNameEnabled=value; } public void setLastNameEnabled(value) { lastNameEnabled=value; } } notice the verbosity, and four extra and redundant slots that this component now takes up in memory and when serialized... -igor - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Just like EJBs, you should be careful about how much interaction you do beyond your object's scope within the constructor. Your component doesn't have a hierarchy, getPage() cannot be accessed, none of your subclass constructors have been invoked and therefore your instance is not properly initialized and ready for use. not really sure what you mean by subclass constructors or how they come into play when constructing an instance... If I understand correctly, here is an example of what he means : Exactly. If B extends A and I do something in A's constructor that goes beyond the scope of setting up A's instance variables, then I risk a side-effect that relies on the instance to be constructed at B's level as well. There's nothing in the JVM that prevents this and these bugs are very hard to see and debug. They should be avoided by good coding practices. On 08 Mar 2011, at 22:07, Igor Vaynberg wrote: i think code like this should be possible: NameEditor e=new NameEditor(name, firstNameModel, lastNameModel); e.getFirstNameEditor().setRequired(true); e.getLastNameEditor().setEnabled(false); taking the previous example of a name editor, with constructor we have: class nameeditor extends panel { public nameeditor(...) { add(new textfield(first,..); add(new textifled(last, ...); } public textfield getfirst() { return get(first); } public textfield getlast() { return get(last); } } Firstly, it's my opinion that you really shouldn't be doing anything to components directly, especially not from outside your class. As for why, see Encapsulation and Law of Demeter. But if you really wanted to use this pattern, it really wouldn't have to be as cumbersome as you make it out to be. Also, I really don't like condoning get(String), it's nasty and very type-unfriendly. It also breaks as soon as you do any refactoring in your component hierarchy. class NameEditor extends Panel { TextFieldString firstField; TextFieldString lastField; @Override protected void onInitialize() { super.onInitialize(); add(getFirst(), getLast()); } public textfield getFirst() { if (firstField == null) firstField = new TextFieldString(); return firstField; } public textfield getLast() { if (lastField == null) lastField = new TextFieldString(); return lastField; } } - To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org