You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Ate Douma <at...@douma.nu> on 2007/09/18 14:14:11 UTC
Re: Please review WICKET-983: Merge the portlet support branch into
the trunk
Guys,
I'd like to respond here on the concerns raised by Al, Gwyn and Kent on the "Re: [jira] Resolved: (WICKET-647) New Wicket Portlet support" thread.
As I created a dedicated JIRA issue, WICKET-983, for this as well as this specific mail subject, I think we should try to centralize the discussion about it here.
First of all, thanks a lot for taking the time to look into this and review the changes I propose for adding portlet-support.
I do understand the concerns raised but I'd like to point out that, at least in my view, the impact for *normal* wicket runtime (e.g. servlet, not portlet) the
changes really are very, very minimal.
Al, Gwyn and Kent: you indicate that you see major and/or fundamental changes to wicket, but please make explicit which changes you are referring to.
Now I only have something like "the portlet changes /do/ touch some fundamental bits of Wicket", "making major changes to the codebase" and "It seems that
those changes are quite fundamental" to respond to.
Anyway, I'll try to make clear why in my view the portlet changes aren't going to cause "major" or "fundamental" changes in the core wicket.
If you evaluate the issues I listed in WICKET-983 and how I resolved those, you'll see that the changes which *might* have a small to zero effect on the
standard wicket behavior are the following:
1) WICKET-649: fix appending query parameters
a) replacing blindly appending "&" with a check if it really should be a "?"
In my view harmless and actually really improving wicket quality itself
b) moving AbstractAjaxBehavior.getCallbackUrl() appending "wicket:ignoreIfNotActive=true" to WebRequestCodingStrategy.encode().
This is a quite simple reordering of internal processing with exactly the same functionality and result as outcome, quite harmless I think
c) wicket-ajax Wicket.Ajax.Request.get(path) checking on query parameters and in that case replacing it with a Request.post(body) call
This one I think can/should be discussed as it really does result in a *technically* different behavior.
I do think in the case of Ajax requests, this really doesn't matter as it won't effect the browser state at all, and my solution really is
most transparent one I could think of.
But, as it also changes the behavior in a plain servlet environment, and as such it I could understand people would feel a bit uncomfortable with it.
I already suggest in the description for this change I'm open to other solutions, like only doing this when running in a portlet environment.
Then, this issue would become moot for the servlet environment, we just need to find some way to indicate the current environment (servlet|portlet)
in javascript to the browser.
d) IOnChangeListener components with wantOnSelectionChangedNotifications()==true
If you review my changes you'll find out there are *zero* changes when running in a servlet environment, as is with almost all other changes.
Based on the boolean evaluation of RequestContext.isPortletRequest(), which is always false in a servlet environment as you can imagine, only the original
code is executed. Only the portlet specific behavior is "wrapped" in a simple if (RequestContext.isPortletRequest())... nothing more.
2) WICKET-650: namespacing component markupId
This change does nothing when not running in a portlet environment
3) WICKET-651: extending IHeaderResponse
This change only adds a close() and isClosed() method to IHeaderResponse and delegates getResponse() to a getRealResponse() implementation.
These changes also have no side-effect, all current behavior is maintained. It just allows capturing the HeaderResponse when running in a portlet context.
It also allows you now to do the same in a servlet environment if you would want that, so it is an expansion of the api but it is not used other than from
portlet specific code.
4) WICKET-657: upgrading wicket-examples to require servlet api 2.4
I don't consider this a "major" change. For one, its just an example and not touching wicket core at all. Furthermore, I doubt there will be many users still
stuck on a servlet 2.3 container, especially as wicket-examples already requires Java 1.5 anyway.
Anyway, this change isn't critical for merging the portlet support in the trunk but for demonstration purposes it would be nice to be able to use it.
5) WICKET-924: non-relative urls in Ajax.Request.redirect callback handling
Actually, I would consider proposing this change regardless of the discussion of portlet-support. The current hard coded expectancy an Ajax callback redirect
can and may only be a Wicket relative url doesn't seem very flexible. For the portlet-support this change is needed though.
And, if the redirect url *is* relative as expected in the current implementation, my changes won't do anything anyway.
But if there are major objections to it, we could discuss solving it in a similar way as we could do with the Ajax.Request.get(path) -> .post(body) change.
If we can push the current runtime environment (servlet|portlet) to the browser the ajax script could maintain the current behavior when running in a servlet.
6) WICKET-926: recognizing popup/detached pages urlFor calls
This change also does nothing when not running in a portlet environment. But I did listed in WICKET-983 because the solution I chose didn't exactly result in
"beautiful" code... I couldn't come up with something else though which would be just as transparent and without *any* side-effects.
So, I'm actually hoping someone calls me stupid and comes up with a more "feel good" solution :)
That's it. All other changes to existing wicket classes will only come into effect when run in a portlet environment. When running in a servlet environment
Wicket behaves exactly as before!
Concluding: in my view the only real "issue" we might want to discuss is the Ajax.Request.get(path) -> .post(body) change.
I personally don't see it as an issue, but if that change would be viewed as unacceptable/dangerous for the current trunk, it should not be difficult to come up
with another solution which will keep the current behavior in a servlet environment (although that then will have to be a little bit less transparent I'm afraid).
So, if this assessment is valid, I really don't see why portlet support couldn't be integrated into the 1.3.0 trunk.
Again, I do understand the concerns raised, but don't forget quite a few major changes (like for bugfixes, but probably not even only for that) have been
committed, and still are committed to the trunk too. Some of those might have (much) more side-effects and consequences then adding portlet-support...
Delaying portlet-support to after the 1.3 release really would be a shame in this light in my view.
For those of us who don't need/care about portlet it won't matter, but I know quite a few developers and project teams are eager to be able to use Wicket for
portlet development. Waiting until the 1.4 branch stabilizes really means cutting them out.
Causing a delay of the 1.3 release isn't good for the community (and I don't think should be needed) but delaying portlet-support until after the 1.3 release
isn't good for the community either.
I do hope my above explanations make it a bit clearer what the real issues are to discuss.
If anyone of you still have reservations and/or major objections of merging portlet-support in the current trunk, please give it head and toe and indicate which
changes you think are questionable so we can properly discuss them.
Regards,
Ate
Ate Douma wrote:
> I've created https://issues.apache.org/jira/browse/WICKET-983 to help
> out reviewing the portlet-support changes.
>
> Although I initially planned to create separate "sub" patches for
> reviewing the important wicket changes individually, this turned out to
> be a
> rather impossible task. The complete size of the full patch is almost
> 200k and several different issues I needed to solve concerned the same
> classes/files. Breaking all that up in digestible peaces would cost just
> to much time to be realistic.
>
> I did however upload a full patch file and provide a short summary of
> the most important changes with links to the specific subtask issues of
> WICKET-647.
> These subtasks provide more detailed descriptions of the issues at hand
> as well as the commit logs of the changes I made.
>
> All committers or otherwise interested: please take some time to at
> least read the description of the problems I needed to solve.
> If you see anything you don't like or would have chosen a different
> solution, please bring it up here so we can discuss it.
>
> I would like to merge the portlet support branch into the trunk before
> the -beta4 cutoff, so your input on this is very much appreciated.
>
> Thanks,
>
> Ate Douma
>
>
>
Re: Please review WICKET-983: Merge the portlet support branch into
the trunk
Posted by Ate Douma <at...@douma.nu>.
Al, thanks for the reply.
I have a few comments inline below.
Al Maw wrote:
> Ate Douma wrote:
>> I do understand the concerns raised but I'd like to point out that, at
>> least in my view, the impact for *normal* wicket runtime (e.g.
>> servlet, not portlet) the changes really are very, very minimal.
>>
>> Al, Gwyn and Kent: you indicate that you see major and/or fundamental
>> changes to wicket, but please make explicit which changes you are
>> referring to.
>> Now I only have something like "the portlet changes /do/ touch some
>> fundamental bits of Wicket", "making major changes to the codebase"
>> and "It seems that those changes are quite fundamental" to respond to.
>>
>> Anyway, I'll try to make clear why in my view the portlet changes
>> aren't going to cause "major" or "fundamental" changes in the core
>> wicket.
>
> They're not major changes, but they do touch some fundamental bits of
> Wicket. :-)
>
> I'm specifically thinking about the URL handling, which isn't terribly
> well unit tested and has in the past proved to be rather brittle. I'd be
> quite surprised if you haven't introduced something subtle here, as
> there are so many facets (interaction with the various
> UrlCodingStrategies (both with and without parameters), getting it right
> for both WicketServlet and WicketFilter, auto-prepending relative paths
> to raw src/href/whatever attributes in HTML, getting it right when
> outputting AJAX fragments for requests made to a URL that's not at the
> same relative depth as the URL for the page it's from, etc. etc.).
I know the URL handling is quite complex :)
It was quite difficult to find the right spots where to intercept/provide alternate behavior for portlets.
And I agree, I might (and probably haven't) covered all the specific issues yet... for portlets.
The point I wanted to make is, for servlets behavior *hasn't* changed, except for the few specific areas I found where query string parameters are appended
manually (and I might have missed a few still).
So, while the url handling for portlets might not be perfect yet, this will only cause problems for portlets.
I do think do I covered the most common cases already enough to get started with real portlet development with Wicket.
Bugs will always be there, and in the case of portlets, I'll take the responsibility to maintain and fix those as best as I can.
But for servlets, this really won't introduce new issues beyond what is already brittle or even broken in the first place.
>
>> If you evaluate the issues I listed in WICKET-983 and how I resolved
>> those, you'll see that the changes which *might* have a small to zero
>> effect on the standard wicket behavior are the following:
>> [...]
>
> Thanks for your comprehensive list. It's very helpful. I'll need to
> spend some time evaluating these in some detail in order to say whether
> I'm happy with them or not, but won't really be able to look before
> about Sunday. :-(
That's ok for me, I just hope the others can wait with the -beta4 cutoff that long too.
>
> Here are my comments on the things that I don't need to look at the code
> for...
>
>> 4) WICKET-657: upgrading wicket-examples to require servlet api 2.4
>
> I have no objection to that one.
>
>> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback
>> handling
>> Actually, I would consider proposing this change regardless of the
>> discussion of portlet-support. The current hard coded expectancy an
>> Ajax callback redirect
>> can and may only be a Wicket relative url doesn't seem very flexible.
>> For the portlet-support this change is needed though.
>> And, if the redirect url *is* relative as expected in the current
>> implementation, my changes won't do anything anyway.
>> But if there are major objections to it, we could discuss solving it
>> in a similar way as we could do with the Ajax.Request.get(path) ->
>> .post(body) change.
>> If we can push the current runtime environment (servlet|portlet) to
>> the browser the ajax script could maintain the current behavior when
>> running in a servlet.
>
> The issue here is that we've made all the URLs relative for a good
> reason - to let you run Wicket behind a mod_proxy without having lots of
> pain. Additionally, you need the URLs in generated AJAX fragments to be
> relative to the page that the request was made from, not relative to the
> request itself. Your patch needs to make sure this behaves properly.
Not really...
Understand that a Wicket portlet runs *embedded* in a page and doesn't control the (browser) url anyway.
For portals, this is a known issue with any portlet url, regardless which framework the portlet is developed in.
For the portlet-support I actually had to rewrite the Wicket relative urls back to context relative urls to be able to serve them from a portlet!
But my change above does not affect plain (relative) Wicket urls at all, it just caters for the cases where the url happens to be fully qualified anyway.
>
>> Delaying portlet-support to after the 1.3 release really would be a
>> shame in this light in my view.
>
> Yes, I agree. It's really hard to maintain a patchset of this size, and
> squeezing it out to 1.4 (or whatever we're calling the next version)
> would make for unnecessary work if we can get it into 1.3 with a
> guarantee that it's not going to break things.
>
> I also suspect that we have a bunch of 1.2.x portlet users who we're
> going to leave high-and-dry otherwise, which is certainly a consideration.
>
>> If anyone of you still have reservations and/or major objections of
>> merging portlet-support in the current trunk, please give it head and
>> toe and indicate which changes you think are questionable so we can
>> properly discuss them.
>
> I'll have a proper in-depth review of your patch later in the week and
> let you know.
>
> Regards,
>
> Al
>
Thanks a lot already!
Regards,
Ate
Re: Please review WICKET-983: Merge the portlet support branch into
the trunk
Posted by Al Maw <wi...@almaw.com>.
Ate Douma wrote:
> I do understand the concerns raised but I'd like to point out that, at
> least in my view, the impact for *normal* wicket runtime (e.g. servlet,
> not portlet) the changes really are very, very minimal.
>
> Al, Gwyn and Kent: you indicate that you see major and/or fundamental
> changes to wicket, but please make explicit which changes you are
> referring to.
> Now I only have something like "the portlet changes /do/ touch some
> fundamental bits of Wicket", "making major changes to the codebase" and
> "It seems that those changes are quite fundamental" to respond to.
>
> Anyway, I'll try to make clear why in my view the portlet changes aren't
> going to cause "major" or "fundamental" changes in the core wicket.
They're not major changes, but they do touch some fundamental bits of
Wicket. :-)
I'm specifically thinking about the URL handling, which isn't terribly
well unit tested and has in the past proved to be rather brittle. I'd be
quite surprised if you haven't introduced something subtle here, as
there are so many facets (interaction with the various
UrlCodingStrategies (both with and without parameters), getting it right
for both WicketServlet and WicketFilter, auto-prepending relative paths
to raw src/href/whatever attributes in HTML, getting it right when
outputting AJAX fragments for requests made to a URL that's not at the
same relative depth as the URL for the page it's from, etc. etc.).
> If you evaluate the issues I listed in WICKET-983 and how I resolved
> those, you'll see that the changes which *might* have a small to zero
> effect on the standard wicket behavior are the following:
> [...]
Thanks for your comprehensive list. It's very helpful. I'll need to
spend some time evaluating these in some detail in order to say whether
I'm happy with them or not, but won't really be able to look before
about Sunday. :-(
Here are my comments on the things that I don't need to look at the code
for...
> 4) WICKET-657: upgrading wicket-examples to require servlet api 2.4
I have no objection to that one.
> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback handling
> Actually, I would consider proposing this change regardless of the
> discussion of portlet-support. The current hard coded expectancy an Ajax
> callback redirect
> can and may only be a Wicket relative url doesn't seem very flexible.
> For the portlet-support this change is needed though.
> And, if the redirect url *is* relative as expected in the current
> implementation, my changes won't do anything anyway.
> But if there are major objections to it, we could discuss solving it in
> a similar way as we could do with the Ajax.Request.get(path) ->
> .post(body) change.
> If we can push the current runtime environment (servlet|portlet) to the
> browser the ajax script could maintain the current behavior when running
> in a servlet.
The issue here is that we've made all the URLs relative for a good
reason - to let you run Wicket behind a mod_proxy without having lots of
pain. Additionally, you need the URLs in generated AJAX fragments to be
relative to the page that the request was made from, not relative to the
request itself. Your patch needs to make sure this behaves properly.
> Delaying portlet-support to after the 1.3 release really would be a
> shame in this light in my view.
Yes, I agree. It's really hard to maintain a patchset of this size, and
squeezing it out to 1.4 (or whatever we're calling the next version)
would make for unnecessary work if we can get it into 1.3 with a
guarantee that it's not going to break things.
I also suspect that we have a bunch of 1.2.x portlet users who we're
going to leave high-and-dry otherwise, which is certainly a consideration.
> If anyone of you still have reservations and/or major objections of
> merging portlet-support in the current trunk, please give it head and
> toe and indicate which changes you think are questionable so we can
> properly discuss them.
I'll have a proper in-depth review of your patch later in the week and
let you know.
Regards,
Al
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Johan Compagner <jc...@gmail.com>.
i am +1 to drop it into beta4, Then if somehow we encounter many bugs
because of that we could undo it
But don't think this is really needed.
Portlet support is also a nice marketing yell
johan
On 9/18/07, Ate Douma <at...@douma.nu> wrote:
>
> Guys,
>
>
> I'd like to respond here on the concerns raised by Al, Gwyn and Kent on
> the "Re: [jira] Resolved: (WICKET-647) New Wicket Portlet support" thread.
>
> As I created a dedicated JIRA issue, WICKET-983, for this as well as this
> specific mail subject, I think we should try to centralize the discussion
> about it here.
>
> First of all, thanks a lot for taking the time to look into this and
> review the changes I propose for adding portlet-support.
>
> I do understand the concerns raised but I'd like to point out that, at
> least in my view, the impact for *normal* wicket runtime (e.g. servlet,
> not portlet) the
> changes really are very, very minimal.
>
> Al, Gwyn and Kent: you indicate that you see major and/or fundamental
> changes to wicket, but please make explicit which changes you are referring
> to.
> Now I only have something like "the portlet changes /do/ touch some
> fundamental bits of Wicket", "making major changes to the codebase" and "It
> seems that
> those changes are quite fundamental" to respond to.
>
> Anyway, I'll try to make clear why in my view the portlet changes aren't
> going to cause "major" or "fundamental" changes in the core wicket.
>
> If you evaluate the issues I listed in WICKET-983 and how I resolved
> those, you'll see that the changes which *might* have a small to zero effect
> on the
> standard wicket behavior are the following:
>
> 1) WICKET-649: fix appending query parameters
> a) replacing blindly appending "&" with a check if it really
> should be a "?"
> In my view harmless and actually really improving wicket quality
> itself
>
> b) moving AbstractAjaxBehavior.getCallbackUrl() appending
> "wicket:ignoreIfNotActive=true" to WebRequestCodingStrategy.encode().
> This is a quite simple reordering of internal processing with exactly
> the same functionality and result as outcome, quite harmless I think
>
> c) wicket-ajax Wicket.Ajax.Request.get(path) checking on query
> parameters and in that case replacing it with a Request.post(body) call
> This one I think can/should be discussed as it really does result in a
> *technically* different behavior.
> I do think in the case of Ajax requests, this really doesn't matter as
> it won't effect the browser state at all, and my solution really is
> most transparent one I could think of.
> But, as it also changes the behavior in a plain servlet environment,
> and as such it I could understand people would feel a bit uncomfortable with
> it.
> I already suggest in the description for this change I'm open to other
> solutions, like only doing this when running in a portlet environment.
> Then, this issue would become moot for the servlet environment, we
> just need to find some way to indicate the current environment
> (servlet|portlet)
> in javascript to the browser.
>
> d) IOnChangeListener components with
> wantOnSelectionChangedNotifications()==true
> If you review my changes you'll find out there are *zero* changes when
> running in a servlet environment, as is with almost all other changes.
> Based on the boolean evaluation of RequestContext.isPortletRequest(),
> which is always false in a servlet environment as you can imagine, only the
> original
> code is executed. Only the portlet specific behavior is "wrapped" in a
> simple if (RequestContext.isPortletRequest())... nothing more.
>
> 2) WICKET-650: namespacing component markupId
> This change does nothing when not running in a portlet environment
>
> 3) WICKET-651: extending IHeaderResponse
> This change only adds a close() and isClosed() method to IHeaderResponse
> and delegates getResponse() to a getRealResponse() implementation.
> These changes also have no side-effect, all current behavior is
> maintained. It just allows capturing the HeaderResponse when running in a
> portlet context.
> It also allows you now to do the same in a servlet environment if you
> would want that, so it is an expansion of the api but it is not used other
> than from
> portlet specific code.
>
> 4) WICKET-657: upgrading wicket-examples to require servlet api 2.4
> I don't consider this a "major" change. For one, its just an example and
> not touching wicket core at all. Furthermore, I doubt there will be many
> users still
> stuck on a servlet 2.3 container, especially as wicket-examples already
> requires Java 1.5 anyway.
> Anyway, this change isn't critical for merging the portlet support in the
> trunk but for demonstration purposes it would be nice to be able to use it.
>
> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback
> handling
> Actually, I would consider proposing this change regardless of the
> discussion of portlet-support. The current hard coded expectancy an Ajax
> callback redirect
> can and may only be a Wicket relative url doesn't seem very flexible. For
> the portlet-support this change is needed though.
> And, if the redirect url *is* relative as expected in the current
> implementation, my changes won't do anything anyway.
> But if there are major objections to it, we could discuss solving it in a
> similar way as we could do with the Ajax.Request.get(path) -> .post(body)
> change.
> If we can push the current runtime environment (servlet|portlet) to the
> browser the ajax script could maintain the current behavior when running in
> a servlet.
>
> 6) WICKET-926: recognizing popup/detached pages urlFor calls
> This change also does nothing when not running in a portlet environment.
> But I did listed in WICKET-983 because the solution I chose didn't exactly
> result in
> "beautiful" code... I couldn't come up with something else though which
> would be just as transparent and without *any* side-effects.
> So, I'm actually hoping someone calls me stupid and comes up with a more
> "feel good" solution :)
>
> That's it. All other changes to existing wicket classes will only come
> into effect when run in a portlet environment. When running in a servlet
> environment
> Wicket behaves exactly as before!
>
> Concluding: in my view the only real "issue" we might want to discuss is
> the Ajax.Request.get(path) -> .post(body) change.
> I personally don't see it as an issue, but if that change would be viewed
> as unacceptable/dangerous for the current trunk, it should not be difficult
> to come up
> with another solution which will keep the current behavior in a servlet
> environment (although that then will have to be a little bit less
> transparent I'm afraid).
>
> So, if this assessment is valid, I really don't see why portlet support
> couldn't be integrated into the 1.3.0 trunk.
> Again, I do understand the concerns raised, but don't forget quite a few
> major changes (like for bugfixes, but probably not even only for that) have
> been
> committed, and still are committed to the trunk too. Some of those might
> have (much) more side-effects and consequences then adding
> portlet-support...
>
> Delaying portlet-support to after the 1.3 release really would be a shame
> in this light in my view.
> For those of us who don't need/care about portlet it won't matter, but I
> know quite a few developers and project teams are eager to be able to use
> Wicket for
> portlet development. Waiting until the 1.4 branch stabilizes really means
> cutting them out.
> Causing a delay of the 1.3 release isn't good for the community (and I
> don't think should be needed) but delaying portlet-support until after the
> 1.3 release
> isn't good for the community either.
>
> I do hope my above explanations make it a bit clearer what the real issues
> are to discuss.
> If anyone of you still have reservations and/or major objections of
> merging portlet-support in the current trunk, please give it head and toe
> and indicate which
> changes you think are questionable so we can properly discuss them.
>
> Regards,
>
> Ate
>
> Ate Douma wrote:
> > I've created https://issues.apache.org/jira/browse/WICKET-983 to help
> > out reviewing the portlet-support changes.
> >
> > Although I initially planned to create separate "sub" patches for
> > reviewing the important wicket changes individually, this turned out to
> > be a
> > rather impossible task. The complete size of the full patch is almost
> > 200k and several different issues I needed to solve concerned the same
> > classes/files. Breaking all that up in digestible peaces would cost just
> > to much time to be realistic.
> >
> > I did however upload a full patch file and provide a short summary of
> > the most important changes with links to the specific subtask issues of
> > WICKET-647.
> > These subtasks provide more detailed descriptions of the issues at hand
> > as well as the commit logs of the changes I made.
> >
> > All committers or otherwise interested: please take some time to at
> > least read the description of the problems I needed to solve.
> > If you see anything you don't like or would have chosen a different
> > solution, please bring it up here so we can discuss it.
> >
> > I would like to merge the portlet support branch into the trunk before
> > the -beta4 cutoff, so your input on this is very much appreciated.
> >
> > Thanks,
> >
> > Ate Douma
> >
> >
> >
>
>
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Frank Bille <fr...@apache.org>.
On 9/20/07, Frank Bille <fr...@apache.org> wrote:
>
> I don't have time to release this weekend anyway, so it's fine to do the
> vote after the weekend. Then we start the vote on Monday, you can merge (if
> thats what we decide of cause) in the middle of next week and I can release
> beta4 next weekend. In that way we quickly get some testers on it.
Or we can vote now, and let it last till Monday :) (Didn't see there already
was a vote thread)
Frank
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Frank Bille <fr...@apache.org>.
On 9/20/07, Ate Douma <at...@douma.nu> wrote:
>
> Frank Bille wrote:
> > I think we should wrap this up and hold a vote on how to proceed with
> > portlets. As I see it there are two options:
> >
> > 1) Merge portlets into trunk now and delay the final 1.3 release until
> the
> > dust has settled.
> > 2) Delay portlets support until 1.4.
> >
> > WDYT?
> +1 from me, but I'm not sure everybody is ready to anwser a vote yet.
> At least, Al indicated he wanted to review the changes in more detail but
> wouldn't have time before end of this week/this weekend.
> Not sure what his view on this is now and when the -beta4 cutoff is going
> to be.
> I do need at least one day if the vote result is to include it into trunk
> now as I already discovered I cannot automatically apply my changes anymore.
>
> But yes, I'm willing to call a vote as soon as everyone is willing and
> ready for it.
>
I don't have time to release this weekend anyway, so it's fine to do the
vote after the weekend. Then we start the vote on Monday, you can merge (if
thats what we decide of cause) in the middle of next week and I can release
beta4 next weekend. In that way we quickly get some testers on it.
Frank
Re: Please review WICKET-983: Merge the portlet support branch into
the trunk
Posted by Ate Douma <at...@douma.nu>.
Frank Bille wrote:
> I think we should wrap this up and hold a vote on how to proceed with
> portlets. As I see it there are two options:
>
> 1) Merge portlets into trunk now and delay the final 1.3 release until the
> dust has settled.
> 2) Delay portlets support until 1.4.
>
> WDYT?
+1 from me, but I'm not sure everybody is ready to anwser a vote yet.
At least, Al indicated he wanted to review the changes in more detail but wouldn't have time before end of this week/this weekend.
Not sure what his view on this is now and when the -beta4 cutoff is going to be.
I do need at least one day if the vote result is to include it into trunk now as I already discovered I cannot automatically apply my changes anymore.
But yes, I'm willing to call a vote as soon as everyone is willing and ready for it.
Ate
>
> Frank
>
>
> On 9/19/07, Gwyn Evans <gw...@gmail.com> wrote:
>> On Wednesday, September 19, 2007, 12:12:16 AM, Ate <at...@douma.nu> wrote:
>>
>>> Since -beta3, there have been several other commits to the trunk
>>> which also affect critical areas like url handling: should those not
>> have been applied either?
>>
>> Probably not! :-)
>>
>>> The amount of changes to the existing code isn't very big and won't cost
>> you much time to review.
>>> If you're willing to do so, I'd suggest checking out the -beta3
>>> release and apply the patch file I attached to WICKET-983. Then, using
>> the Eclipse Team
>>> Synchronization Perspective, you can get a quick view on all the
>> changes, within context.
>>
>> I used IDEA, but yes - the only issues I had were the changed test
>> expectations in jdk-1.4\wicket\src\test\java\org\apache\wicket\ajax\
>> markup\html\componentMap\SimpleTestPageExpectedResult.html &
>> SimpleTestPageExpectedResult-1.html, where the change in
>> AbstractAjaxBehaviour.java results in the test page having a
>> IActivePageBehaviorListener rather than a IBehaviorListener.
>>
>>> I also provided a build wicket-examples.war based on the
>>> portlet-support branch for testing purposes which you can deploy in any
>> servlet container:
>>> http://people.apache.org/~ate/wicket/wicket-examples.war
>> I used the one I built, but no problems found.
>>
>>>> At the moment I'd have a "-0.2" position on things, i.e. slightly
>>>> negative, as I want to get a 1.3 release out and have concerns that
>>>> this might delay things, but open to persuasion & would welcome some
>>>> input from someone more familiar with the core than I am who might be
>>>> able to comment as to if my concerns are likely to be unfounded.
>> Well, I can't see any problems, no one else has raised anything &
>> we're still getting a reasonable rate of issues being raised/fixed on
>> trunk, so I'm happy for you to apply the changes now... it's cost you
>> though! :-)
>>
>> How about, once this is all in & working OK, about doing a couple of
>> wiki articles - One "Hello Portal" for users, describing how to get
>> started with Wicket and an example portal server, and a second for the
>> developers, flagging things to watch out for when coding the core?
>>
>> /Gwyn
>>
>>
>
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Frank Bille <fr...@apache.org>.
I think we should wrap this up and hold a vote on how to proceed with
portlets. As I see it there are two options:
1) Merge portlets into trunk now and delay the final 1.3 release until the
dust has settled.
2) Delay portlets support until 1.4.
WDYT?
Frank
On 9/19/07, Gwyn Evans <gw...@gmail.com> wrote:
>
> On Wednesday, September 19, 2007, 12:12:16 AM, Ate <at...@douma.nu> wrote:
>
> > Since -beta3, there have been several other commits to the trunk
> > which also affect critical areas like url handling: should those not
> have been applied either?
>
> Probably not! :-)
>
> > The amount of changes to the existing code isn't very big and won't cost
> you much time to review.
> > If you're willing to do so, I'd suggest checking out the -beta3
> > release and apply the patch file I attached to WICKET-983. Then, using
> the Eclipse Team
> > Synchronization Perspective, you can get a quick view on all the
> changes, within context.
>
> I used IDEA, but yes - the only issues I had were the changed test
> expectations in jdk-1.4\wicket\src\test\java\org\apache\wicket\ajax\
> markup\html\componentMap\SimpleTestPageExpectedResult.html &
> SimpleTestPageExpectedResult-1.html, where the change in
> AbstractAjaxBehaviour.java results in the test page having a
> IActivePageBehaviorListener rather than a IBehaviorListener.
>
> > I also provided a build wicket-examples.war based on the
> > portlet-support branch for testing purposes which you can deploy in any
> servlet container:
> > http://people.apache.org/~ate/wicket/wicket-examples.war
>
> I used the one I built, but no problems found.
>
> >> At the moment I'd have a "-0.2" position on things, i.e. slightly
> >> negative, as I want to get a 1.3 release out and have concerns that
> >> this might delay things, but open to persuasion & would welcome some
> >> input from someone more familiar with the core than I am who might be
> >> able to comment as to if my concerns are likely to be unfounded.
>
> Well, I can't see any problems, no one else has raised anything &
> we're still getting a reasonable rate of issues being raised/fixed on
> trunk, so I'm happy for you to apply the changes now... it's cost you
> though! :-)
>
> How about, once this is all in & working OK, about doing a couple of
> wiki articles - One "Hello Portal" for users, describing how to get
> started with Wicket and an example portal server, and a second for the
> developers, flagging things to watch out for when coding the core?
>
> /Gwyn
>
>
Re: Please review WICKET-983: Merge the portlet support branch into
the trunk
Posted by Ate Douma <at...@douma.nu>.
Gwyn Evans wrote:
> On Wednesday, September 19, 2007, 12:12:16 AM, Ate <at...@douma.nu> wrote:
>
>> Since -beta3, there have been several other commits to the trunk
>> which also affect critical areas like url handling: should those not have been applied either?
>
> Probably not! :-)
>
>> The amount of changes to the existing code isn't very big and won't cost you much time to review.
>> If you're willing to do so, I'd suggest checking out the -beta3
>> release and apply the patch file I attached to WICKET-983. Then, using the Eclipse Team
>> Synchronization Perspective, you can get a quick view on all the changes, within context.
>
> I used IDEA, but yes - the only issues I had were the changed test
> expectations in jdk-1.4\wicket\src\test\java\org\apache\wicket\ajax\
> markup\html\componentMap\SimpleTestPageExpectedResult.html &
> SimpleTestPageExpectedResult-1.html, where the change in
> AbstractAjaxBehaviour.java results in the test page having a
> IActivePageBehaviorListener rather than a IBehaviorListener.
Ouch, I missed that one.
Strangely enough though this test doesn't fail when run during the build of wicket, but when invoked directly it does.
Weird.
It can easily be fixed of course which I'll do in a minute.
>
>> I also provided a build wicket-examples.war based on the
>> portlet-support branch for testing purposes which you can deploy in any servlet container:
>> http://people.apache.org/~ate/wicket/wicket-examples.war
>
> I used the one I built, but no problems found.
Cool, good to hear.
>
>>> At the moment I'd have a "-0.2" position on things, i.e. slightly
>>> negative, as I want to get a 1.3 release out and have concerns that
>>> this might delay things, but open to persuasion & would welcome some
>>> input from someone more familiar with the core than I am who might be
>>> able to comment as to if my concerns are likely to be unfounded.
>
> Well, I can't see any problems, no one else has raised anything &
> we're still getting a reasonable rate of issues being raised/fixed on
> trunk, so I'm happy for you to apply the changes now... it's cost you
> though! :-)
LOL, I knew this was coming ;)
>
> How about, once this is all in & working OK, about doing a couple of
> wiki articles - One "Hello Portal" for users, describing how to get
> started with Wicket and an example portal server, and a second for the
> developers, flagging things to watch out for when coding the core?
Yes, I will. I already had that on my todo list.
Thanks for the support,
Ate
>
> /Gwyn
>
>
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Gwyn Evans <gw...@gmail.com>.
On Wednesday, September 19, 2007, 12:12:16 AM, Ate <at...@douma.nu> wrote:
> Since -beta3, there have been several other commits to the trunk
> which also affect critical areas like url handling: should those not have been applied either?
Probably not! :-)
> The amount of changes to the existing code isn't very big and won't cost you much time to review.
> If you're willing to do so, I'd suggest checking out the -beta3
> release and apply the patch file I attached to WICKET-983. Then, using the Eclipse Team
> Synchronization Perspective, you can get a quick view on all the changes, within context.
I used IDEA, but yes - the only issues I had were the changed test
expectations in jdk-1.4\wicket\src\test\java\org\apache\wicket\ajax\
markup\html\componentMap\SimpleTestPageExpectedResult.html &
SimpleTestPageExpectedResult-1.html, where the change in
AbstractAjaxBehaviour.java results in the test page having a
IActivePageBehaviorListener rather than a IBehaviorListener.
> I also provided a build wicket-examples.war based on the
> portlet-support branch for testing purposes which you can deploy in any servlet container:
> http://people.apache.org/~ate/wicket/wicket-examples.war
I used the one I built, but no problems found.
>> At the moment I'd have a "-0.2" position on things, i.e. slightly
>> negative, as I want to get a 1.3 release out and have concerns that
>> this might delay things, but open to persuasion & would welcome some
>> input from someone more familiar with the core than I am who might be
>> able to comment as to if my concerns are likely to be unfounded.
Well, I can't see any problems, no one else has raised anything &
we're still getting a reasonable rate of issues being raised/fixed on
trunk, so I'm happy for you to apply the changes now... it's cost you
though! :-)
How about, once this is all in & working OK, about doing a couple of
wiki articles - One "Hello Portal" for users, describing how to get
started with Wicket and an example portal server, and a second for the
developers, flagging things to watch out for when coding the core?
/Gwyn
Re: Please review WICKET-983: Merge the portlet support branch into
the trunk
Posted by Ate Douma <at...@douma.nu>.
Gwyn Evans wrote:
> On Tuesday, September 18, 2007, 1:14:11 PM, Ate <at...@douma.nu> wrote:
>
>> b) moving AbstractAjaxBehavior.getCallbackUrl() appending
>> "wicket:ignoreIfNotActive=true" to WebRequestCodingStrategy.encode().
>> This is a quite simple reordering of internal processing with
>> exactly the same functionality and result as outcome, quite harmless I think
>
>> c) wicket-ajax Wicket.Ajax.Request.get(path) checking on query
>> parameters and in that case replacing it with a Request.post(body) call
>> This one I think can/should be discussed as it really does
>> result in a *technically* different behavior.
>> I do think in the case of Ajax requests, this really doesn't
>> matter as it won't effect the browser state at all, and my solution really is
>> most transparent one I could think of.
>> But, as it also changes the behavior in a plain servlet
>> environment, and as such it I could understand people would feel a bit uncomfortable with it.
>> I already suggest in the description for this change I'm open
>> to other solutions, like only doing this when running in a portlet environment.
>> Then, this issue would become moot for the servlet environment,
>> we just need to find some way to indicate the current environment (servlet|portlet)
>> in javascript to the browser.
>
>
>> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback handling
>> Actually, I would consider proposing this change regardless of the
>> discussion of portlet-support. The current hard coded expectancy an Ajax callback redirect
>> can and may only be a Wicket relative url doesn't seem very
>> flexible. For the portlet-support this change is needed though.
>> And, if the redirect url *is* relative as expected in the current
>> implementation, my changes won't do anything anyway.
>> But if there are major objections to it, we could discuss solving
>> it in a similar way as we could do with the Ajax.Request.get(path) -> .post(body) change.
>> If we can push the current runtime environment (servlet|portlet) to
>> the browser the ajax script could maintain the current behavior when running in a servlet.
>
>
> I think the areas that I'm concerned about are primarily these ones,
> not with respect to the specific changes themselves, but more of a
> concern as to whether we should be changing the trunk right now & how
> much of an impact these would have on non-portlet specific codepaths.
Gwyn,
The only thing I can say about this is: please do review the actual changes I propose and see for yourself the amount of impact they have on non-portlet codepaths.
As I've stated before, these changes are nothing more than quite simple "bug" fixes from a portlet perspective, and I really, really did my best to make them
cause no side-effects in a non-portlet environment.
Since -beta3, there have been several other commits to the trunk which also affect critical areas like url handling: should those not have been applied either?
I'm not questioning those to be sure, but please do (re)view my proposed changes in this perspective.
Of the proposed changes you highlighted above, I think only the Ajax.Request.get(path) -> Ajax.Request.post(body) change might be considered to potentially have
a side-effect yet unknown. I don't expect it, but of course there's always a chance.
But the same can be said for many other changes/bugfixes committed since -beta3 and that doesn't hold us back either, does it?
We review them and if need be question them, but in general accept them if it does improve Wicket and we don't see a danger of it breaking things.
I won't (nor can) say my changes are 100% perfect and guaranteed not going to break anything, but then: who can?
I'm just asking for you and the others to review my changes in the same light, and if you actually do see danger in it breaking stuff, then I'm all ears and
definitely willing to try to fix it. And if needed even pull back my proposal if I can't.
But without concrete information where you or the others do see potential problems, it is difficult for me to answer the type of concerns you raise, especially
as I don't see any of those problems myself yet.
> I guess the specific concern relates to whether we have sufficient
> confidence, whether or not that's via unit test, to be happy with the
> changes.
I think that really is the important question here.
And I don't know what else I can do or say right now to provide you and the others more confidence other then just asking for a proper review of my changes and
testing it out.
The amount of changes to the existing code isn't very big and won't cost you much time to review.
If you're willing to do so, I'd suggest checking out the -beta3 release and apply the patch file I attached to WICKET-983. Then, using the Eclipse Team
Synchronization Perspective, you can get a quick view on all the changes, within context.
I also provided a build wicket-examples.war based on the portlet-support branch for testing purposes which you can deploy in any servlet container:
http://people.apache.org/~ate/wicket/wicket-examples.war
HTH,
Ate
>
> At the moment I'd have a "-0.2" position on things, i.e. slightly
> negative, as I want to get a 1.3 release out and have concerns that
> this might delay things, but open to persuasion & would welcome some
> input from someone more familiar with the core than I am who might be
> able to comment as to if my concerns are likely to be unfounded.
>
> /Gwyn
>
>
>
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Eelco Hillenius <ee...@gmail.com>.
On 9/18/07, Gwyn Evans <gw...@gmail.com> wrote:
> On Tuesday, September 18, 2007, 1:14:11 PM, Ate <at...@douma.nu> wrote:
>
> > b) moving AbstractAjaxBehavior.getCallbackUrl() appending
> > "wicket:ignoreIfNotActive=true" to WebRequestCodingStrategy.encode().
> > This is a quite simple reordering of internal processing with
> > exactly the same functionality and result as outcome, quite harmless I think
>
> > c) wicket-ajax Wicket.Ajax.Request.get(path) checking on query
> > parameters and in that case replacing it with a Request.post(body) call
> > This one I think can/should be discussed as it really does
> > result in a *technically* different behavior.
> > I do think in the case of Ajax requests, this really doesn't
> > matter as it won't effect the browser state at all, and my solution really is
> > most transparent one I could think of.
> > But, as it also changes the behavior in a plain servlet
> > environment, and as such it I could understand people would feel a bit uncomfortable with it.
> > I already suggest in the description for this change I'm open
> > to other solutions, like only doing this when running in a portlet environment.
> > Then, this issue would become moot for the servlet environment,
> > we just need to find some way to indicate the current environment (servlet|portlet)
> > in javascript to the browser.
>
>
> > 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback handling
> > Actually, I would consider proposing this change regardless of the
> > discussion of portlet-support. The current hard coded expectancy an Ajax callback redirect
> > can and may only be a Wicket relative url doesn't seem very
> > flexible. For the portlet-support this change is needed though.
> > And, if the redirect url *is* relative as expected in the current
> > implementation, my changes won't do anything anyway.
> > But if there are major objections to it, we could discuss solving
> > it in a similar way as we could do with the Ajax.Request.get(path) -> .post(body) change.
> > If we can push the current runtime environment (servlet|portlet) to
> > the browser the ajax script could maintain the current behavior when running in a servlet.
>
>
> I think the areas that I'm concerned about are primarily these ones,
> not with respect to the specific changes themselves, but more of a
> concern as to whether we should be changing the trunk right now & how
> much of an impact these would have on non-portlet specific codepaths.
> I guess the specific concern relates to whether we have sufficient
> confidence, whether or not that's via unit test, to be happy with the
> changes.
I share that concern. But the best thing we can do here is write
better/ more unit tests for the URL handling. We should do that
regardless of this proposal really.
Eelco
Re: Please review WICKET-983: Merge the portlet support branch into the trunk
Posted by Gwyn Evans <gw...@gmail.com>.
On Tuesday, September 18, 2007, 1:14:11 PM, Ate <at...@douma.nu> wrote:
> b) moving AbstractAjaxBehavior.getCallbackUrl() appending
> "wicket:ignoreIfNotActive=true" to WebRequestCodingStrategy.encode().
> This is a quite simple reordering of internal processing with
> exactly the same functionality and result as outcome, quite harmless I think
> c) wicket-ajax Wicket.Ajax.Request.get(path) checking on query
> parameters and in that case replacing it with a Request.post(body) call
> This one I think can/should be discussed as it really does
> result in a *technically* different behavior.
> I do think in the case of Ajax requests, this really doesn't
> matter as it won't effect the browser state at all, and my solution really is
> most transparent one I could think of.
> But, as it also changes the behavior in a plain servlet
> environment, and as such it I could understand people would feel a bit uncomfortable with it.
> I already suggest in the description for this change I'm open
> to other solutions, like only doing this when running in a portlet environment.
> Then, this issue would become moot for the servlet environment,
> we just need to find some way to indicate the current environment (servlet|portlet)
> in javascript to the browser.
> 5) WICKET-924: non-relative urls in Ajax.Request.redirect callback handling
> Actually, I would consider proposing this change regardless of the
> discussion of portlet-support. The current hard coded expectancy an Ajax callback redirect
> can and may only be a Wicket relative url doesn't seem very
> flexible. For the portlet-support this change is needed though.
> And, if the redirect url *is* relative as expected in the current
> implementation, my changes won't do anything anyway.
> But if there are major objections to it, we could discuss solving
> it in a similar way as we could do with the Ajax.Request.get(path) -> .post(body) change.
> If we can push the current runtime environment (servlet|portlet) to
> the browser the ajax script could maintain the current behavior when running in a servlet.
I think the areas that I'm concerned about are primarily these ones,
not with respect to the specific changes themselves, but more of a
concern as to whether we should be changing the trunk right now & how
much of an impact these would have on non-portlet specific codepaths.
I guess the specific concern relates to whether we have sufficient
confidence, whether or not that's via unit test, to be happy with the
changes.
At the moment I'd have a "-0.2" position on things, i.e. slightly
negative, as I want to get a 1.3 release out and have concerns that
this might delay things, but open to persuasion & would welcome some
input from someone more familiar with the core than I am who might be
able to comment as to if my concerns are likely to be unfounded.
/Gwyn