Juergen, thanks for the review. I modified the code according to your suggestions.
> - EnclosureHandler: A variable to increment the id postfix will not > work. A fresh copy of MarkupParser and all its handlers is created for > every markup file (page, panel, border, etc.). Your increment will > thus only be unique within the a single markup file. Your page however > may have many Panels.... Good point, I fixed this by changing the simple variable to the sequence value from Session class, making the id's session-unique. I also added an unit test to verify this. > - you added code to EnclosureHandler and EnclosureResolver and > Enclosure. Tinkering a bit with the code, I think having an > InlineEnclosureHandler and InlineEnclosure keeps them nicely separated > and thus cleaner. InlineEnclosureHandler has only very little in > common with EnclosureHandler and could be separate, whereas > InlineEnclosure is probably better derived from Enclosure. Right, I created a new InlineEnclosureHandler class and removed all the changes from EnclosureHandler. I also created a new InlineEnclosure class, extending Enclosure. Now the only modification in the Enclosure-class is that the visibility of the method getEnclosureParent() is relaxed to protected (was private), since the extending InlineEnclosure class uses the method as well. I uploaded the new patch to JIRA. https://issues.apache.org/jira/browse/WICKET-3422 - Joonas On Sun, Feb 20, 2011 at 11:16 PM, Juergen Donnerstag < [email protected]> wrote: > I did a first review. > > - EnclosureHandler: A variable to increment the id postfix will not > work. A fresh copy of MarkupParser and all its handlers is created for > every markup file (page, panel, border, etc.). Your increment will > thus only be unique within the a single markup file. Your page however > may have many Panels. That wouldn't be a problem for Wicket because of > its hierarchy, but HTML IDs must be unique on the page. > Markup parsing / loading is completely decoupled from wickets > components hierarchy, there is no Page available. That linkage happens > in Resolvers. See the EnclosureResolver for an example. > > - besides that I think its poor practice to introduce a dependency on > Enclosure in MarkupContainer, auto-added components must be removed > after the render process as you otherwise keep on adding new ones and > your are not freeing them up. I don't understand anyway why you did > this change, since Enclosure don't have any state. > > - you added code to EnclosureHandler and EnclosureResolver and > Enclosure. Tinkering a bit with the code, I think having an > InlineEnclosureHandler and InlineEnclosure keeps them nicely separated > and thus cleaner. InlineEnclosureHandler has only very little in > common with EnclosureHandler and could be separate, whereas > InlineEnclosure is probably better derived from Enclosure. > > > -Juergen > > On Sun, Feb 20, 2011 at 5:51 AM, Joo Hama <[email protected]> > wrote: > >> the problem with the tag is that it is not rendered into the markup so > >> cannot be targetted via ajax. > >> > >> -igor > > > > > > Yes, this is exactly the point. We cannot render Enclosure tags without > > breaking html, > > for example inside a <table>. If we cannot render it, we cannot target it > > with ajax. When we > > use an Enclosure as an attribute, this limitation is lifted, since we use > an > > existing html tag, > > for example <tr wicket:enclosure="child:label1">, which gets naturally > > rendered without > > breaking the html. Also, the Enclosure as attribute doesn't get removed, > > thus it can have state. > > So now we have a stateful Enclosure enclosing a section of the html, and > > it's visibility can be toggled > > through ajax calls that toggle the visibility of it's designated child > > object. > > > > About the amount of boilerplate code to create an ajax-toggleable > enclosure: > > > > HTML: (excluding closing tags) > > --------------- > > Before: > > <wicket:enclosure child="enclosureContainer:toggleable"> > > <tr wicket:id="enclosureContainer""> > > ...... > > After: > > <tr wicket:enclosure="child:toggleable"> > > ....... > > > > Java: (Excluding toggling mechanism) > > ------------------ > > Before: > > WebMarkupContainer enclosureContainer = new > > WebMarkupContainer("enclosureContainer"){ > > @Override > > public boolean isVisible() { > > return get(toggleable).isVisible(); > > } > > }).setOutputMarkupPlaceholderTag(true)); > > add(enclosureContainer); > > .......... > > After: > > <nothing, all this is automatic with the enclosure as attribute> > > .......... > > > > > > - Joonas > > > > > > On Sun, Feb 20, 2011 at 7:35 AM, Igor Vaynberg <[email protected] > >wrote: > > > >> On Sat, Feb 19, 2011 at 2:20 PM, Juergen Donnerstag > >> <[email protected]> wrote: > >> > A couple of comments on previous posts > >> > - you can always assign your own wicket:id like <wicket:enclosure > >> > wicket:id="myEnclosure" child="label1"> > >> > - yes, <wicket:enclosure> creates an auto component (no java code > >> > necessary) that is auto-added to the container which at that point in > >> > time gets rendered, when during the render process the markup stream > >> > hits the enclosure tag . And it gets automatically removed again which > >> > means you can not have any state with it. > >> > > >> > I looked at the patch and have a couple questions > >> > > >> > It seems you try to achieve 2 very different things: > >> > > >> > a) "Changing the visibility of a child component in Ajax callback > >> > method will not affect the entire enclosure but just the child > >> > component itself. This is because only the child component is added to > >> > the AjaxRequestTarget." > >> > > >> > what is the limitation here? Enclosure checks the childComponent's > >> > visibility before it renders the Enclosure. At the end that's the > >> > whole idea of Enclosure. You never explicity change the Enclosure's > >> > visibility. You always (with and without ajax) change the visibility > >> > of the child component. Do I misunderstand something? > >> > > >> > b) you prefer a wicket:enclosure attribute over a wicket:enclosure tag > >> > you prefer <div wicket:enclosure="child:label1"> over > >> > <wicket:enclosure child="label1"> > >> > Neither requires any java code. Neither is really longer or shorter > >> > than the other. Neither requires more or less boilerplate. It seems to > >> > me a matter of personal preference. That's up to the community what > >> > you like. We can have either or both. I'm personally more in favour of > >> > the tag, but that's of course only my opinion. > >> > >> the problem with the tag is that it is not rendered into the markup so > >> cannot be targetted via ajax. > >> > >> -igor > >> > >> > > >> > - Juergen > >> > > >> > > >> > On Tue, Feb 8, 2011 at 7:34 AM, Igor Vaynberg < > [email protected]> > >> wrote: > >> >> i will try to look over it in the few coming days and give you some > >> feedback. > >> >> > >> >> -igor > >> >> > >> >> On Mon, Feb 7, 2011 at 3:43 AM, Joo Hama < > [email protected]> > >> wrote: > >> >>> Thanks Igor and Jeremy, > >> >>> > >> >>> I managed to resolve these issues and committed a patch to JIRA: > >> >>> > >> >>> https://issues.apache.org/jira/browse/WICKET-3422 > >> >>> > >> >>> It introduces an "inline" Enclosure defined as an attribute of a > >> >>> html tag, and a listener to find the right inline Enclosures at the > >> >>> time of Ajax request handling. It seems to work well with the > existing > >> >>> wicket > >> >>> code base, and all tests are green. What do you guys think? Is it > >> suitable > >> >>> material to commit to wicket? > >> >>> > >> >>> - Joonas > >> >>> <https://issues.apache.org/jira/browse/WICKET-3422> > >> >>> > >> >>> On Fri, Jan 21, 2011 at 8:41 PM, Joo Hama < > [email protected] > >> >wrote: > >> >>> > >> >>>> > >> >>>> The EnclosureResolver... container.autoAdd seems to be invoked only > in > >> the > >> >>>> parsing > >> >>>> phase of the original request, whereas the onBeforeRender is > invoked > >> during > >> >>>> handling > >> >>>> of the AjaxRequest (when the listener is attached to the > >> >>>> AjaxRequestTarget). > >> >>>> > >> >>>> I found out that the Enclosure is not a child of a Page, thus > making > >> it > >> >>>> quite elusive > >> >>>> to get hold of in the onBeforeRespond event. I even attempted to > >> >>>> read MarkupStream, > >> >>>> and indeed found the tag there. (source below) However, in order to > >> add it > >> >>>> to the requestTarget, we > >> >>>> need to get hold of the component. I tried to read the path of the > tag > >> in > >> >>>> the document > >> >>>> using ComponentTag.getPath(), but it was null. This is where i hit > >> brick > >> >>>> wall. > >> >>>> > >> >>>> Is it possible to get hold of the Enclosure component with only > >> >>>> the WicketTag from the > >> >>>> MarkupStream as reference? If not, i think this approach might not > be > >> >>>> viable. > >> >>>> > >> >>>> > >> >>>> // Override of the application class method newAjaxRequestTarget, > >> trying to > >> >>>> find enclosures that have > >> >>>> // the child id of a component > >> >>>> // in the ajaxTarget: > >> >>>> //----------------------- > >> >>>> @Override > >> >>>> public AjaxRequestTarget newAjaxRequestTarget(Page page) { > >> >>>> AjaxRequestTarget target = super.newAjaxRequestTarget(page); > >> >>>> target.addListener(new AjaxRequestTarget.IListener() { > >> >>>> > >> >>>> public void onBeforeRespond(Map<String, Component> map, > >> >>>> AjaxRequestTarget target) { List<String> ids = new > >> ArrayList<String>(); > >> >>>> Page page = null; // first read all the component id:s from the > target > >> for > >> >>>> (Component component : map.values()) { page = component.getPage(); > >> >>>> ids.add(component.getId()); } // then traverse the markupStream to > >> find > >> >>>> enclosureTags if (page != null) { MarkupStream stream = > >> >>>> page.getMarkupStream(); stream.setCurrentIndex(0); while > >> (stream.hasMore()) > >> >>>> { stream.skipRawMarkup(); if (stream.hasMore()) { WicketTag tag = > new > >> >>>> WicketTag(stream.getTag()); if (tag.isEnclosureTag()) { // Try to > >> match the > >> >>>> enclosureTag:s child id to the ids in the target String childId = > >> >>>> tag.getAttribute("child"); if (childId != null) { String[] parts = > >> >>>> childId.split(":"); childId = parts[parts.length - 1]; for (String > id > >> : ids) > >> >>>> { if (id.equals(childId)) { System.err .println("Found enclosure " > + > >> >>>> tag.getId() + " at path:" + tag.getPath() + ", who's child is > targeted > >> Id=" > >> >>>> + childId); } } } } stream.next(); } } } } > >> >>>> public void onAfterRespond(Map<String, Component> map, > >> IJavascriptResponse > >> >>>> response) { System.err.println("onAfterRespond"); } }); return > target; > >> } > >> >>>> //------------- > >> >>>> > >> >>>> > >> >>>> On Fri, Jan 21, 2011 at 3:58 PM, Martin Makundi < > >> >>>> [email protected]> wrote: > >> >>>> > >> >>>>> Hi! > >> >>>>> > >> >>>>> EnclosureResolver....container.autoAdd is invoked at render phase? > >> >>>>> Would it be necessary to hook into a different event (instead of > >> >>>>> onBeforeRender) or could it be pre-sniffed at onBeforeRender? > >> >>>>> > >> >>>>> ** > >> >>>>> Martin > >> >>>>> > >> >>>>> 2011/1/21 Joo Hama <[email protected]>: > >> >>>>> > I tested this idea by adding the code below to my web > application > >> >>>>> > object. Problem was though, > >> >>>>> > that when viewing the variables in debugger, the enclosure > object > >> didn't > >> >>>>> > seem to be included in > >> >>>>> > the tree of the parent objects. It was as if the enclosure was > >> invisible > >> >>>>> to > >> >>>>> > them. Perhaps because > >> >>>>> > Enclosure is a transparent resolver? > >> >>>>> > > >> >>>>> > object tree in my application: > >> >>>>> > > >> >>>>> > - page > >> >>>>> > - enclosure > >> >>>>> > - div (the parent of this was shown to be page, not the > >> enclosure) > >> >>>>> > > >> >>>>> > > >> >>>>> > Code: > >> >>>>> > //----------------------------- > >> >>>>> > @Override public AjaxRequestTarget newAjaxRequestTarget(Page > page) > >> { > >> >>>>> > AjaxRequestTarget target = super.newAjaxRequestTarget(page); > >> >>>>> > target.addListener(new AjaxRequestTarget.IListener() { public > void > >> >>>>> > onBeforeRespond(Map<String, Component> map, AjaxRequestTarget > >> target) { > >> >>>>> > for(Component component : map.values()) { Enclosure > parentEnclosure > >> = > >> >>>>> > component.findParent(Enclosure.class); if (parentEnclosure != > null) > >> { > >> >>>>> > Enclosure topParent = new Enclosure("DUMMY", "DUMMY"); while > >> (topParent > >> >>>>> != > >> >>>>> > null) { topParent = parentEnclosure.findParent(Enclosure.class); > if > >> >>>>> > (topParent != null) { parentEnclosure = topParent; } } > >> >>>>> > target.addComponent(parentEnclosure); } } } public void > >> >>>>> > onAfterRespond(Map<String, Component> map, IJavascriptResponse > >> response) > >> >>>>> {} > >> >>>>> > }); return target; }; > >> >>>>> > //----------------------- > >> >>>>> > > >> >>>>> > > >> >>>>> > On Fri, Jan 21, 2011 at 3:07 AM, Jeremy Thomerson < > >> >>>>> [email protected] > >> >>>>> >> wrote: > >> >>>>> > > >> >>>>> >> On Thu, Jan 20, 2011 at 1:51 PM, Igor Vaynberg < > >> >>>>> [email protected] > >> >>>>> >> >wrote: > >> >>>>> >> > >> >>>>> >> > interesting idea. i think this would require a bit of a > trick. > >> >>>>> >> > > >> >>>>> >> > a) modify enclosure tag handler to accept an attribute > instead > >> of a > >> >>>>> tag > >> >>>>> >> > b) modify enclosure tag handler to add a bit of metadata to > the > >> >>>>> >> > component marking that it belongs to an enclosure > >> >>>>> >> > c) add a ajaxrequesttarget.listener to the request target > that > >> checks > >> >>>>> >> > for this bit of metadata and adds the enclosure container to > the > >> ajax > >> >>>>> >> > request target. > >> >>>>> >> > > >> >>>>> >> > not sure how feasible this all is because the enclosure > >> container is > >> >>>>> >> > an auto component. but, you are welcome to tinker around. > >> >>>>> >> > > >> >>>>> >> > -igor > >> >>>>> >> > >> >>>>> >> > >> >>>>> >> I, too, like the idea. Couldn't it be simpler? Couldn't he: > >> >>>>> >> > >> >>>>> >> 1: Override newAjaxRequestTarget in WebApplication > >> >>>>> >> 2: When he creates an ART, add a listener to it. > >> >>>>> >> 3: In the listener, in onBeforeRespond, do this: > >> >>>>> >> > >> >>>>> >> for(Component component : map.values()) { > >> >>>>> >> Enclosure parentEnclosure = > >> component.findParent(Enclosure.class); > >> >>>>> >> while (parentEnclosure != null) { > >> >>>>> >> Enclosure topParent = > >> >>>>> parentEnclosure.findParent(Enclosure.class); > >> >>>>> >> if (topParent != null) { > >> >>>>> >> parentEnclosure = topParent; > >> >>>>> >> } > >> >>>>> >> } > >> >>>>> >> if (parentEnclosure != null) { > >> >>>>> >> addComponent(parentEnclosure); > >> >>>>> >> } > >> >>>>> >> } > >> >>>>> >> > >> >>>>> >> > >> >>>>> >> > >> >>>>> >> -- > >> >>>>> >> Jeremy Thomerson > >> >>>>> >> http://wickettraining.com > >> >>>>> >> *Need a CMS for Wicket? Use Brix! http://brixcms.org* > >> >>>>> >> > >> >>>>> > > >> >>>>> > >> >>>> > >> >>>> > >> >>> > >> >> > >> > > >> > > >
