wicket-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matej Knopp <matej.kn...@gmail.com>
Subject Re: [wicket 1.5] url handling refactor preview
Date Sun, 04 Oct 2009 13:43:36 GMT
On Sun, Oct 4, 2009 at 3:03 PM, Erik van Oosten <e.vanoosten@grons.nl> wrote:
> 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.
Yes. The root mapper can intercept all URLs. It can handle only
certain URLs and let the delegate handler others. Even better it can
easily transform the URLs.
>
> * 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.
Given the confusion the map methods cause we might consider renaming them.
>
> * 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.
Yes. Some mappers might need to see the POST data therefore the method
gets Request object instead of URL.
>
> * 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.)
RequestHandler should not care about request parameters at all. That's
what RequestMapper does. It takes request and produces RequestHandler.
It must of course know which handlers to produce and which it can
accept (to generate URLs). On the other hand RequestHandler doesn't
know about RequestMapper nor should it. It's not tight coupling, just
a chain of responsibility.

RequestMapper knows how to parse urls and create request handlers, it
also knows how to produce URLs for those request handlers. The
responsibility of RequestHandler is to produce response (which can
sometimes include other activities, such as invoking listener
interface on components).

>
> * I don't like AbstractRequestMapper. Its sole function is to provide helper
> methods. These are better located in a separate helper class.
Helper class with bunch of static methods? I really don't see any
benefit. i think it would just make the code uglier.
>
> * 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?
It should be much easier than in 1.4. If you want to store some state
as parameter you would call
getPage().getPageParameters().setNamedParameter("foo", "bar")  or
getPage().getPageParameters().setIndexedParameter(0, "abc");

Look at wicket-ng-webapp TestPage1.
>
> * 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?
Well, if being the smallest positive number counts as magical then
yes. If java had unsigned numbers the result type would be unsigned
int. Look at RequestMapper#getCompatibilityScore javadoc. There is a
suggestion how the algorithm should work (matching number of
segments).
>
> * 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.
Premature optimizations most probably. Unless you have thousands of
mount points. Also, implementation wise compatibility score
can be completely independent to request decoding.
Nevertheless should this become a bottleneck (which i think is highly
unlikely)  we can cache
the compatibility score during request in RequestCycle metadata.
>
> 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.
Some instanceofs are used in RequestMappers. Each request mapper knows
what kind of requesthandlers it can encode. We could probably
implement it without the instanceof checks but I really doubt it would
make the code any more readable or simpler.

Thanks for the feedback!

-Matej
>
> 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