Component can continue having the public isVisible/Enabled method always returning its correct state avaliable to use. Just the core request code will use an safe version that can be: isVisible/EnabledOnRequest. I really value an request cycle protected against volatile component states. I like the idea of using composition. We can refactor all the Component state code to an StateManager object. Its main goal can be: - maintain the components state: all its flags, the data object and respective API - define how close the "visible/enabledOnRequest" state will be to the user logic: we can define the check points, by default will be only the onConfigure. Perhaps user will can even override the SateManager, coding specific component logic (can solve some security restrictions).
On Fri, Dec 3, 2010 at 11:12 PM, Eelco Hillenius <eelco.hillen...@gmail.com>wrote: > That's actually also an example of why I prefer overriding isVisible: > I can have that button and other widgets (maybe completely unrelated) > widgets that depend on a particular state (like whether a record > exists), and a function (override of isVisible) will always yield the > correct result. In contrast, relying on the internal component state > (set/isVisible) puts the responsibility of updating the relevant > components on the handler (or worse, it may have to be triggered from > the business layer). > > Eelco > > On Fri, Dec 3, 2010 at 10:46 AM, Igor Vaynberg <igor.vaynb...@gmail.com> > wrote: > > ldm also doesnt always work nicely. suppose you have a delete button > > that is only visible if the record exists. the page renders, the user > > clicks the button. the onclick handler invoked, and isvisible is > > checked - at which point it is true. the record is deleted in the > > onclick handler, the page is rerendered - delete button is still > > visible because the ldm has cached the visibility value from before > > onclick handler is invoked. to make it work correctly the ldm would > > have to be detached manually after onclick handler. > > > > -igor > > > > On Thu, Dec 2, 2010 at 6:36 PM, Clint Checketts <checke...@gmail.com> > wrote: > >> Yesterday a friend following this thread pointed out that we should > rethink > >> our overriding of onVisible and use onConfigure. I've used > >> LoadabledDetachableModels to cache the value used in my > isVisible/isEnabled > >> overriding so changing values mid request aren't a problem. That is its > >> whole purpose. Also calling .detach() on that model isn't hacky, that is > its > >> design. > >> > >> While I appreciate having onConfigure as an option it seems like > overriding > >> isVisible is still the cleaner and clearer way. Folks just need to > follow > >> the rule that expensive calls should be contained in an LDM. > >> > >> Am I stuck in the past in holding this view? > >> > >> -Clint > >> > >> On Thu, Dec 2, 2010 at 4:03 AM, Martin Makundi < > >> martin.maku...@koodaripalvelut.com> wrote: > >> > >>> What about using onconfigure to replace loadabledetachablemodel ? We > >>> have had some trouble with loadabledetachablemodels when their state > >>> is frozen before a dependent model has been initialized (for example) > >>> and we need to call model.detach() from within our code, which seems > >>> bit hacky. > >>> > >>> Initializing also models at a specific known requestcycle moment might > >>> be beneficial. Ofcourse it is not so straightforward as with > >>> enable/visible state. > >>> > >>> ** > >>> Martin > >>> > >>> 2010/12/1 Igor Vaynberg <igor.vaynb...@gmail.com>: > >>> > i would be happy if that was good enough. in the past it hasnt been, > >>> > thats why we have the current solution. maybe we can try it again in > >>> > 1.5 and see what happens. > >>> > > >>> > -igor > >>> > > >>> > On Tue, Nov 30, 2010 at 11:44 AM, Pedro Santos <pedros...@gmail.com> > >>> wrote: > >>> >> I have a different point of view, the HTTP imposes us some > limitations, > >>> we > >>> >> will hardly have an good synchronization between the component state > on > >>> >> browser and server using only HTTP conversation. So it is mandatory > the > >>> >> service layer to respect the described security restriction. > >>> >> > >>> >> On Tue, Nov 30, 2010 at 5:32 PM, Igor Vaynberg < > igor.vaynb...@gmail.com > >>> >wrote: > >>> >> > >>> >>> an easy example is security. > >>> >>> > >>> >>> a user views a page that allows them to delete another user > >>> >>> meanwhile their permissions are tweaked and they can no longer > delete > >>> >>> other users > >>> >>> half an hour later the user clicks the delete button - this should > >>> >>> fail, but wont if we are using last-rendered state. > >>> >>> > >>> >>> -igor > >>> >>> > >>> >>> On Tue, Nov 30, 2010 at 11:18 AM, Pedro Santos < > pedros...@gmail.com> > >>> >>> wrote: > >>> >>> > I need to look better on which core components are relying on an > >>> updated > >>> >>> > visibility/enabled state at the process event time, and why the > last > >>> >>> > rendered state wouldn't be enough to them to work nicely. > >>> >>> > > >>> >>> > On Tue, Nov 30, 2010 at 3:19 PM, Igor Vaynberg < > >>> igor.vaynb...@gmail.com > >>> >>> >wrote: > >>> >>> > > >>> >>> >> currently we only invoke configure before the render. this would > >>> mean > >>> >>> >> we would have to invoke it before processing a listener, > clearing > >>> the > >>> >>> >> cache, and then invoking it again before render. i wonder if > that is > >>> >>> >> enough places to invoke it.... > >>> >>> >> > >>> >>> >> -igor > >>> >>> >> > >>> >>> >> On Tue, Nov 30, 2010 at 9:15 AM, Pedro Santos < > pedros...@gmail.com> > >>> >>> wrote: > >>> >>> >> > If user click an link, it will change the value of some > property > >>> at > >>> >>> the > >>> >>> >> > process_event request cycle step. Then the processor will go > to > >>> the > >>> >>> >> respond > >>> >>> >> > step, will invoke every component before render method which > will > >>> end > >>> >>> up > >>> >>> >> > invoking the Component#configure and updating the > >>> visibility/enabled > >>> >>> >> state > >>> >>> >> > (even if it changes, we are able to work with the updated > state). > >>> So > >>> >>> when > >>> >>> >> > the this component has the opportunity to render it self, it > will > >>> be > >>> >>> >> aware > >>> >>> >> > its update state. > >>> >>> >> > > >>> >>> >> > On Tue, Nov 30, 2010 at 2:39 PM, Igor Vaynberg < > >>> >>> igor.vaynb...@gmail.com > >>> >>> >> >wrote: > >>> >>> >> > > >>> >>> >> >> there are other places that should be checked though. for > example > >>> >>> >> >> before we invoke a listener on the component we should check > >>> again to > >>> >>> >> >> make sure that visibility hasnt changed. eg if visibility > depends > >>> on > >>> >>> >> >> some property of the user clicking the link that changed > between > >>> >>> >> >> render and clicking the link. > >>> >>> >> >> > >>> >>> >> >> -igor > >>> >>> >> >> > >>> >>> >> >> On Tue, Nov 30, 2010 at 8:30 AM, Pedro Santos < > >>> pedros...@gmail.com> > >>> >>> >> wrote: > >>> >>> >> >> > An implementation idea: > >>> >>> >> >> > > >>> >>> >> >> > Component { > >>> >>> >> >> > public final void configure() > >>> >>> >> >> > { > >>> >>> >> >> > if (!getFlag(FLAG_CONFIGURED)) > >>> >>> >> >> > { > >>> >>> >> >> > setVisible_NoClientCode(isVisible()); //we only > >>> check > >>> >>> the > >>> >>> >> user > >>> >>> >> >> > isVisible in here > >>> >>> >> >> > onConfigure(); > >>> >>> >> >> > setFlag(FLAG_CONFIGURED, true); > >>> >>> >> >> > } > >>> >>> >> >> > } > >>> >>> >> >> > } > >>> >>> >> >> > > >>> >>> >> >> > On Tue, Nov 30, 2010 at 2:16 PM, Igor Vaynberg < > >>> >>> >> igor.vaynb...@gmail.com > >>> >>> >> >> >wrote: > >>> >>> >> >> > > >>> >>> >> >> >> so how is it different if they can still override > something > >>> that > >>> >>> >> needs > >>> >>> >> >> >> to be checked all the time? > >>> >>> >> >> >> > >>> >>> >> >> >> -igor > >>> >>> >> >> >> > >>> >>> >> >> >> On Tue, Nov 30, 2010 at 8:02 AM, Pedro Santos < > >>> >>> pedros...@gmail.com> > >>> >>> >> >> wrote: > >>> >>> >> >> >> > I understand the concern about possible isVisible > >>> >>> implementations > >>> >>> >> like > >>> >>> >> >> >> > > >>> >>> >> >> >> > isVisible(return currentlyTime < 10:00:00;) //imagine > this > >>> >>> >> component > >>> >>> >> >> >> being > >>> >>> >> >> >> > rendered at 09:59:59 > >>> >>> >> >> >> > isVisible(return dao.list().size() > 0);// performance > >>> issues > >>> >>> >> >> >> > > >>> >>> >> >> >> > But maybe we can have the best from both approaches. > This is > >>> an > >>> >>> >> >> >> copy/paste > >>> >>> >> >> >> > from java.awt.Component: > >>> >>> >> >> >> > > >>> >>> >> >> >> > public boolean isVisible() { > >>> >>> >> >> >> > return isVisible_NoClientCode(); > >>> >>> >> >> >> > } > >>> >>> >> >> >> > final boolean isVisible_NoClientCode() { > >>> >>> >> >> >> > return visible; > >>> >>> >> >> >> > } > >>> >>> >> >> >> > > >>> >>> >> >> >> > There are some points in the awt framework were the > >>> isVisible > >>> >>> >> method > >>> >>> >> >> is > >>> >>> >> >> >> not > >>> >>> >> >> >> > used in benefit of isVisible_NoClientCode > >>> >>> >> >> >> > I'm in favor of create an final isVisible/Enabled > version > >>> and > >>> >>> >> change > >>> >>> >> >> the > >>> >>> >> >> >> > Wicket core to use it. Also maintain the hotspot to > users > >>> >>> provide > >>> >>> >> >> their > >>> >>> >> >> >> > isVisible/Enable implementations that will serve to feed > the > >>> >>> core > >>> >>> >> >> >> component > >>> >>> >> >> >> > state. > >>> >>> >> >> >> > > >>> >>> >> >> >> > On Mon, Nov 29, 2010 at 4:56 PM, Igor Vaynberg < > >>> >>> >> >> igor.vaynb...@gmail.com > >>> >>> >> >> >> >wrote: > >>> >>> >> >> >> > > >>> >>> >> >> >> >> ive run into plenty of weird problems with overrides, > but > >>> maybe > >>> >>> >> >> >> >> because this was in a high concurrency app where data > >>> changed > >>> >>> >> >> >> >> frequently. the problems arise from the fact that the > value > >>> >>> >> returned > >>> >>> >> >> >> >> from isvisible() can change while we are doing > traversals, > >>> etc. > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> eg we run a traversal for all visible components and > put > >>> them > >>> >>> in a > >>> >>> >> >> >> >> list. later we iterate over the list and try to render > >>> these > >>> >>> >> >> >> >> components. the render function also checks their > >>> visibility > >>> >>> and > >>> >>> >> if > >>> >>> >> >> >> >> they are no longer visible it throws an exception. > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> if isvisible() override depends on some external factor > >>> like > >>> >>> the > >>> >>> >> >> >> >> database there is a small window where the value can > change > >>> and > >>> >>> >> now > >>> >>> >> >> >> >> you can have a weird exception: such as "tried to > invoke a > >>> >>> >> listener > >>> >>> >> >> on > >>> >>> >> >> >> >> a component that is not visible or not enabled". these > are > >>> very > >>> >>> >> >> >> >> intermittent and damn near impossible to reproduce. > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> another problem is performance. isvisible() is called > >>> multiple > >>> >>> >> times > >>> >>> >> >> >> >> during the request and if it depends on the database it > can > >>> be > >>> >>> a > >>> >>> >> >> >> >> performance problem. in fact a couple of users have > >>> complained > >>> >>> >> about > >>> >>> >> >> >> >> this on the list in the past. at least now we have an > easy > >>> >>> >> solution > >>> >>> >> >> >> >> for them - use onconfigure(). > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> so as of right now the developers have two choices: > >>> override > >>> >>> >> >> >> >> isvisible() and potentially suffer the consequences. > or, > >>> >>> override > >>> >>> >> >> >> >> onconfigure() and set visibility there in a more > >>> deterministic > >>> >>> >> >> >> >> fashion. > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> -igor > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> > >>> >>> >> >> >> >> On Mon, Nov 29, 2010 at 10:21 AM, Eelco Hillenius > >>> >>> >> >> >> >> <eelco.hillen...@gmail.com> wrote: > >>> >>> >> >> >> >> > To expand, unless I'm missing something (new?), > things > >>> are > >>> >>> >> really > >>> >>> >> >> only > >>> >>> >> >> >> >> > problematic when both the mutable value and the > override > >>> are > >>> >>> >> mixed. > >>> >>> >> >> In > >>> >>> >> >> >> >> > a way, I think that using the override is 'more > pure', as > >>> >>> it's a > >>> >>> >> >> >> >> > simple function that is executed when needed, whereas > >>> mutable > >>> >>> >> state > >>> >>> >> >> >> >> > can be harder to deal with when trying to figure out > how > >>> it > >>> >>> got > >>> >>> >> to > >>> >>> >> >> be > >>> >>> >> >> >> >> > in that state. So, sorry Igor, but we disagree on > this > >>> one. > >>> >>> >> >> >> >> > > >>> >>> >> >> >> >> > Eelco > >>> >>> >> >> >> >> > > >>> >>> >> >> >> >> > > >>> >>> >> >> >> >> > On Mon, Nov 29, 2010 at 10:13 AM, Eelco Hillenius > >>> >>> >> >> >> >> > <eelco.hillen...@gmail.com> wrote: > >>> >>> >> >> >> >> >> Niether is evil. It has potential pitfalls, which > you > >>> should > >>> >>> >> just > >>> >>> >> >> be > >>> >>> >> >> >> >> >> aware of. We use such overrides all over the place > and > >>> never > >>> >>> >> have > >>> >>> >> >> >> >> >> problems with them either. :-) Avoiding it is safer, > but > >>> >>> also > >>> >>> >> more > >>> >>> >> >> >> >> >> verbose (in 1.3.x at least). > >>> >>> >> >> >> >> >> > >>> >>> >> >> >> >> >> Eelco > >>> >>> >> >> >> >> >> > >>> >>> >> >> >> >> >> On Mon, Nov 29, 2010 at 9:49 AM, Igor Vaynberg < > >>> >>> >> >> >> igor.vaynb...@gmail.com> > >>> >>> >> >> >> >> wrote: > >>> >>> >> >> >> >> >>> On Mon, Nov 29, 2010 at 9:35 AM, Sven Meier < > >>> >>> s...@meiers.net> > >>> >>> >> >> >> wrote: > >>> >>> >> >> >> >> >>>> Hi Douglas, > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>> WICKET-3171 describes a problematic case, where > >>> visibility > >>> >>> of > >>> >>> >> a > >>> >>> >> >> >> >> >>>> component changes while its form is being > processed. > >>> >>> >> >> >> >> >>>> In our projects we're overriding isVisible() where > >>> >>> >> appropriate > >>> >>> >> >> and > >>> >>> >> >> >> >> never > >>> >>> >> >> >> >> >>>> encountered a similar problem. > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>> I'd say WICKET-3171 is the rare 5% usecase. What's > >>> next, > >>> >>> is > >>> >>> >> >> >> overriding > >>> >>> >> >> >> >> >>>> isEnabled() going to be declared evil too? ;) > >>> >>> >> >> >> >> >>> > >>> >>> >> >> >> >> >>> yes > >>> >>> >> >> >> >> >>> > >>> >>> >> >> >> >> >>> -igor > >>> >>> >> >> >> >> >>> > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>> Sven > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>> On Mon, 2010-11-29 at 11:22 -0600, Douglas > Ferguson > >>> wrote: > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>>> Can you explain why? We have done this all over > the > >>> >>> place. > >>> >>> >> >> >> >> >>>>> > >>> >>> >> >> >> >> >>>>> D/ > >>> >>> >> >> >> >> >>>>> > >>> >>> >> >> >> >> >>>>> On Nov 29, 2010, at 10:00 AM, Martin Grigorov > wrote: > >>> >>> >> >> >> >> >>>>> > >>> >>> >> >> >> >> >>>>> > The recommended way since a few 1.4 releases is > to > >>> >>> >> override > >>> >>> >> >> >> >> onConfigure() > >>> >>> >> >> >> >> >>>>> > and call setVisible(true|false) depending on > your > >>> >>> >> conditions. > >>> >>> >> >> >> >> >>>>> > > >>> >>> >> >> >> >> >>>>> > On Mon, Nov 29, 2010 at 4:49 PM, Douglas > Ferguson < > >>> >>> >> >> >> >> >>>>> > doug...@douglasferguson.us> wrote: > >>> >>> >> >> >> >> >>>>> > > >>> >>> >> >> >> >> >>>>> >> Igor posted a comment to this bug saying that > >>> >>> overriding > >>> >>> >> >> >> >> isVisible() is > >>> >>> >> >> >> >> >>>>> >> "evil" > >>> >>> >> >> >> >> >>>>> >> > >>> >>> >> >> >> >> >>>>> >> > >>> >>> https://issues.apache.org/jira/browse/WICKET-3171 > >>> >>> >> >> >> >> >>>>> >> > >>> >>> >> >> >> >> >>>>> >> I was surprised by this and am curious to hear > >>> more. > >>> >>> >> >> >> >> >>>>> >> > >>> >>> >> >> >> >> >>>>> >> D/ > >>> >>> >> >> >> >> >>>>> > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>>> > >>> >>> >> >> >> >> >>> > >>> >>> >> >> >> >> >> > >>> >>> >> >> >> >> > > >>> >>> >> >> >> >> > >>> >>> >> >> >> > > >>> >>> >> >> >> > > >>> >>> >> >> >> > > >>> >>> >> >> >> > -- > >>> >>> >> >> >> > Pedro Henrique Oliveira dos Santos > >>> >>> >> >> >> > > >>> >>> >> >> >> > >>> >>> >> >> > > >>> >>> >> >> > > >>> >>> >> >> > > >>> >>> >> >> > -- > >>> >>> >> >> > Pedro Henrique Oliveira dos Santos > >>> >>> >> >> > > >>> >>> >> >> > >>> >>> >> > > >>> >>> >> > > >>> >>> >> > > >>> >>> >> > -- > >>> >>> >> > Pedro Henrique Oliveira dos Santos > >>> >>> >> > > >>> >>> >> > >>> >>> > > >>> >>> > > >>> >>> > > >>> >>> > -- > >>> >>> > Pedro Henrique Oliveira dos Santos > >>> >>> > > >>> >>> > >>> >> > >>> >> > >>> >> > >>> >> -- > >>> >> Pedro Henrique Oliveira dos Santos > >>> >> > >>> > > >>> > >> > > > -- Pedro Henrique Oliveira dos Santos