Wednesday, October 28, 2015, 3:52:35 AM, Pradeep Murugesan wrote: >> By that do you mean that you intend to continue it later so that it >> will only skip whitespace, etc., or you think this approach is more >> practical? (If it's the later, why?) > > ---- So by @@previous the user expects the previous node. But > currently it returns the \n , spaces, as you mentioned CDATA etc. > To skip these we need to maintain a list of blacklisted candidates > to skip. Today we have 3 candidates (let's assume) later we may get > lot to skip which we should be adding as blacklisted. > I went for this approach assuming there won't be any scenario > where we skip any nodes of type ELEMENT_NODE to fetch the > previousSibling node. If we will skip ELEMENT_NODE as well then no > other go we need to maintain a list of candidates to skip.
I'm not sure what you mean be "maintaining". We just check the node on the fly, and decide if we proceed with its sibling or return it. What we want to skip certainly won't change over time, as the information model of XML won't change any time soon, if ever. It's WS-only text (it doesn't mater if it's plain text or a CDATA section), comments and PI-s. (We never skip elements.) > Kindly let me know if I am wrong. > > Regarding the nullPointer exception I have handled it. But Didn't > commit. Its like parent directive right we will return null if its > the root node, similarly we can return null if its first and last > accessing previous and next respectively. > > Pradeep. > ________________________________________ > From: Daniel Dekany <[email protected]> > Sent: Wednesday, October 28, 2015 2:45 AM > To: Pradeep Murugesan > Subject: Re: Adding a new BuiltIn - previousSibling > > Tuesday, October 27, 2015, 6:04:19 PM, Pradeep Murugesan wrote: > >> Hi Daniel, >> >> Have fixed the code review comments here. >> https://github.com/pradeepmurugesan/incubator-freemarker/commit/2e1b0d834e941eaf4ea8aafad720333c7ec1040c > > It's minor issue, but BuiltInsExtForNode and BuiltInExtForNod still > don't follow the same convention as the others. The ...BI classes > should just be inside BuiltInsForNodes (no need for > BuiltInsExtForNode), and BuiltInExtForNode should be called > BuiltInForNodeEx. > >> Regarding the @@previous and @@next we decided to skip the >> whitespaces and other character data. Instead I tried to find first >> occurrence of the node which is of type Node.ELEMENT_NODE > > By that do you mean that you intend to continue it later so that it > will only skip whitespace, etc., or you think this approach is more > practical? (If it's the later, why?) > > Also, I believe that the current implementation will throw > NullPointerException after you have reached the first or the last > node. > >> https://github.com/pradeepmurugesan/incubator-freemarker/commit/2e1b0d834e941eaf4ea8aafad720333c7ec1040c#diff-a029bb56a7cedf8c6272a6d8b566f446 >> >> I tried few cases and things worked fine there. Kindly let me know your >> thoughts. >> >> P.S : I am working on the Junit test cases. >> >> Pradeep. >> >> ________________________________________ >> From: Daniel Dekany <[email protected]> >> Sent: Tuesday, October 27, 2015 3:36 AM >> To: Pradeep Murugesan >> Subject: Re: Adding a new BuiltIn - previousSibling >> >> OK, let's see. I have ran through the diff and have spotted these >> (just in the order as I find then): >> >> putBI("previousSibling", new previousSiblingBI()), etc. should be >> putBI("previous_sibling", "previousSibling", new previousSiblingBI()). >> >> BuiltInExtForNode: Doesn't follow the naming pattern of the other >> BuiltIns... classes. >> >> TemplateNodeModelExt: Should be TemplateNodeModelEx (as we already >> have other Ex models, we are stuck with that convention...) >> >> BuiltinVariable: You have registered two new names there, but these >> aren't built-in variables. >> >> In ElementModel: @@previous and @@next doesn't yet implement what we >> were talking about. I mean, it doesn't just skip white-space and >> comments and PI-s, but any text nodes. (Also an XPath-based >> implementation won't be very fast. org.w3c.dom.Node-s has >> getPreviousSibling()/getNextSibling() methods. Also, if you will skip >> WS text only, you won't be able to do that with XPath anyway.) >> >> (As a policy, there should not be author comments ("created by") in >> the source code.) >> >> -- >> Thanks, >> Daniel Dekany >> >> >> Friday, October 23, 2015, 9:09:56 PM, Pradeep Murugesan wrote: >> >>> Hi Daniel, >>> >>> https://github.com/pradeepmurugesan/incubator-freemarker/commit/465ed1bd768e8a5bee91bea7d3b291a5872efae5 >>> I have added the builtIns which will return blindly the previous >>> and next sibling and also the special variables @@previous and >>> @@next which will return the valid node. In the special variable >>> case I have used the xpath to get the required nodes. >>> Kindly review and let me know your thoughts. >>> Pradeep. >>>> Date: Sun, 18 Oct 2015 11:42:04 +0200 >>>> From: [email protected] >>>> To: [email protected] >>>> Subject: Re: Adding a new BuiltIn - previousSibling >>>> >>>> Returning the sibling node without skipping stuff is not XML-specific, >>>> so certainly that should be ?previous (and a new method in the new >>>> TemplateNodeModelEx interface), not a hash key that starts with "@". >>>> >>>> BTW, of course all of these has an opposite direction variant, like >>>> "@next". And "@prev" may should be "@previous". >>>> >>>> -- >>>> Thanks, >>>> Daniel Dekany >>>> >>>> >>>> Sunday, October 18, 2015, 5:31:50 AM, Pradeep Murugesan wrote: >>>> >>>> > yeah makes sense.. >>>> > so we need to return a valid element node he is looking for >>>> > skipping all the whitespace, CDATA etc... >>>> > I am wondering if the user will have any reason to look for a CDATA >>>> > sibling or any non element sibling which we will skip. >>>> > In that case can we have 2 special cases. >>>> > 1. @prev which will return the immediate sibling2. @prevNode or >>>> > something intutive which will return a valid element skipping few . >>>> > Pradeep. >>>> >> Date: Sat, 17 Oct 2015 20:15:57 +0200 >>>> >> From: [email protected] >>>> >> To: [email protected] >>>> >> Subject: Re: Adding a new BuiltIn - previousSibling >>>> >> >>>> >> Saturday, October 17, 2015, 7:09:49 PM, Pradeep Murugesan wrote: >>>> >> >>>> >> > hmm then I think @@prev should return the immediate sibling with the >>>> >> > following issues/advantages. >>>> >> > 1. In xml doc its a overhead for user to call it twice to get the >>>> >> > previous element node2. >>>> >> >>>> >> It much worse than that, if it just returns the previous sibling on >>>> >> the DOM, as you can't know if you have to call it once, twice, 3 >>>> >> times, etc. >>>> >> >>>> >> > For less document centric it is not a problem. >>>> >> >>>> >> For non-document XML it's similarly desirable. I meant JSON and such, >>>> >> where @@prev doesn't exist anyway... >>>> >> >>>> >> > 3. for Non-normalized dom we wont do anything before they normalize >>>> >> > it . >>>> >> >>>> >> Actually, we can do a little effort... skipping *all* the >>>> >> white-space-only character date nodes and comments and PI-s. But >>>> >> that's all. >>>> >> >>>> >> > Let me know If I got that correctly. >>>> >> > If so I will add @@prev as a special case and use >>>> >> > .node.@@prev.@@prev to get to theprevious sibling node. >>>> >> >>>> >> You mean, you will use: .node.@@prev >>>> >> >>>> >> > Pradeep. >>>> >> > >>>> >> >> Date: Fri, 16 Oct 2015 01:09:36 +0200 >>>> >> >> From: [email protected] >>>> >> >> To: [email protected] >>>> >> >> Subject: Re: Adding a new BuiltIn - previousSibling >>>> >> >> >>>> >> >> Thursday, October 15, 2015, 10:44:10 PM, Pradeep Murugesan wrote: >>>> >> >> >>>> >> >> > Hi Daniel, >>>> >> >> > So you are saying we need to have it that way and leave the >>>> >> >> > responsibility to the caller. Lets say in case of us to get to >>>> >> >> > check >>>> >> >> > if template is preceded by formDataModel we will do the follwing ? >>>> >> >> > <#local siblingElement = .node.@@prev.@@prev> >>>> >> >> > then check the role attribute of siblingElement ? >>>> >> >> > I assume the semantic for @@prev should return the immediate >>>> >> >> > sibling being it a whitespace , CDATA or \n as in our case. >>>> >> >> > Let me know your thoughts. >>>> >> >> >>>> >> >> I think that in almost all cases the user means the previous DOM node >>>> >> >> ignoring white-space nodes and comments, and certainly PI-s too. >>>> >> >> (That's also why ?previous or such wouldn't solve the problem you ran >>>> >> >> into, while it can be still very useful in some other applications, >>>> >> >> like where the tree is not from XML but something less >>>> >> >> document-centric.) (Non-normalized DOM-s, like one with sibling cdata >>>> >> >> nodes, could also complicate what we need, but I belive that such >>>> >> >> cases can only be addressed reasonably be ensuring that the whole DOM >>>> >> >> is normalized before we do anything with it... so it doesn't mater >>>> >> >> now.) >>>> >> >> >>>> >> >> > Pradeep. >>>> >> >> >> Date: Thu, 15 Oct 2015 20:32:33 +0200 >>>> >> >> >> From: [email protected] >>>> >> >> >> To: [email protected] >>>> >> >> >> Subject: Re: Adding a new BuiltIn - previousSibling >>>> >> >> >> >>>> >> >> >> Thursday, October 15, 2015, 4:13:18 PM, Pradeep Murugesan wrote: >>>> >> >> >> >>>> >> >> >> > HI Daniel, >>>> >> >> >> > Its not preceeded by white spaces but "\n" is taken as sibling. >>>> >> >> >> >>>> >> >> >> \n is whitespace, and it's a sibling in XML. >>>> >> >> >> >>>> >> >> >> > In book.xml <programlisting role="formDataModel">dsadsd >>>> >> >> >> > fdfsdfdsf dfds</programlisting> >>>> >> >> >> > <programlisting role="template"><#if cargo.weight < >>>> >> >> >> > <emphasis>100</emphasis>>Light >>>> >> >> >> > cargo</#if></programlisting> >>>> >> >> >> > I am trying to get the programlisting with role formDataModel as >>>> >> >> >> > previousSibling. But the "\n" is returned as the sibling. To >>>> >> >> >> > confirm >>>> >> >> >> > the same I just checked it with >>>> >> >> >> > node.previousSibling().previousSibling() and I am able to get >>>> >> >> >> > to formDataModel. >>>> >> >> >> > What should we need to do for this here ? >>>> >> >> >> >>>> >> >> >> Nothing... it's correct that way. it's that you want the sibling >>>> >> >> >> *element*, as I said. Actually, it's a bit trickier than that. You >>>> >> >> >> want to get the sibling element, unless the interfering character >>>> >> >> >> data >>>> >> >> >> is non-whitespace. Because, if you have <a/>cdata<b/>, then >>>> >> >> >> surely you >>>> >> >> >> don't want to say that <b/> is preceded bu <a/>, but "cdata". >>>> >> >> >> >>>> >> >> >> > I have also added a key with @@prev in ElementModel and that >>>> >> >> >> > works fine. >>>> >> >> >> >>>> >> >> >> So what exactly is the semantic of @@prev? >>>> >> >> >> >>>> >> >> >> > Pradeep. >>>> >> >> >> >> Date: Wed, 14 Oct 2015 22:32:40 +0200 >>>> >> >> >> >> From: [email protected] >>>> >> >> >> >> To: [email protected] >>>> >> >> >> >> Subject: Re: Adding a new BuiltIn - previousSibling >>>> >> >> >> >> >>>> >> >> >> >> I'm not sure what's improper in the result (I don't know what >>>> >> >> >> >> was >>>> >> >> >> >> expected). Isn't that node preceded by white space? That would >>>> >> >> >> >> explain >>>> >> >> >> >> it. You might rather want the previous *element*. But that >>>> >> >> >> >> will be >>>> >> >> >> >> difficult to express on the TemplateNodeModel level, which is >>>> >> >> >> >> not >>>> >> >> >> >> bound to XML. >>>> >> >> >> >> >>>> >> >> >> >> One important point is that you can't add new methods to >>>> >> >> >> >> TemplateNodeModel, as that breaks backward compatibility. It >>>> >> >> >> >> can only >>>> >> >> >> >> be added to a new sub-interface, like TemplateNodeModelEx. But >>>> >> >> >> >> even >>>> >> >> >> >> that won't solve getting the sibling element node. >>>> >> >> >> >> >>>> >> >> >> >> So another approach is instead of adding a built-in, adding a >>>> >> >> >> >> new >>>> >> >> >> >> special key that's specific to freemarker.ext.dom models, like >>>> >> >> >> >> "@@prev" and "@@next". >>>> >> >> >> >> >>>> >> >> >> >> -- >>>> >> >> >> >> Thanks, >>>> >> >> >> >> Daniel Dekany >>>> >> >> >> >> >>>> >> >> >> >> >>>> >> >> >> >> Wednesday, October 14, 2015, 9:10:25 PM, Pradeep Murugesan >>>> >> >> >> >> wrote: >>>> >> >> >> >> >>>> >> >> >> >> > Hi Daniel, >>>> >> >> >> >> > I tried to add a new built in & of course it DIDN'T work �. >>>> >> >> >> >> > I did the following. >>>> >> >> >> >> > 1. added putBI("previousSibling", new previousSiblingBI()); >>>> >> >> >> >> > in >>>> >> >> >> >> > BuiltIn.java2. added a static class in BuiltInForNodes.java >>>> >> >> >> >> > static >>>> >> >> >> >> > class previousSiblingBI extends BuiltInForNode { >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > @Override >>>> >> >> >> >> > TemplateModel calculateResult(TemplateNodeModel >>>> >> >> >> >> > nodeModel, >>>> >> >> >> >> > Environment env) throws TemplateModelException { >>>> >> >> >> >> > return nodeModel.getPreviousSibling(); >>>> >> >> >> >> > } >>>> >> >> >> >> > } >>>> >> >> >> >> > 3. added a method in Interface TemplateNodeModel.java >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > TemplateNodeModel getPreviousSibling() throws >>>> >> >> >> >> > TemplateModelException; >>>> >> >> >> >> > 4. In package freemarker.ext.dom's NodeModel added the >>>> >> >> >> >> > following method >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > public TemplateNodeModel getPreviousSibling() { Node >>>> >> >> >> >> > previousSibling = node.getPreviousSibling(); >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > return wrap(previousSibling);} >>>> >> >> >> >> > Once this is done I tried to access it as >>>> >> >> >> >> > .node?previousSibling >>>> >> >> >> >> > from template and it reached till the NodeModel class i >>>> >> >> >> >> > defined in >>>> >> >> >> >> > the 4th step. But the returned previousSibling is not >>>> >> >> >> >> > proper. It's >>>> >> >> >> >> > not returning the programListingNode with formDataModel >>>> >> >> >> >> > instead returns someother node. >>>> >> >> >> >> > I tried to log the node returned and I got the following o/p >>>> >> >> >> >> > [docgen:transform] [#text: >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > [docgen:transform] ] >>>> >> >> >> >> > I clearly understand the implementation of >>>> >> >> >> >> > getPreviousSibling is >>>> >> >> >> >> > not proper, but I couldn't figure out where we have >>>> >> >> >> >> > implemented the same. >>>> >> >> >> >> > Please advise. >>>> >> >> >> >> > Pradeep. >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> > >>>> >> >> >> >> >>>> >> >> >> > >>>> >> >> >> >>>> >> >> >> -- >>>> >> >> >> Thanks, >>>> >> >> >> Daniel Dekany >>>> >> >> >> >>>> >> >> > >>>> >> >> >>>> >> >> -- >>>> >> >> Thanks, >>>> >> >> Daniel Dekany >>>> >> >> >>>> >> > >>>> >> >>>> >> -- >>>> >> Thanks, >>>> >> Daniel Dekany >>>> >> >>>> > >>>> >>> >>> > > -- > Thanks, > Daniel Dekany -- Thanks, Daniel Dekany
