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/
