Hi Daniel, Have fixed the code review comments here. https://github.com/pradeepmurugesan/incubator-freemarker/commit/2e1b0d834e941eaf4ea8aafad720333c7ec1040c
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 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 >> >> >> > >> > >
