On Thu, Sep 27, 2012 at 10:29 PM, Sven Meier <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>>>> 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 >>>>>>>>>>> <[email protected]> >>>>>>>>>>> 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 >>>>>>>>>>>>> <[email protected]> >>>>>>>>>>>>> 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 >>>>>>>>>>>>>>> <[email protected]> >>>>>>>>>>>>>>> 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
