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
