On Sat, Sep 17, 2011 at 2:57 AM, Igor Vaynberg <[email protected]> wrote: > On Fri, Sep 16, 2011 at 3:26 AM, Martin Grigorov <[email protected]> wrote: >> Hi, >> >> Currently AjaxRequestTarget (ART) is not very extendable because some >> of its methods are final. The same reason makes it impossible to use a >> mock instance for unit tests (pure unit test, not wicket tester test). >> Trying to instantiate ART is also not possible because it uses >> "RequestCycle.get()" in its constructor and >> ThreadContext.setRequestCycle() accepts RequestCycle, the class, not >> IRequestCycle - the interface, and this leads to creating mocks of >> several transitive dependencies to be able to use ART. > > so pass the RC into the constructor and keep it as a field... Actually just Response object is needed, but this is an API break... > >> In long term (maybe when we fix the packages split problem) I think we >> have to introduce IAjaxRequestHandler and change all signatures >> (onClick, onEvent, ...) to receive it instead of ART. > > this will basically be the public interface extracted from ART? Yes. Will extend IPageRequestHandler and will define the public methods from ART > >> This will also >> allow passing of NoopART as some people asked for. > > -1 on the noop thing. it seems pretty stupid. In my previous job we used custom noop impl from 2+ years and it served us well, but yes, maybe it shouldn't be part of the framework. > >> And maybe >> Optional<ART> wont be needed for AjaxFallback** components ?! > > introducing optional<art> uncovered a ton of NPEs in the tree > components. peter fixed some of these in a recent commit. some > components need to know if they are responding to an ajax request or > not, a noop target doesnt give them this option. Not sure when the code changed but in 1.4 it was more safe. See org.apache.wicket.markup.html.tree.AbstractTree.updateTree(AjaxRequestTarget) in 1.4. #updateTree(ART) returns early if the passed ART is null. In 1.5 we do the check in the callers which is worse than 1.4. That's why the bugs appeared. With Optional<ART> it will be the same as in 1.4, just the code that checks the parameter will be slightly different. > >> In short term removing "final" from method signatures would allow to >> mock it for tests and return customized impl from >> org.apache.wicket.protocol.http.WebApplication.getAjaxRequestTargetProvider(). > > i thought some newer libs could mock final methods, rg > > http://code.google.com/p/powermock/wiki/MockFinal Maybe this is the way to go. We use Mockito and it seems PowerMock supports the Mockito API and it will be easier to migrate to this more powerful library. I'll experiment. > >> I know 'final's helps to keep the framework stable by not allowing the >> user to override critical pieces of the functionality but I don't see >> better solution. >> >> What do you think ? > > its not as simple as removing the final keyword. we also need to > rewrite whatever code is written with the assumption that methods its > calling is final inside the ART. Yes, this is true. > > -igor > >> >> -- >> Martin Grigorov >> jWeekend >> Training, Consulting, Development >> http://jWeekend.com >> >
-- Martin Grigorov jWeekend Training, Consulting, Development http://jWeekend.com
