When we are nesting (the same with IncludeTag and InsertPage plugin),
the context of the rendering needs to be changed, since all subtags
assume that wikiContext.getPage() gives you the right context.
The way we do this is simply by replacing the current context with a
fabricated one. This is a fairly normal idiom in recursion, though
the manipulation should probably be done a bit more cleanly than it's
done now.
I would be a bit hesitant to turn this into a Stack, since it's really
recursion. A single tag never manipulates the Stack as such, it just
takes whatever is offered. I don't really see if it clarified
anything - it might even confuse things since then we would have to
run through some hoops to make sure that the Stack is not visible to
any internal tags. I like the way we do it currently, which is fairly
clean.
The reason why WikiContext is protected instead of private is simply
because it's a fairly obvious thing to do - after all, it would
normally be a field available for the tag class; in this case it's
just put into a superclass. I'm not sure what exactly we would gain
by encapsulating it behind a method. Sometimes inheritance is great
for fields as well.
My guess is that some of your other modifications are causing problems
with the page names - on 2.8.x, the AttachmentsIteratorTag seems to be
working perfectly. Or maybe I misunderstood your problem?
/Janne
On 21 May 2009, at 03:11, Andrew Jaquith wrote:
This is mostly for Janne, but I'd appreciate insights from anybody
else who has an opinion on this...
I've been digging back into the JSP-tier stuff, and found something
that's a little odd. It's a bug, but like most bugs, it's also an
opportunity. :)
Here's the situation. The re-jiggered AttachmentsTab.jsp that I'm
working on, in my local branch, is injected into the normal view
template. The view template which has a wiki:PageName tag at the top,
which should render the name of the page. What is rendered depends on
the current WikiContext, i.e., the contents of the inherited field
m_wikiContext. This is all normal stuff, and is how we've always done
it.
Now, here's the issue. The AttachmentsIterator tag seems to do some
mucking around with the current wiki context, and appears to
re-assigns the value of it as it loops. In theory this "should" be ok,
because it (or its parent IteratorTag?) is supposed to reset it to the
original context.
I don't completely understand how this works. There seems to be two
ways that it's done: via direct manipulation of field m_wikiContext,
and possibly also via manipulation of a request-scoped attribute that
stores the WikiContext also (ATTR_CONTEXT).
What I do know is that when the AttachmentsIterator tag is present,
the wiki:PageName at the top of the page is rendering an attachment
name, instead of the page name. If I remove the AttachmentsIterator
element and its children, the page name is rendered correctly.
A couple of observations:
- It's quite hard to understand how and when the various WikiTagBase
subclasses change the value of field m_wikiContext. Would it be better
to hide this field (make it private), and provided accessor/mutator
methods to subclasses? This would make state manipulation more
transparent. It would also break non-JSPWiki WikiTagBase subclasses
(although to be fair I don't think there are likely to be that many).
- Same observation about ATTR_CONTEXT. It's very hard to figure out
how/why this request attribute is manipulated.
- For manipulating the current wikiContext, would a push/pop set of
methods be better? For example, when entering an iterator tag, push
the context onto a stack. When done, pop to restore the original
state. Among other things, this would make problems like this much
easier to debug.
Thoughts? I'd rather talk this through before writing (and
committing) code.
Andrew
