wicket-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Erik van Oosten <e.vanoos...@grons.nl>
Subject Re: [wicket 1.5] url handling refactor preview
Date Sun, 04 Oct 2009 13:03:16 GMT
Hi,

* I am delighted that the root request mapper is also pluggable and that 
request mappers can easily decorate other request mappers.

* In the new setup, am I correct in assuming that you can override the 
root request mapper to make it possible to parse parameters for the 
'home page'? I am looking for an easy way to parse URIs like "/?p1=v1". 
I see that the example app still needs to override getHomePage. I guess 
this is no longer needed always.

* With all respect to the given code, good names are important and 
therefore comments on names are important as well. A good name will 
deepen understanding. I too find that overloading of these 2 method 
names is not correct as they do not perform the same function.

* There is also a little bell in my head saying that the 2 map methods 
are not symmetric. I guess this can not be helped as incoming requests 
have post data where as generated URLs do not. I like the URL class, in 
a RequestMapper decorator it makes it possible to manipulate the URL 
that was given by the chained RequestMapper.

* I am a bit confused about the RequestHandler interface. It contains 
nothing to get the request's parameters. How is method 
RequestMapper#map(RequestHandler) supposed to work then? I guess it will 
always need to know about the types of RequestHandlers it supports. This 
introduces a tight coupling I did not expect. (I am not saying it is 
wrong, it just confused me.)

* I don't like AbstractRequestMapper. Its sole function is to provide 
helper methods. These are better located in a separate helper class.

* Its not yet clear to me how to implement pages that put part of the 
parameters in the URL and part in the page state. E.g. for a product 
list page put the product category in the URL and the current sort order 
in a component field. Will this be possible? Easy?

* I guess BufferedResponseMapper#getCompatibilityScore should return 
Integer.MIN_VALUE (instead of 0) when it can not process the request. Or 
is 0 a kind of magic value?

* What MountedMapper does is priceless!

* When I look at ThreadsafeCompoundRequestMapper I am a (little) bit 
worried about performance. The main worry is that methods are called 
multiple times for the same request. For example method 
getCompatibilityScore of the nested RequestMappers is called in both 
map(Request) as in getCompatibilityScore. A request mapper can not 
simultaneously calculate a map and a compatibility score precluding some 
optimizations.

My time is up. This is definitely an improvement over previous 
implementations. Even though there are still some corner cases (I still 
see a few instanceof's), it is much more straight forward. I hope to see 
this code merged as soon as possible.

Regards,
    Erik.


Igor Vaynberg wrote:
> as ive mentioned before, the focus of 1.5 will be the overhaul of how
> we handle the urls and process requests. this part of wicket has grown
> organically and has turned into a bunch of overcomplicated spaghetti
> code.
>
> .........
>
> feedback is welcome. above all we would like to hear all your weird
> and interesting url mapping scheme ideas so we can proof the api
> against them.
>
> -igor
>   


-- 
Erik van Oosten
http://www.day-to-day-stuff.blogspot.com/


Mime
View raw message