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 <joonas.hamalai...@gmail.com> 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 <igor.vaynb...@gmail.com>wrote:
>
>> On Sat, Feb 19, 2011 at 2:20 PM, Juergen Donnerstag
>> <juergen.donners...@gmail.com> 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 <igor.vaynb...@gmail.com>
>> 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 <joonas.hamalai...@gmail.com>
>> 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 <joonas.hamalai...@gmail.com
>> >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 <
>> >>>> martin.maku...@koodaripalvelut.com> 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 <joonas.hamalai...@gmail.com>:
>> >>>>> > 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 <
>> >>>>> jer...@wickettraining.com
>> >>>>> >> wrote:
>> >>>>> >
>> >>>>> >> On Thu, Jan 20, 2011 at 1:51 PM, Igor Vaynberg <
>> >>>>> igor.vaynb...@gmail.com
>> >>>>> >> >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*
>> >>>>> >>
>> >>>>> >
>> >>>>>
>> >>>>
>> >>>>
>> >>>
>> >>
>> >
>>
>
