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: [email protected] > For additional commands, e-mail: [email protected] > > > -- 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: [email protected] For additional commands, e-mail: [email protected]
