On 6/9/06, Johan Compagner <[EMAIL PROTECTED]> wrote:
> i still don't know exactly what you want to have as a mutable reference
> inside component...
> I don't want it to be the ComponentTag or a Part of the markup!
> That is much much to big.
>
that is true and I don't want that either.
But IMO a Component has associated Markup and the Markup has
associated the tag and that has the attributes.
First point is I don't want to clutter Component with getTag and
getMarkup and whatever. I'd rather want to make the API smaller and
more concise.
2nd point currently MarkupStream references the Markup. Its a small
class and its major purpose is to provide an iterator for Markup.
There is more to it, but that is it major purpose. I imagine to have
MarkupFragment which is similar to MarkupStream in that it references
the Markup (not the iterator thing) but allows to modify the lazy
created copy of the start tags attribute. I guess the whole point is
about structure. We can do what you want and put all functionality
into Component and focus on memory footprint but I'm more into
smaller, easily testable, separated etc modules may be at the cost of
a little bit more memory. I think it is worth it. Remember, it is only
the reference to Markup which I want to keep. I don't want to copy the
markup.
> So i think we talk about 2 different things here. I just want the markup
> attributes found in the constructor
> of the component for 2 things..
> 1> check directly if the component is in the right place at construct time.
> 2> get the mutable markup attributes of the start tag so that directly in
> the constructor you can do:
> getMarkupAttributes().put("style","mystyle: xxx");
>
I don't think we are talkng about different things except that I would
add (not replace or change) additional requirements. E.g. the Markup
is required for the render phase later on as well. If there were a
possibility to search (remember we'll need resolvers for different
user needs to find the markup fragment which we would need to call at
least twice than) the associated markup only once (at constructor
time) than that be better.
And it wouldn't cause any issues if the markup file is changed in
between the constructor and the render phase. You load it at
constructor time, you keep a reference, and as there will not be
anymore request for the markup, if won't be reloaded and not causing
any problem because they are out of sync.
> The changes you want lays in the way we ask for those 2 things. And that can
> be changed however we want.
> now we do this in the constructor:
>
>
> MarkupStream markupStream = MarkupFragmentFinder.find
> (this);
> ComponentTag tag = markupStream.getTag();
> if(tag.hasAttributes())
> {
> markupAttributes = new
> CopyOnWriteValueMap(tag.getAttributes());
> }
>
>
> that can be changed fine to:
>
>
> ComponentTag tag = getMarkup().getTag();
> if(tag.hasAttributes())
> {
> markupAttributes = new CopyOnWriteValueMap(
> tag.getAttributes());
> }
>
> and then in that getMarkup() call we do the magic you describe so that the
> component gets its immutable markup reference that is also used
> in the renderComponent call. (which then takes the current markupAttributes
> map instead of the one in the immutable markup)
This is true but it is not the point I'm trying to make. IMO keeping a
(lazy) copy of the tag attributes with the Component is not the best
solution. We should rather keep a MarkupFragement (which is not a copy
of the whole Markup as discribed).
Juergen
>
> johan
>
>
> On 6/9/06, Juergen Donnerstag <[EMAIL PROTECTED]> wrote:
> >
> On 6/9/06, Johan Compagner <[EMAIL PROTECTED]> wrote:
> > What you describe is exactly what i said
> > that MarkupFragment is the ComponentTag (with the XmlTag) inside it.
>
> not exactly. It is more like Markup.java which contains the start tag,
> for containers all MarkupElements of the body and the close tag. The
> reason why I prefer the Markup is because we'll need it for the render
> phase anyway. The open tag is always the first MarkupElement and
> easily be accessed through Markup. And IMO
> Component.getMarkup().getTag() wouldn't clutter the Component API as
> much. Just one single method which MarkupContainers already have.
>
> > But those are not mutable because then you would alter the markup file
> > itself which is bad.
>
> thats true. But I'd much have that code outside Component. Component
> already is a monster (not complicated but big). Ok, lets say we call
> it MarkupFragmentStream (similar to the naming we currently have) and
> the Stream is Component specific and refers the MarkupFragment. Than
> MFS could handle the case where we need a copy of the start tag and
> its parameters because any of the parameters must be modified. At
> least the code is out of Component than.
>
> >
> > And i don't want to store (transient is not possible if the
> > Tag/MarkupFragment where mutable!)
> > the Tag/MarkupFragment completely in every component. Because that would
> > mean that the 2 objects
> > (ComponentTag and its child XmlTag) where cloned and hold on to for every
> > component.
>
> see above.
>
> >
> > My current implementation is. If the xmltag had attributes then i make a
> > mutable copy of that
> > and only that map is stored in the component. And that map is used in the
> > render phase
>
> for the reasons mentioned in the original mail, I do not prefer that
> solution. IMO is half baked. We need the Markup / MarkupStream for
> render anyway.
>
> > So SimpleAttributeModifiers (just String-String things) can be removed
> now.
> > Because you can just do
> > getMarkupAttributes().put("attributename",
> > "attributevalue")
> >
> > We need that attributemap anyway in the constructor. Because in 2.0 we
> > should be able to get the markup
> > attributes (like markupid/cssid) inside the constructor of the component.
> > And we want to check as early as possible
> > if the component can be added to the parent (if it has a component tag on
> > that level in the markup)
> >
>
> that is not different in either approaches. As I mentioned in my first
> response that is what is needed. I just don't agree with the
> ComponentTag being added to the Component. I'd rather like to have the
> MarkupFragmentStream (or whatever you want to call it). As the tag and
> the markup are need heavily afterwards anyway.
>
> > I just go now through the MarkupFragmentFinder in the constructor to check
> > for the component tag
> > and to get the attributes if there are attributes:
> >
>
> In case someone implemented a resolver, he now has to make sure it is
> synchronized (reverse algorithm) with the one he'll for the the
> MarkupFinder. That is awkward and should be avoided.
>
> > This is what the Component base constructor looks like now:
> >
> >
> >
> > public Component(MarkupContainer<?> parent, final String id, final
> > IModel<T> model)
> > {
> > if (parent == null)
> > {
> > if (!(this instanceof Page))
> > {
> > throw new
> WicketRuntimeException("component
> > without a parent is not allowed.");
> > }
> > }
> > this.parent = parent;
> > setId(id);
> > this.model = model;
> >
> >
> >
> getApplication().notifyComponentInstantiationListeners(this);
> > if (id.startsWith(AUTO_COMPONENT_PREFIX))
> > {
> > parent.autoAdd(this);
> > }
> > else
> >
> > {
> > try
> > {
> > MarkupStream markupStream =
> MarkupFragmentFinder.find(this);
> > ComponentTag tag = markupStream.getTag();
> > if(tag.hasAttributes())
> > {
> > markupAttributes = new ValueMap(tag.getAttributes());
> >
> > }
> > }
> > catch(RuntimeException re)
> > {
> > throw new
> WicketRuntimeException("Couldn't
> > find the markup of the component " + id + " in parent " +
> > parent.getPageRelativePath());
> > }
> > parent.add(this);
> > }
> > }
> >
> > Can we optimize that find() a bit?
> > For example the markup just has a map as far as i know of componentpath ->
> > index of the tag? That looks fast to me..
> > What is slow there?
> >
>
> I think it is the wrong apporach. As mentioned above is not only
> finding it, it is a) about often do you need to call it to find the
> fragment during the whole cycle and not only in the constructor and
> 2nd it must be more flexible. The algorithm to find the associated
> fragment must be replaceble for users who are e.g. using flat
> structures compared to hierarchical ones
>
> Juergen
>
> > johan
> >
> >
> >
> > johan
> >
> >
> >
> > On 6/8/06, Juergen Donnerstag < [EMAIL PROTECTED] > wrote:
> > >
> > Hopefully the javadoc still says that getMarkupAttribute() is not very
> > efficient and that it should only be used in rare cases. The reason
> > why is because finding the tag in the markup is yet very inefficient
> > compared to using the tag parameter provided to many render XXX
> > functions. But I agree that in general the approach is what we want,
> > IMO just not the tag but the MarkupFragment (the piece of Markup that
> > is associated with the component). And because the Container (not the
> > Component though) does already have a MarkupStream, you wouldn't need
> > an additional variable. More importantly may be, MarkupFragment could
> > be transient as you only need to ask the markupcache to get a new
> > copy. MarkupFragment provides you access to the tag which than can be
> > modified. And at load time the MarkupFragment would be created and
> > kept in a meaningful structure (cache) to find it more efficiently.
> > The current implement is far from perfect. One more note: currently we
> > use resolvers which allow to augment or replace the strictly
> > hierarchical order of tags e.g. with a flat one (scan the mail archive
> > for people doing that). We'd need a similar flexibility to find the
> > tag in the markup. IMO that would be a more complete approach but it'd
> > be much more work.
> > As a side note: I'm not especially proud of that renderComponent code
> > everything that is related to it. It was more a quick hack to have a
> > fast result rather than an well thougt through approach. It was meant
> > to be replaced in 2.0 with what I described above. IMO we shouldn't
> > modify it a lot. It is like hacking a hack.
> >
> > Juergen
> >
> > On 6/8/06, Johan Compagner < [EMAIL PROTECTED]> wrote:
> > > Hi,
> > >
> > > i am looking at this code inside Component:
> > >
> > > public final ValueMap getMarkupAttributes()
> > > {
> > > MarkupStream markupStream = MarkupFragmentFinder.find(this);
> > > ValueMap attrs = new ValueMap(
> > > markupStream.getTag().getAttributes());
> > > attrs.makeImmutable ();
> > > return attrs;
> > > }
> > >
> > > and:
> > >
> > > public final void renderComponent(final MarkupStream markupStream)
> > > {
> > > this.markupIndex = markupStream.getCurrentIndex ();
> > >
> > > // Get mutable copy of next tag
> > > final ComponentTag openTag = markupStream.getTag();
> > > final ComponentTag tag = openTag.mutable();
> > >
> > > // Call any tag handler
> > > onComponentTag(tag);
> > >
> > >
> > > What i would like to do is change this behaviour and let the Component
> > > creates a copy of there own Tag inside itself (at construct time)
> > >
> > > so that getMarkupAttributes() is not immutable anymore.
> > > you can just do:
> > >
> > > mylabel.getMarkupAttributes().put("class", "mycssclass");
> > >
> > > instead of making a attribute modifer object with attribtue "class" and
> > > value "mycssclass" and then add that as a behaviour/attributemodifier
> on
> > the
> > > component.
> > >
> > > so i want something like this: (all on Component)
> > >
> > >
> > > public Component(MarkupContainer<?> parent, final String id, final
> > > IModel<T> model)
> > > {
> > > if (parent == null)
> > > {
> > > if (!(this instanceof Page))
> > > {
> > > throw new
> > WicketRuntimeException("component
> > > without a parent is not allowed.");
> > > }
> > > }
> > > this.parent = parent;
> > > setId(id);
> > >
> > > try
> > > {
> > > MarkupStream markupStream = MarkupFragmentFinder.find(this);
> > > tag = markupStream.getTag().mutable(); // GET the Tag out of
> > the
> > > markup and make a copy.
> > > }
> > > catch(RuntimeException re)
> > > {
> > > throw new WicketRuntimeException("Couldn't
> > find
> > > the markup of the component " + id + " in parent " +
> > > parent.getPageRelativePath ());
> > > }
> > > ........
> > >
> > >
> > > ComponentTag getTag() // Return the tag. Maybe not needed this
> > method..
> > > {
> > > return tag;
> > > }
> > >
> > > public final ValueMap getMarkupAttributes()
> > > {
> > > return tag.getAttributes(); // Directly return the mutable tags
> > > attributes
> > > }
> > >
> > > public final void renderComponent(final MarkupStream markupStream)
> > > {
> > > this.markupIndex = markupStream.getCurrentIndex ();
> > >
> > > final ComponentTag tag = getTag(); // Just use the mutable tag
> > > attributes.
> > >
> > > // Call any tag handler
> > > onComponentTag(tag);
> > > .................
> > >
> > >
> > > The only thing is that we then hold a reference to a Tag in every
> > > component..
> > > We don't need some AttributeModifiers then because you can do that
> > directly.
> > > And we don't generate constant copies..
> > >
> > > what we also could do is not get a copy of the tag but just the
> attributes
> > > map.
> > > Then we don't hold the complete xml tree as a copy in mem.
> > >
> > > Then we just need to set those attributes in the component tag at render
> > > time.
> > > i guess this would be even much better.
> > >
> > > any other ideas?
> > >
> > > johan
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > Wicket-develop mailing list
> > > Wicket-develop@lists.sourceforge.net
> > >
> >
> https://lists.sourceforge.net/lists/listinfo/wicket-develop
> > >
> > >
> > >
> >
> >
> > _______________________________________________
> >
> > Wicket-develop mailing list
> > Wicket-develop@lists.sourceforge.net
> >
> https://lists.sourceforge.net/lists/listinfo/wicket-develop
> >
> >
> >
> >
> > _______________________________________________
> > Wicket-develop mailing list
> > Wicket-develop@lists.sourceforge.net
> >
> https://lists.sourceforge.net/lists/listinfo/wicket-develop
> >
> >
> >
>
>
> _______________________________________________
>
> Wicket-develop mailing list
> Wicket-develop@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/wicket-develop
>
>
>
>
>
> _______________________________________________
> Wicket-develop mailing list
> Wicket-develop@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/wicket-develop
>
>
>
_______________________________________________
Wicket-develop mailing list
Wicket-develop@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/wicket-develop
