You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@wicket.apache.org by Maarten Billemont <lh...@gmail.com> on 2011/03/08 12:11:48 UTC
[VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
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 IModel<String> name = Model.of();
public MyPanel(String id) {
add(new TextField<String>("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 TextField<String>("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 wicket code out there still uses constructors to create component hierarchies.
2. The safer onInitialize will always and consistently be used for any component hierarchy construction. This also requires the good code habits that you shouldn't be doing anything with your components outside of wicket lifecycle methods.
THE BAD
1. Mixing constructor and onInitialize usage, whether it's within one class or between different classes in your type and component hierarchy, is and will always be dodgy. You really shouldn't do it. Also, to future-proof your code, you should really already be moving your initialization code into onInitialize. Additionally, this introduces an API inconsistency: For pages, you cannot use onInitialize, for everything else, you should. That's not very intuitive. Additionally, it makes refactoring pages into panels and back a real pain (we've had to do this quite often already).
2. Checking for add() usage with a runtime exception is not very neat. You'll only notice your mistake at runtime (albeit, very soon. And during your migration to 1.5, you should expect more trouble than just this). Probably the biggest down-side is that a lot of people will need to move code around, from constructors into onInitialize (it can be argued that you really should be doing this anyway).
REFERENCES
The original discussion on the onInitialize issue with Pages:
http://mail-archives.apache.org/mod_mbox/wicket-dev/201012.mbox/%3C1291238699.1749.1408166705@webmail.messagingengine.com%3E
The JIRA issue:
https://issues.apache.org/jira/browse/WICKET-3218
Please refer to these if you're not entirely sure about the arguments of either side.
Cast your vote for the future shape of wicket's component hierarchy initialization API.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Carl-Eric Menzel <cm...@wicketbuch.de>.
On Tue, 8 Mar 2011 17:46:26 +0100
Maarten Billemont <lh...@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Gary Thomas <7z...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Gary Thomas <7z...@gmail.com>.
On 3/8/11 12:33 PM, Martijn Dashorst wrote:
> On Tue, Mar 8, 2011 at 6:03 PM, GOODWIN, MATTHEW (ATTCORP)
> <mg...@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Martijn Dashorst <ma...@gmail.com>.
On Tue, Mar 8, 2011 at 6:03 PM, GOODWIN, MATTHEW (ATTCORP)
<mg...@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by "GOODWIN, MATTHEW (ATTCORP)" <mg...@att.com>.
+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.lesmeister@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Hans Lesmeister 2 <ha...@lessy-software.de>.
+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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Eelco Hillenius <ee...@gmail.com>.
> 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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Bertrand Guay-Paquet <be...@step.polymtl.ca>.
On 08/03/2011 4:07 PM, Igor Vaynberg wrote:
> On Tue, Mar 8, 2011 at 11:42 AM, Maarten Billemont<lh...@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...
If I understand correctly, here is an example of what he means :
class A extends Panel {
public A() {
super("anID");
add(new Label("ALabel"));
}
// Hypothetic method called when a component is added
@Override
protected void onComponentAdd(Component component) {
// do stuff
}
}
class B extends A {
private final Label label;
public B() {
label = new Label("BLabel");
// etc.
}
@Override
protected void onComponentAdd(Component component) {
super.onComponentAdd(component);
// do stuff, accessing member label => NULL pointer exception
}
}
When an instance of B is created, the "B" part of the object is not
constructed yet and B#onComponentAdd() is called anyway.
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.
Regarding the problem of storing Page or Panel constructor parameters:
Would it be possible to add a factory method to create Panels and Pages
in Wicket like so :
public static <T extends Panel> T initPanel(T instance, PageParameters
parameters) {
instance.initialize(parameters); // new method used to add() components
instance.onInitialize();
}
This could be used like this :
setResponsePage(WicketClassSomewhere.initPanel(new MyPanel("id"),
parameters);
I'm not crazy about this construct, but I'm pitching it anyway to maybe
inspire someone else.
As an aside, I also just learned with this thread that I should not be
adding components to a Page or Panel in the constructor but rather do it
in onInitialize(). I based this design on examples, Wicket in Action and
also read the 1.5 migration notes. I must have missed that somewhere!
Regards,
Bertrand Guay-Paquet
>> 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-unsubscribe@wicket.apache.org
>> For additional commands, e-mail: users-help@wicket.apache.org
>>
>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Chris Colman <ch...@stepaheadsoftware.com>.
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:lhunath@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-unsubscribe@wicket.apache.org
>For additional commands, e-mail: users-help@wicket.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Scott Swank <sc...@gmail.com>.
+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
<Ch...@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:lhunath@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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by "Coleman, Chris" <Ch...@thalesgroup.com.au>.
+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:lhunath@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Igor Vaynberg <ig...@gmail.com>.
On Wed, Mar 9, 2011 at 11:54 AM, Maarten Billemont <lh...@gmail.com> wrote:
> On 09 Mar 2011, at 18:56, Igor Vaynberg wrote:
>>
>> On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont <lh...@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 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.
>
> You're making it sound a little black and white. You don't need unlimited memory and infinite bandwidth to support holding state and/or components in instance variables instead of casting results of get() operations on fragile strings. I think the grey zone is quite large, and while the exceptional case where you might want to optimize the maintainability out of your code would be nice to support, it really shouldn't be condoned as the standard way of doing things.
im not making things black or white, nor am i condoning the regular
use of get() to retrieve component instances. i am simply making sure
that we leave the door open for me to do so when necessary.
>>>> *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(); }}
>>
>> }
>
> I would if it weren't a "special cases where I'll have to forgive myself the bad design". And I think you're implying I could do this per default in all my pages.
yes, you can trivially create your own oninitialize() for pages, it is
what i am implying. and yes, it feels dirty because its special casing
something that you feel may not need to be special cased, i know the
feeling well. howeever, as framework designers we have to weigh
perfection against practicality. we need to evaluate whether
eliminating this special case will remove certain functionality that
people may need. its not an easy decision because it is not black and
white.
>>>> *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.
>>
>> it has to do with the fact that if i use a constructor and add those
>> components then i do not need the references. i can always get them
>> via get().
>>
>>>> * 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.
>>
>> we are working on enabling this. if NameEditor is a panel or a border
>> then its children should be able to access its markup before
>> NameEditor instance is added to the page since its markup is loaded
>> independently of its parents.
>
> In the ideal world, putting your component initialization code in your constructors has no issues or side-effects. You'd have access to your markup and your parent, you'd have a fully initialized object, and you wouldn't have to make the mental leap that post-constructing actually makes sense.
> Unfortunately, it seems we can't have that. Using constructors for component initialization does have issues:
> 1. You don't know your markup.
not an issue for a vast majority of components out there
> 2. You don't know your parent.
again, not an issue for a vast majority
> 3. Your subclasses haven't initialized yet.
this problem has existed as long as oo languages have. interestingly
in its long lifetime java has not provided us with a generic
post-construct callback, so perhaps the problem is not as big as you
make it out to be.
> And while it obviously also has advantages to be able to use the constructor, it doesn't make sense to blindly argue the advantages without proposing a way of dealing with the issues.
interesting because in the beginning of this thread you wanted to
blindly castrate constructors :) i have proposed a way to deal with
the issue. you can create your own one-time callback in the page from
inside onconfigure().
anyways, the reason i replied to this thread was to:
* play devil's advocate
* flush out the reasoning for both sides of the argument and give
other people reading this a deeper understanding
* arguing helps me think through the problem
in the end i dont think my mind has been changed much. if i were to
vote on this issue after your initial posting my vote would be
* -1 to forbid add() from constructors
* +0 to delay oninitialize() cascade until right before the first call
to onconfigure()
and it would remain the same now. my main reason for arguing was to
flush out any problems surrounding delaying the initial oninitialize()
cascade. i dont think it will cause too many problems, especially
considering pages are not the unit of reusability. possible problems
it may cause, however, is that now components will remain
uninitialized for longer and that may make more of their methods
unavailable post construction possibly making them harder to
configure. this is why my vote is +0 and not +1.
-igor
> I've proposed a way of dealing with the issues: Use onInitialize always. This also has issues, and I've proposed solutions for all of them. You'll find this thread documents them well, and in that regard, the onInitialize side of the argument has actually been worked out better. The disadvantages are clear and the way you should design your code to cope with them have been explained fully.
>
> You say you're working on a way of dealing with issue [1] for constructor-based component initialization. That'd be grand. But that leaves [2] and [3]. I can only think of one way to resolve [2]: In addition to taking an `String id` in components' super calls, also take a `MarkupContainer parent`. But that would make all component constructors even more verbose. And it still leaves [3] unresolved.
>
> That's ignoring the troubles you get when you use onInitialize and constructors in your Page. However, that can be documented in the JavaDocs, if need be: <b>If you add components in your constructor, don't use onInitialize</b>.
>
> I see the only recurring case for constructor-based initialization being: We shouldn't prohibit those that want to do it that way from doing it that way. What about the fact that you're prohibiting me from using onInitialize in pages?
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
On 09 Mar 2011, at 18:56, Igor Vaynberg wrote:
>
> On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont <lh...@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 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.
You're making it sound a little black and white. You don't need unlimited memory and infinite bandwidth to support holding state and/or components in instance variables instead of casting results of get() operations on fragile strings. I think the grey zone is quite large, and while the exceptional case where you might want to optimize the maintainability out of your code would be nice to support, it really shouldn't be condoned as the standard way of doing things.
>
>>> *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(); }}
>
> }
I would if it weren't a "special cases where I'll have to forgive myself the bad design". And I think you're implying I could do this per default in all my pages.
>
>>> *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.
>
> it has to do with the fact that if i use a constructor and add those
> components then i do not need the references. i can always get them
> via get().
>
>>> * 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.
>
> we are working on enabling this. if NameEditor is a panel or a border
> then its children should be able to access its markup before
> NameEditor instance is added to the page since its markup is loaded
> independently of its parents.
In the ideal world, putting your component initialization code in your constructors has no issues or side-effects. You'd have access to your markup and your parent, you'd have a fully initialized object, and you wouldn't have to make the mental leap that post-constructing actually makes sense.
Unfortunately, it seems we can't have that. Using constructors for component initialization does have issues:
1. You don't know your markup.
2. You don't know your parent.
3. Your subclasses haven't initialized yet.
And while it obviously also has advantages to be able to use the constructor, it doesn't make sense to blindly argue the advantages without proposing a way of dealing with the issues.
I've proposed a way of dealing with the issues: Use onInitialize always. This also has issues, and I've proposed solutions for all of them. You'll find this thread documents them well, and in that regard, the onInitialize side of the argument has actually been worked out better. The disadvantages are clear and the way you should design your code to cope with them have been explained fully.
You say you're working on a way of dealing with issue [1] for constructor-based component initialization. That'd be grand. But that leaves [2] and [3]. I can only think of one way to resolve [2]: In addition to taking an `String id` in components' super calls, also take a `MarkupContainer parent`. But that would make all component constructors even more verbose. And it still leaves [3] unresolved.
That's ignoring the troubles you get when you use onInitialize and constructors in your Page. However, that can be documented in the JavaDocs, if need be: <b>If you add components in your constructor, don't use onInitialize</b>.
I see the only recurring case for constructor-based initialization being: We shouldn't prohibit those that want to do it that way from doing it that way. What about the fact that you're prohibiting me from using onInitialize in pages?
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Igor Vaynberg <ig...@gmail.com>.
On Wed, Mar 9, 2011 at 1:15 AM, Maarten Billemont <lh...@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 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.
it has to do with the fact that if i use a constructor and add those
components then i do not need the references. i can always get them
via get().
>> * 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.
we are working on enabling this. if NameEditor is a panel or a border
then its children should be able to access its markup before
NameEditor instance is added to the page since its markup is loaded
independently of its parents.
-igor
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
> 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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Igor Vaynberg <ig...@gmail.com>.
On Wed, Mar 9, 2011 at 12:22 AM, Coleman, Chris
<Ch...@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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by "Coleman, Chris" <Ch...@thalesgroup.com.au>.
> 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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Igor Vaynberg <ig...@gmail.com>.
On Tue, Mar 8, 2011 at 11:22 PM, Maarten Billemont <lh...@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 {
> TextField<String> firstField;
> TextField<String> lastField;
> @Override protected void onInitialize() { super.onInitialize(); add(getFirst(), getLast()); }
> public textfield getFirst() { if (firstField == null) firstField = new TextField<String>(); return firstField; }
> public textfield getLast() { if (lastField == null) lastField = new TextField<String>(); 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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
>>>
>>> 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 {
TextField<String> firstField;
TextField<String> lastField;
@Override protected void onInitialize() { super.onInitialize(); add(getFirst(), getLast()); }
public textfield getFirst() { if (firstField == null) firstField = new TextField<String>(); return firstField; }
public textfield getLast() { if (lastField == null) lastField = new TextField<String>(); return lastField; }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Igor Vaynberg <ig...@gmail.com>.
On Tue, Mar 8, 2011 at 11:42 AM, Maarten Billemont <lh...@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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Carl-Eric Menzel <cm...@wicketbuch.de>.
On Tue, 8 Mar 2011 17:43:29 +0100
Maarten Billemont <lh...@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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Pedro Santos <pe...@gmail.com>.
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 <lh...@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 IModel<String> name = Model.of();
>
> public MyPanel(String id) {
>
> add(new TextField<String>("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 TextField<String>("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 wicket
> code out there still uses constructors to create component hierarchies.
>
> 2. The safer onInitialize will always and consistently be used for any
> component hierarchy construction. This also requires the good code habits
> that you shouldn't be doing anything with your components outside of wicket
> lifecycle methods.
>
>
> THE BAD
>
> 1. Mixing constructor and onInitialize usage, whether it's within one class
> or between different classes in your type and component hierarchy, is and
> will always be dodgy. You really shouldn't do it. Also, to future-proof
> your code, you should really already be moving your initialization code into
> onInitialize. Additionally, this introduces an API inconsistency: For
> pages, you cannot use onInitialize, for everything else, you should. That's
> not very intuitive. Additionally, it makes refactoring pages into panels
> and back a real pain (we've had to do this quite often already).
>
> 2. Checking for add() usage with a runtime exception is not very neat.
> You'll only notice your mistake at runtime (albeit, very soon. And during
> your migration to 1.5, you should expect more trouble than just this).
> Probably the biggest down-side is that a lot of people will need to move
> code around, from constructors into onInitialize (it can be argued that you
> really should be doing this anyway).
>
>
> REFERENCES
>
> The original discussion on the onInitialize issue with Pages:
>
> http://mail-archives.apache.org/mod_mbox/wicket-dev/201012.mbox/%3C1291238699.1749.1408166705@webmail.messagingengine.com%3E
>
> The JIRA issue:
> https://issues.apache.org/jira/browse/WICKET-3218
>
> Please refer to these if you're not entirely sure about the arguments of
> either side.
>
> Cast your vote for the future shape of wicket's component hierarchy
> initialization API.
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
--
Pedro Henrique Oliveira dos Santos
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Peter Ertl <pe...@gmx.org>.
-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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
RE: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Chris Colman <ch...@stepaheadsoftware.com>.
>> 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-unsubscribe@wicket.apache.org
>For additional commands, e-mail: users-help@wicket.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Maarten Billemont <lh...@gmail.com>.
> [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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Martijn Dashorst <ma...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Jim Pinkham <pi...@gmail.com>.
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 <zb...@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-unsubscribe@wicket.apache.org
> For additional commands, e-mail: users-help@wicket.apache.org
>
>
Re: [VOTE] WICKET-3218 - Component#onInitialize is broken for Pages
Posted by Zoltán Nagy <zb...@gmail.com>.
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-unsubscribe@wicket.apache.org
For additional commands, e-mail: users-help@wicket.apache.org