On Thu, Sep 27, 2012 at 10:45 PM, Sven Meier <s...@meiers.net> wrote: >> this will make more advanced developer more unconfortable > > > I hear that ;). > > Jeremy gave a good suggestion: "set up some kind of code-check"
Well this is an option but again it makes more advanced developers not feeling comfortable. For example: recently I had to suppress such CheckStyle check for the usage of PageParameters#add() in our app because a colleague of mine earlier had a bug with getPageParameters().add() and use this parameters for the generation of a new Url. The side effect was that after that call the current page instance had few more parameters and this leaded to the bug. In my case I really wanted to use #add() because I wanted to add several values. A perfectly valid case. So now we have: https://issues.apache.org/jira/browse/WICKET-4774 Here is another suggestion for the archives :-) trait Advanced { self: Component => def setDefaultModel(model: IModel) { ... } .... } val myComponent = new Label(id, text) with Advanced myComponent.setDefaultModel(Model.of(advancedText)) without "with Advanced" you wont be able to see #setDefaultModel (assuming that it is removed from Component API) > > Sven > > > > On 09/27/2012 09:38 PM, Martin Grigorov wrote: >> >> On Thu, Sep 27, 2012 at 10:29 PM, Sven Meier <s...@meiers.net> wrote: >>>> >>>> lets take a concrete example of FormComponent ... your thinking we would >>>> have to make >>>> >>>> both setDefaultModel and setModel methods protected >>> >>> >>> Nope, I just ruminated about making setDefaultModel protected. >>> >>> >>>> a common subclass of FormComponent is FormComponentPanel which pretty >>>> much always distributes its model. >>> >>> >>> In my experience FormComponentPanels are often highly specialized >>> components >>> developed by senior developers. Perhaps there are a handful of these in a >>> project, most other cases are just nested panels. >>> >>> >>>> in the end, the developer has to know what the method >>>> does if they chose to call it. >>> >>> >>> Agreed. >>> >>> Just one little nitpick: In my scenario it's two developers, one who has >>> to >>> safeguard against another developer calling a method he isn't suppose to >>> call. >> >> I think Michael Mossman is in the same situation. >> And I understand what you see as an improvement that will protect the >> average developer. >> But I don't like that: >> - this change will lead to an inconsistent API - setDefaultModel() and >> seModel() are basically the same method. Just #setModel() has some >> help from the compiler >> - this will make more advanced developer more unconfortable - he has >> to extend the component just to make the method public to be able to >> use it for his needs. And sooner or later every average developer >> becomes advanced... >> >>> Sven >>> >>> >>> >>> On 09/27/2012 08:59 PM, Igor Vaynberg wrote: >>>> >>>> On Thu, Sep 27, 2012 at 10:49 AM, Sven Meier <s...@meiers.net> wrote: >>>>> >>>>> We're discussing the case where a component distributes its model to >>>>> its >>>>> children or behaviors. >>>>> For the component developer it's easier to assume that the model can't >>>>> be >>>>> changed without its consensus (i.e. by offering a generic #setModel()). >>>>> >>>>> Of course this can be handled perfectly by a coding guideline. I always >>>>> tell >>>>> people to *never* change a component's model. >>>>> I cannot count times where I'm called to a developer's IDE with him >>>>> having >>>>> absolutely no clue why something entered here doesn't display over >>>>> there: >>>>> In >>>>> many cases this is caused by setting models. >>>>> >>>>> But a protected #setDefaultModelObject() would make this explicit in >>>>> the >>>>> API. >>>> >>>> ok. lets start with a bit of history to have more context. >>>> setDefaultModel() only exists because of type-erasure. before wicket >>>> supported generics all components had a public setModel() method. so, >>>> one might say that having a public setModel() is "the wicket way" >>>> because it was there since 1.0. just to establish the baseline. >>>> >>>> lets take a concrete example of FormComponent. right now it has a >>>> public setModel() method, but by your thinking we would have to make >>>> both setDefaultModel and setModel methods protected, because we do not >>>> know that all FormComponents support changing the model. after all, a >>>> common subclass of FormComponent is FormComponentPanel which pretty >>>> much always distributes its model. so, we leave it to subclasses of >>>> FormComponent and FormComponentPanel to decide whether or not to >>>> override setModel() to make it public. >>>> >>>> a TextField would make its setModel() public - because it properly >>>> handles the usecase, correct? so it is still possible for your >>>> developers to call setModel() on a textfield and rewire it so it no >>>> longer links with a model of another component correctly. >>>> >>>> so we are now back to square one with the addition that a lot of >>>> components have to override setModel() just to change its visibility >>>> from protected to public - introducing a lot of noise. >>>> >>>> im all for making the code better, but i do not think that this change >>>> does. in the end, the developer has to know what the method does if >>>> they chose to call it. >>>> >>>> -igor >>>> >>>> >>>> >>>> >>>>> Sven >>>>> >>>>> >>>>> >>>>> On 09/27/2012 07:09 PM, Igor Vaynberg wrote: >>>>>> >>>>>> i thought the issue we were discussing here is the way the models are >>>>>> linked...which is not solved by making setdefaultmodelobject >>>>>> non-public. >>>>>> >>>>>> -igor >>>>>> >>>>>> On Thu, Sep 27, 2012 at 10:04 AM, Sven Meier <s...@meiers.net> wrote: >>>>>>> >>>>>>> If you extend GenericPanel you inherit setModel() and getModel(), >>>>>>> that's >>>>>>> the >>>>>>> whole purpose of this base class. >>>>>>> You want these two methods, otherwise you wouldn't extend it - >>>>>>> there's >>>>>>> nothing to fix. >>>>>>> >>>>>>> Component#setDefaultModel() is dangerous because it allows others to >>>>>>> tinker >>>>>>> with your component innards. >>>>>>> >>>>>>> I still think limiting access to #setDefaultModel() is a good idea, >>>>>>> but >>>>>>> this >>>>>>> is no crucial issue anyway. >>>>>>> >>>>>>> Sven >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 09/27/2012 06:16 PM, Michael Mosmann wrote: >>>>>>>> >>>>>>>> Am 27.09.2012 17:51, schrieb Igor Vaynberg: >>>>>>>> >>>>>>>> good point.. >>>>>>>> -1 from me.. thought it was a good idea, but wasn’t >>>>>>>> >>>>>>>> Michael >>>>>>>> >>>>>>>>> so what happens if panel A extends GenericPanel which has setModel? >>>>>>>>> you havent fixed anything. >>>>>>>>> >>>>>>>>> -igor >>>>>>>>> >>>>>>>>> On Thu, Sep 27, 2012 at 8:50 AM, Michael Mosmann >>>>>>>>> <mich...@mosmann.de> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Am 27.09.2012 17:32, schrieb Igor Vaynberg: >>>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> .. i would leave setModel as it is, only make this change for >>>>>>>>>> Component.setDefaultModel(). >>>>>>>>>> >>>>>>>>>> Michael >>>>>>>>>> >>>>>>>>>>> -1 on changing setDefaultModel(). >>>>>>>>>>> >>>>>>>>>>> 1) if B panel's model is truly dependent on A's then that >>>>>>>>>>> dependency >>>>>>>>>>> should be expressed: >>>>>>>>>>> >>>>>>>>>>> add(new BPanel("b", new PropertyModel(this, "defaultModel")); >>>>>>>>>>> >>>>>>>>>>> or do not use the default model slot of B to store the model. >>>>>>>>>>> that >>>>>>>>>>> way >>>>>>>>>>> setDefaultModel() calls on B will be a noop and you can choose >>>>>>>>>>> not >>>>>>>>>>> to >>>>>>>>>>> provide a setter. >>>>>>>>>>> >>>>>>>>>>> 2) you are only "solving" this for a subset of usecases where the >>>>>>>>>>> container (A) is not generic. are we also going to make >>>>>>>>>>> setModel(T) >>>>>>>>>>> protected? that would require the model assignment be done >>>>>>>>>>> through >>>>>>>>>>> the >>>>>>>>>>> constructor only and would eliminate any possibility of writing >>>>>>>>>>> builder-style code. consider a simple example: >>>>>>>>>>> >>>>>>>>>>> new DropDownChoice("foo").setModel(bar).setChoices(baz)... >>>>>>>>>>> >>>>>>>>>>> this kind of code should be possible whether written directly by >>>>>>>>>>> the >>>>>>>>>>> developer in the page or produced by some builder. >>>>>>>>>>> >>>>>>>>>>> -igor >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Thu, Sep 27, 2012 at 5:19 AM, Sven Meier <s...@meiers.net> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Even a simpler example might fail (no PropertyModel involved): >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> class APanel extends Panel { >>>>>>>>>>>> APanel(String id, IModel<Some> model) { >>>>>>>>>>>> super(id,model); >>>>>>>>>>>> >>>>>>>>>>>> add(new BPanel("b", model); >>>>>>>>>>>> } >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> A client using APanel might later change the model, leaving >>>>>>>>>>>> BPanel >>>>>>>>>>>> working >>>>>>>>>>>> on the old model: >>>>>>>>>>>> aPanel.setDefaultModel(otherModel); >>>>>>>>>>>> >>>>>>>>>>>> You could argue that APanel should be made failsafe when passing >>>>>>>>>>>> the >>>>>>>>>>>> model: >>>>>>>>>>>> >>>>>>>>>>>> add(new BPanel("b", new PropertyModel(this, >>>>>>>>>>>> "defaultModel"))); >>>>>>>>>>>> >>>>>>>>>>>> But it would be much easier if APanel could assume that its >>>>>>>>>>>> model >>>>>>>>>>>> isn't >>>>>>>>>>>> changed unattendedly. >>>>>>>>>>>> >>>>>>>>>>>> IMHO changing a component's model isn't the "wicket way" so I'd >>>>>>>>>>>> suggest >>>>>>>>>>>> changing the visibility of Component#setDefaultModel() to >>>>>>>>>>>> protected. >>>>>>>>>>>> >>>>>>>>>>>> Sven >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 09/27/2012 10:47 AM, Martin Grigorov wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> I see. This is an advanced way to create a static model :-) >>>>>>>>>>>>> But again I find PropertyModel as the real problem here. >>>>>>>>>>>>> >>>>>>>>>>>>> I'll let others give their opinions too. >>>>>>>>>>>>> >>>>>>>>>>>>> On Thu, Sep 27, 2012 at 11:39 AM, Michael Mosmann >>>>>>>>>>>>> <mich...@mosmann.de> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Am 27.09.2012 09:51, schrieb Martin Grigorov: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>> >>>>>>>>>>>>>> a dont care about the type issue here.. Maybe i can explain it >>>>>>>>>>>>>> again >>>>>>>>>>>>>> in >>>>>>>>>>>>>> an >>>>>>>>>>>>>> other way: >>>>>>>>>>>>>> >>>>>>>>>>>>>> APanel uses model instance A and the label uses a property >>>>>>>>>>>>>> model >>>>>>>>>>>>>> instance >>>>>>>>>>>>>> P >>>>>>>>>>>>>> which uses a reference to model instance A. >>>>>>>>>>>>>> >>>>>>>>>>>>>> After calling APanel.setDefaultModel(B) APanel uses model >>>>>>>>>>>>>> instance >>>>>>>>>>>>>> B,but >>>>>>>>>>>>>> label uses model instance P which uses model instance A as >>>>>>>>>>>>>> before. >>>>>>>>>>>>>> So >>>>>>>>>>>>>> the >>>>>>>>>>>>>> label does not see any changes, because no one tells the model >>>>>>>>>>>>>> instance >>>>>>>>>>>>>> P, >>>>>>>>>>>>>> that it should use B instead of A. I think, there are rare >>>>>>>>>>>>>> cases >>>>>>>>>>>>>> for >>>>>>>>>>>>>> such >>>>>>>>>>>>>> a >>>>>>>>>>>>>> usage. >>>>>>>>>>>>>> >>>>>>>>>>>>>> thanks >>>>>>>>>>>>>> Michael >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> In this particular code I think the "problem" is >>>>>>>>>>>>>>> PropertyModel, >>>>>>>>>>>>>>> since >>>>>>>>>>>>>>> it brings the type unsafety. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Another solution is to make Component<T>, this way we can >>>>>>>>>>>>>>> remove >>>>>>>>>>>>>>> #setDefaultModel() and have #setModel(IModel<T>) only and >>>>>>>>>>>>>>> such >>>>>>>>>>>>>>> problems will go away. >>>>>>>>>>>>>>> But as discussed in early Wicket 1.4 days this will lead to >>>>>>>>>>>>>>> more >>>>>>>>>>>>>>> typing. With Java 7 diamonds it is half the typing though. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> For now you can use GenericPanel, GenericPage and all >>>>>>>>>>>>>>> FormComponent. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Thu, Sep 27, 2012 at 10:41 AM, Michael Mosmann >>>>>>>>>>>>>>> <mich...@mosmann.de> >>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Am 27.09.2012 09:01, schrieb Martin Grigorov: >>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I think, there is a little difference in using >>>>>>>>>>>>>>>> setDefaultModel >>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>> setDefaultModelObject .. the first one sets a new model >>>>>>>>>>>>>>>> instance, >>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>> second >>>>>>>>>>>>>>>> only change the value in the existing model. Some >>>>>>>>>>>>>>>> pseudo-code: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> class APanel extends Panel { >>>>>>>>>>>>>>>> APanel(String id,IModel<Some> model) { >>>>>>>>>>>>>>>> super(id,model); >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> add(new Label("name",new >>>>>>>>>>>>>>>> PropertyModel(getDefaultModel(),"name")); >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> If you replace the value in model, everything is fine and >>>>>>>>>>>>>>>> works >>>>>>>>>>>>>>>> as >>>>>>>>>>>>>>>> expected. >>>>>>>>>>>>>>>> If you call setDefaultModel you might think, that everything >>>>>>>>>>>>>>>> is >>>>>>>>>>>>>>>> fine, >>>>>>>>>>>>>>>> but >>>>>>>>>>>>>>>> its not. A child component does not use >>>>>>>>>>>>>>>> getParent().getDefaultModel() >>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>> get >>>>>>>>>>>>>>>> these changes. I saw a lot of code like this, which leads to >>>>>>>>>>>>>>>> trouble, >>>>>>>>>>>>>>>> if >>>>>>>>>>>>>>>> you >>>>>>>>>>>>>>>> change the model and not the value. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> If there is no benefit in using setDefaultModel over >>>>>>>>>>>>>>>> setDefaultModelObject i >>>>>>>>>>>>>>>> would like to remove this method. This could prevent many >>>>>>>>>>>>>>>> "you >>>>>>>>>>>>>>>> might >>>>>>>>>>>>>>>> not >>>>>>>>>>>>>>>> got >>>>>>>>>>>>>>>> the full picture how to use wicket the right way" errors. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Michael >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Most of the time it is recommended to use a dynamic model, >>>>>>>>>>>>>>>>> so >>>>>>>>>>>>>>>>> there >>>>>>>>>>>>>>>>> is >>>>>>>>>>>>>>>>> no reason to replace the component's model. >>>>>>>>>>>>>>>>> Component#setDefaultModel() gives you semi-dynamic nature - >>>>>>>>>>>>>>>>> you >>>>>>>>>>>>>>>>> can >>>>>>>>>>>>>>>>> replace the model completely with a new one. Same with >>>>>>>>>>>>>>>>> #setDefaultModelObject(). >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> What is the problem you face with it ? >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Thu, Sep 27, 2012 at 9:57 AM, Michael Mosmann >>>>>>>>>>>>>>>>> <mich...@mosmann.de> >>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> is there any usefull application of >>>>>>>>>>>>>>>>>> Component.setDefaultModel(...)? >>>>>>>>>>>>>>>>>> IMHO >>>>>>>>>>>>>>>>>> this Method is the cause for much trouble without any >>>>>>>>>>>>>>>>>> benefit. >>>>>>>>>>>>>>>>>> But >>>>>>>>>>>>>>>>>> maybe >>>>>>>>>>>>>>>>>> i >>>>>>>>>>>>>>>>>> did not understand when someone should replace a component >>>>>>>>>>>>>>>>>> model... >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> thanks >>>>>>>>>>>>>>>>>> Michael >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >> >> > -- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com
