Hi Daniel, Got some time today to work on this. Will continue in the same fashion. Whenever you get time , kindly review and let me know.
Pradeep. ________________________________________ From: Pradeep Murugesan <[email protected]> Sent: Tuesday, November 3, 2015 11:49 AM To: [email protected] Subject: Re: Adding a new BuiltIn - previousSibling Hi Daniel, I have made the changes you have said and writing unit tests. I have written an unit test and need to check whether can I proceed in the same fashion. One important question I have is accessing the (XML) datamodel required for the testcase. Now I am overriding the function getDataModel() and read the xml from a file. Kindly let me know if that is acceptable. https://github.com/pradeepmurugesan/incubator-freemarker/commit/42132df19b6f8e53f66ff3f6cbbce459376c65a6 P.S : I have removed the author name in next commit. Intellij adds it and I am missing it everytime. Sorry!!. Pradeep. ________________________________________ From: Pradeep Murugesan <[email protected]> Sent: Thursday, October 29, 2015 7:46 AM To: [email protected] Subject: Re: Adding a new BuiltIn - previousSibling oh now I got it. So we can also expect something like <a/> there is some text here <b/> Now when the user do a @@previous on node 'b' he will get node 'a' but he might expect "there is some text here" which is still a valid text node. I thought there can be no such scenario so kept hanging on with blindly skipping all till we get a node. So I will do the following . 1. rename to @@previous_significant 2. skip the siblings when its in any of the blacklisted candidates. ( whitespaces, CDATA, \n(ofcourse)) Pradeep. ________________________________________ From: Daniel Dekany <[email protected]> Sent: Thursday, October 29, 2015 4:12 AM To: Pradeep Murugesan Subject: Re: Adding a new BuiltIn - previousSibling Wednesday, October 28, 2015, 6:21:19 PM, Pradeep Murugesan wrote: > Hi Daniel, > > I agree with that but I have a question kindly don't take it as an argument. > Just curious to know > > 1. <a/>cdata<b/> > 2. <a/> \n<b/> > 3. <a/>comments<b/> > 4. <a/>some PI's<b/> > > In all the above 4 scenarios when we do a @@previous on node 'b' we expect > node 'a'. With what you have implemented so far, that is. > I am suggesting we will keep iterating until we find a node type > ELEMENT_NODE and return it. > you are suggesting to keep iterating until we find a node that is not in \n, > CDATA, PIs etc. > > I think both will work. Do you think any of it which should be > skipped will also have node type ELEMENT_NODE. Nope. > I am not sure about what is a better logic though. Kindly let me > know if I am not getting something which you are telling. Silently skipping non-whitespace text is dangerous. But if you call this @@previous_element, then the user will expect it to happen, so then what you have implemented can be OK. As of my @@previous definition, the name is problematic even there, as it doesn't just return the previous sibling (?previousSibling does that). It does some magic, by skipping whitespace and such. So certainly it should be called @@prevous_significant or @@previous_non_ws, so that it's clear that some trickery is involved. As of the semantic, the motivation is simply to return what many naturally expect to be the previous node. Like remember your case; getting some text instead of the preceding programlisting element was unexpected at first, I assume. Yes, your definition of @@previous fixes that too. But if you had some non-whitespace text between those two programlisting elements, certainly you expect to get that text, not the element before it. People don't see non-whitespace text as ignorable, because in fact it hardly ever is. So after renaming both operations are OK, but I think @@previous_significant is a safer operation than @@previous_element, because you won't unintentionally skip non-whitespace text with it. Surely @@previous_element is quite clear about what it does (that it will skip text), but then, what can the users do about it? They will have to hope that there won't be any non-whitespace text before the target element, ever. Because when there is, they won't know about it, they can't give an error or something. With @@prevous_significant, when that assumption fails, they will get the text node and the template that expects an element can fail or take some special action, so there's no silent information loss. -- Thanks, Daniel Dekany > Pradeep. > ________________________________________ > From: Daniel Dekany <[email protected]> > Sent: Wednesday, October 28, 2015 1:33 PM > To: Pradeep Murugesan > Subject: Re: Adding a new BuiltIn - previousSibling > > 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 >
