What is strange is that isvisible is being checked during detach (I seriously doubt that). That shouldn't be happening: *all* components should be detached regardless of their visibility.
Martijn On Thu, Jan 15, 2009 at 9:29 PM, Jonathan Locke <jonathan.lo...@gmail.com> wrote: > > > i think the next step is to see what the core team thinks about this. filing > a ticket couldn't hurt. > > > Jonathan Locke wrote: >> >> >> yeah, for full flexibility, you might just link a method in Component to a >> default in IRequestCycleSettings: >> >> boolean getCacheComponentVisibility() { return >> getApplication().getRequestCycleSettings().getCacheComponentVisibility(); >> } >> >> then you can set a default and override for any individual component (or >> page). >> >> >> Martin Makundi wrote: >>> >>>> you could always work it the other way and have no caching by default >>>> but >>>> allow people who know what they're doing to enable it for an application >>>> with something like >>>> IRequestCycleSettings#setCacheComponentVisibility(boolean). >>> >>> This sounds more robust, so long as it can be controlled on a >>> per-request basis (i.e., you can configure it for a specific page and >>> state). >>> >>> And don't forget about isEnabled too though it is more rare ;) >>> >>> ** >>> Martin >>> >>>> >>>> >>>> Martin Makundi wrote: >>>>> >>>>> Well... I would not build in the cache because different components >>>>> could interact in a way that if someone prematurely calls the >>>>> isVisible method, it freezes in the wrong state. It is known, that the >>>>> isVisible is called multiple times for various reasons. >>>>> >>>>> ** >>>>> Martin >>>>> >>>>> 2009/1/15 Jonathan Locke <jonathan.lo...@gmail.com>: >>>>>> >>>>>> >>>>>> not sure. it /ought/ to make sense to cache during the render cycle. >>>>>> but >>>>>> i >>>>>> have the eerie sense that this came up before, so maybe there's some >>>>>> reason >>>>>> why it can't be cached? (although even if there are some odd cases >>>>>> where >>>>>> it >>>>>> can't be, it seems like you could make a workaround for those cases >>>>>> ..) >>>>>> >>>>>> >>>>>> Scott Swank wrote: >>>>>>> >>>>>>> Is there a reason why the default behavior is not to cache the result >>>>>>> of isVisible()? Are there cases where the result of isVisible() is >>>>>>> expected to change over the course of rendering? >>>>>>> >>>>>>> Would a JIRA w/ code be welcome, or is the current behavior required? >>>>>>> >>>>>>> Scott >>>>>>> >>>>>>> On Thu, Jan 15, 2009 at 10:23 AM, Jonathan Locke >>>>>>> <jonathan.lo...@gmail.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> I would be careful not to throw the baby out with the bath water >>>>>>>> here. >>>>>>>> The >>>>>>>> design decision you're making is push versus pull and implementing >>>>>>>> isVisible >>>>>>>> is often going to be a better design decision because it may be more >>>>>>>> clear >>>>>>>> that visibility state stays consistent for a given problem. If the >>>>>>>> performance actually shows up as a hotspot (don't prematurely >>>>>>>> optimize!), >>>>>>>> you can always do some appropriate caching of the value and still >>>>>>>> have >>>>>>>> it >>>>>>>> "pulled". >>>>>>>> >>>>>>>> >>>>>>>> Erik van Oosten wrote: >>>>>>>>> >>>>>>>>> In the thread "Where to process PageParameters" I was requested to >>>>>>>>> explain why I think you should not override isVisible, but rather >>>>>>>>> should >>>>>>>>> call setVisible in onBeforeRender (slide 100 in my presentation >>>>>>>>> http://www.grons.nl/~erik/pub/20081112%20Effective%20Wicket.pdf). >>>>>>>>> >>>>>>>>> There are 2 reasons, but only the second one is really important. >>>>>>>>> >>>>>>>>> -1- isVisible is called a lot. It is easily called ten times within >>>>>>>>> 1 >>>>>>>>> request >>>>>>>>> >>>>>>>>> So if you have anything processor intensive going on, it will be a >>>>>>>>> performance hit. Just doing a simple expression is of course no >>>>>>>>> problem. >>>>>>>>> (For fun, just set a breakpoint in something like >>>>>>>>> FeedbackPanel#isVisible and request a page that contains one.) >>>>>>>>> >>>>>>>>> -2- isVisible can make your model be reloaded multiple times within >>>>>>>>> 1 >>>>>>>>> request >>>>>>>>> >>>>>>>>> Consider the following case (pseudo code): >>>>>>>>> >>>>>>>>> MyPanel(id, personId) { >>>>>>>>> super(id, new CompoundPropertyModel(new >>>>>>>>> LoadableDetachablePersonModel(personId))); >>>>>>>>> add( new Label("address") { >>>>>>>>> @Override >>>>>>>>> isVisible() { >>>>>>>>> return getDefaultModel() != null; >>>>>>>>> } >>>>>>>>> }); >>>>>>>>> } >>>>>>>>> >>>>>>>>> >>>>>>>>> The label uses the property 'address' of a person to see if the >>>>>>>>> label >>>>>>>>> should be visible. The person is retrieved by a >>>>>>>>> LoadableDetachableModel >>>>>>>>> subclass (LDM) and then wrapped by a CompoundPropertyModel (CPM). >>>>>>>>> >>>>>>>>> During the render phase, isVisible will delegate getting the >>>>>>>>> address >>>>>>>>> property to the CPM and this model delegates it to the LDM. LDM >>>>>>>>> will >>>>>>>>> load the person from the database only once (well until it is >>>>>>>>> detached). >>>>>>>>> >>>>>>>>> At the end of the render phase everything will be detached. But now >>>>>>>>> something weird happens. The problem is that isVisible is called >>>>>>>>> during >>>>>>>>> the detach phase, on the label /after/ the CPM (and therefore also >>>>>>>>> the >>>>>>>>> LDM) are detached. As isVisible retrieves the model from the CPM, >>>>>>>>> and >>>>>>>>> therefore from the LDM, it will trigger a reload of the person >>>>>>>>> inside >>>>>>>>> the LDM. >>>>>>>>> >>>>>>>>> Now, as visibility is often (if not almost always) determined by a >>>>>>>>> business object (e.g. very often a LDM) I think it makes sense to >>>>>>>>> avoid >>>>>>>>> having to think about the situation described above, and just avoid >>>>>>>>> it >>>>>>>>> all together. >>>>>>>>> >>>>>>>>> Note: I observed this behavior in Wicket 1.3 (1.3.3 I think). If it >>>>>>>>> works differently now, I would be very glad to withdraw this >>>>>>>>> recommendation. >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Erik. >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Erik van Oosten >>>>>>>>> http://day-to-day-stuff.blogspot.com/ >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> --------------------------------------------------------------------- >>>>>>>>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>>>>>>>> For additional commands, e-mail: users-h...@wicket.apache.org >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> View this message in context: >>>>>>>> http://www.nabble.com/Why-you-should-not-override-isVisible-tp21474995p21483816.html >>>>>>>> Sent from the Wicket - User mailing list archive at Nabble.com. >>>>>>>> >>>>>>>> >>>>>>>> --------------------------------------------------------------------- >>>>>>>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>>>>>>> For additional commands, e-mail: users-h...@wicket.apache.org >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> --------------------------------------------------------------------- >>>>>>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>>>>>> For additional commands, e-mail: users-h...@wicket.apache.org >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> -- >>>>>> View this message in context: >>>>>> http://www.nabble.com/Why-you-should-not-override-isVisible-tp21474995p21484456.html >>>>>> Sent from the Wicket - User mailing list archive at Nabble.com. >>>>>> >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>>>>> For additional commands, e-mail: users-h...@wicket.apache.org >>>>>> >>>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>>>> For additional commands, e-mail: users-h...@wicket.apache.org >>>>> >>>>> >>>>> >>>> >>>> -- >>>> View this message in context: >>>> http://www.nabble.com/Why-you-should-not-override-isVisible-tp21474995p21485341.html >>>> Sent from the Wicket - User mailing list archive at Nabble.com. >>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>>> For additional commands, e-mail: users-h...@wicket.apache.org >>>> >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org >>> For additional commands, e-mail: users-h...@wicket.apache.org >>> >>> >>> >> >> > > -- > View this message in context: > http://www.nabble.com/Why-you-should-not-override-isVisible-tp21474995p21486172.html > Sent from the Wicket - User mailing list archive at Nabble.com. > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org > For additional commands, e-mail: users-h...@wicket.apache.org > > -- Become a Wicket expert, learn from the best: http://wicketinaction.com Apache Wicket 1.3.5 is released Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.3. --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@wicket.apache.org For additional commands, e-mail: users-h...@wicket.apache.org