On 11-12-09 10:13 AM, mck wrote:
Where is the patch? for my part it's a lot easier to join the discussion
when the code changes in question are also readable...
You're right, I should have told. It's sitting here:
https://github.com/nlebas/tiles/tree/request-api
Right. still getting use to that.
Give me a chance to moderate it...
I'm still a newbie on git, but is it possible for you to collapse these
commits regarding this discussion into one? (...or collapse them so to
meet the separate points in this discussion if there's any likelihood
they'll end up as separate commits).
Basically the branch is all about it. I suppose the first 3 commits
could be collapsed into a big one, although the thought process is more
obvious this way.
Also i notice that some of the diffs are awkward to read because of
whitespace changes on every line. I've also noticed you've removed the
apache license header from some files...
eg
https://github.com/nlebas/tiles/commit/78f6723a36cbfe607befd8621e9dc9354a954088#tiles-request/tiles-request-api/src/main/java/org/apache/tiles/request/Request.java
hmm... Checkstyle should have caught that kind of mistake, I'll look
into it. Obviously there's something wrong with my working environment,
I'll fix it.
Is "WebRequest" the correct name? Servlet and Portlet technologies (and
the java ee stack) indicates web but couldn't the base request be used
for other web requests that were simple (but not java ee),
I'm not sure I understand your point, and I'm not sure if I've been
clear either. Providing the code should help explaining myself. Would
you prefer "JavaEERequest"?
[...]
So my preference is the name DispatchRequest which would also mirror
DispatcherRenderer where you were at one point proposing the
functionality to go to...
I like that.
* getHeader split into two maps, one immutable as documented, the other
write-only for response headers.
The purpose of the Map interface is to access the headers easily from
expression languages (${request.header['User-Agent']}). But of course a
read-only map is enough for that purpose; ELs don't need to access the
response.
Then i'd rather have the method:
Addable<String> getResponseHeaders();
The concept of a write-only map seems odd to me.
It is odd indeed. Returning Addable is fine with me.
At the same time should we change
Map<String,String> getHeaders();
to
ReadOnlyEnumerationMap<String> getHeaders();
?
ReadOnlyEnumerationMap looks like an implementation detail to me, since
it is a map implemented by the o.a.t.r.attribute package.
I agree a ReadOnlyMap interface without the "put" and "putAll" methods
would be best if the java language provided it, but it doesn't. The
example provided by the language is "Collections.unmodifiableMap()", and
I believe it should not surprise the users if we behave the same.
* I'd like to add a method "checkNotModified" to the Request interface.
It would take an timestamp as a parameter, repesenting the actual last
modification time of the model.
And that's exactly the situation I'm addressing.
Then i'm not understanding.
When and where would request.checkNotModified(timestamp) be called?
What would be the implementation of it in ServletRequest?
From the "glue" code between controller and view that typically creates
the Request instance, like TilesDispatchServlet for instance. I used to
call it from a Spring View among other places, now I tend to favor some
kind of renderer wrapper for reusability.
Of course it requires the controller to provide the actual timestamp of
the last modification as a reference.
And of course spring MVC implements that method, so I could have the
controller do it in the situations where I use spring MVC, but... well,
I'm just not always in a web context.
From a long term point of view, we could imagine using wrappers or
interceptors around definitions or renderers. These would be a
generalization of both ViewPreparer and PublisherRenderer, and then a
preparer could get the timestamp and check it against the request.
We could also imagine a "CachingRenderer" with a
"CachingRequestWrapper", caching HTML fragments for performance (ehcache
has a solution to do it using ServletFilters, but... let's just say I
think tiles can do a better job of it).
Nick.
