It looks about right. We shouldn't require data-model for all examples, as in many cases it would be empty anyway.
-- Thanks, Daniel Dekany Thursday, December 17, 2015, 2:52:45 PM, Pradeep Murugesan wrote: > Hi Daniel, > > I have made the changes in doc-gen as per the core changes. > > https://github.com/pradeepmurugesan/incubator-freemarker-docgen/commit/46d77c6b5a3cbe01a50c7756e1efb630ca00e18a > > I have added a formDataModel in /manual/dgui_quickstart_template.html > > Kindly check and let me know if its fine. > > Also should we need to add a datamodel section for all the template section > in the manual ? > > Pradeep. > > > > > > > > > ________________________________________ > From: Daniel Dekany <[email protected]> > Sent: Monday, December 14, 2015 2:39 AM > To: Pradeep Murugesan > Cc: [email protected] > Subject: Re: Adding a new BuiltIn - previousSibling > > You can build on these new "@@" keys in Docgen of course. As of when > it will be merged into a stable release, I don't know yet, maybe > 2.3.24, maybe 2.3.25. In any case, Docgen, as an internal project, can > use nightly versions, so it doesn't have to wait for stable releases. > > For efficiency, I usually try to review contributions in one go, when > the pull request is merged. But I took a quick glance at the commits, > and hasn't spotted any problems. > > -- > Thanks, > Daniel Dekany > > > Sunday, December 13, 2015, 9:48:17 AM, Pradeep Murugesan wrote: > >> Hi Daniel, >> >> I have added those cases for CDATA as well. >> https://github.com/pradeepmurugesan/incubator-freemarker/commit/620d8a35e689bd6e94fb77ceb844105d66b90ca9 >> >> Renamed @@previous and @@next to @@previous_significant and >> @@next_significant >> https://github.com/pradeepmurugesan/incubator-freemarker/commit/cbe7025bfe8fe713b74d1b5499d14fd7cd35c4f8 >> >> Kindly review the same and let me know if we are good to integrate with >> docgen. >> >> Pradeep. >> >> >> ________________________________________ >> From: Daniel Dekany <[email protected]> >> Sent: Sunday, December 13, 2015 12:07 AM >> To: Pradeep Murugesan >> Cc: [email protected] >> Subject: Re: Adding a new BuiltIn - previousSibling >> >> I guess you get it right. We have to ignore text that's white-space >> only, and wether it's CDATA or not we will do the same. >> >> >> Saturday, December 12, 2015, 7:45:17 AM, Pradeep Murugesan wrote: >> >>> Hi Daniel, >>> >>> So we can ignore a CDATA text of that is mere formatting right ? >>> >>> Now it burns down to identify whether the text inside CDATA is a formatted >>> one or not. >>> >>> Am I right or did you mean the other way ? >>> >>> Pradeep. >>> >>> ________________________________________ >>> From: Daniel Dekany <[email protected]> >>> Sent: Friday, December 11, 2015 12:44 AM >>> To: Pradeep Murugesan >>> Subject: Re: Adding a new BuiltIn - previousSibling >>> >>> Thursday, December 10, 2015, 9:28:31 AM, Pradeep Murugesan wrote: >>> >>>> Hi Daniel, >>>> >>>> Done the changes >>>> https://github.com/pradeepmurugesan/incubator-freemarker/commit/296a7a85a1f1683a3d20be0220881333cbdc4216 >>>> (ignore build.xml , have reverted it) >>>> >>>> So previously we discussed to skip the following >>>> >>>> 1. Empty spaces >>>> 2. Comments >>>> 3. PIs >>>> 4. CDATA >>> >>> There's some kind of misunderstand here as CDATA shouldn't be there >>> like that. But see later. >>> >>>> Now 2 & 3s are not included in dom at all. >>> >>> Depends on how the TemplateModel was created. There are some >>> convenience methods included that remove commends and PI-s from the >>> DOM itself before wrapping, but some applications will just give a DOM >>> to wrap. >>> >>>> even the ?previousSibling skips those elements. >>>> >>>> Now the challenge comes in skipping the CDATA. I tried to use the >>>> nodeType check i.e ( if node.getNodeType == Node.CDATA_SECTION_NODE) >>>> but this is not working since CDATA node is considered as text node >>>> and the node type is returned as TEXT. Also the getNodeTextContent >>>> is returning the text inside the CDATA tag (Not the string CDATA >>>> itself) so I am not sure how we will be picking which is a >>>> characterData and which is a non empty text. >>> >>> CDATA is nothing just syntax for avoiding escaping special characters. >>> So of course we don't want to ignore them in general. Just think about >>> it, in <a/><![CDATA[foo bar]]><b/>, it's not like "foo bar" there can >>> be ignored without losing useful information (as opposed to losing >>> text that's just formatting). In fact, something being inside CDATA is >>> a proof that it's not just formatting, even if it's white-space. But >>> as we can't (reliably) tell if a piece of text is coming from CDATA or >>> not, we should ignore that difference even where you can tell it. >>> >>> -- >>> Thanks, >>> Daniel Dekany >>> >>>> Eg: >>>> <person> >>>> <profession>Software Engineer</profession> >>>> <![CDATA[ <a>test<a> ]]> >>>> <phone>12345678</phone> >>>> </person> >>>> >>>> doing a doc.person.phone.@@previous returns the node type as text with >>>> value as <a>test<a>; >>>> >>>> I am not sure which is the criteria to check the CDATA node. Am i missing >>>> something here ? >>>> >>>> Pradeep. >>>> >>>> >>>> >>>> ________________________________________ >>>> From: Daniel Dekany <[email protected]> >>>> Sent: Thursday, December 10, 2015 5:07 AM >>>> To: Pradeep Murugesan >>>> Cc: [email protected] >>>> Subject: Re: Adding a new BuiltIn - previousSibling >>>> >>>> Wednesday, December 9, 2015, 10:11:04 AM, Pradeep Murugesan wrote: >>>> >>>>> Daniel, >>>>> >>>>> you got a chance to review this ? >>>>> >>>>> Pradeep. >>>>> >>>>> ________________________________________ >>>>> From: Pradeep Murugesan <[email protected]> >>>>> Sent: Monday, December 7, 2015 10:15 AM >>>>> To: Daniel Dekany >>>>> Cc: [email protected] >>>>> Subject: Re: Adding a new BuiltIn - previousSibling >>>>> >>>>> Hi daniel, >>>>> >>>>> I have a question on the @@previous and @@next being null. So we >>>>> will return the previous significant node if exists but will return >>>>> an empty set of nodes if its null. which means we will return a >>>>> NodeListModel with an empty ArrayList. >>>> >>>> Yes. >>>> >>>>> In that case shouldn't we be wrapping the non null node too in >>>>> NodeListModel instead of NodeModel ? >>>>> >>>>> Right now the code might look like this >>>>> >>>>> if(previousSibling == null) { >>>>> return new NodeListModel(EMPTY_ARRAYLIST, null); >>>>> } else { >>>>> return wrap(previousSibling); >>>>> } >>>> >>>> Looks OK to me. >>>> >>>>> we need to return one dataType right ? it should be like >>>>> >>>>> if(previousSibling == null) { >>>>> return new NodeListModel(EMPTY_ARRAYLIST, null); >>>>> } else { >>>>> return NodeListModel(previousSibling); >>>>> } >>>>> >>>>> Let me know your inputs. >>>> >>>> NodeModel-s (like ElementModel) implement TemplateSequenceModel, just >>>> like NodeListModel does, so as far as the template is concerned, they >>>> are both list-like. The main difference is that a NodeModel can only >>>> represent a node sequence of size 1, while NodeListModel can represent >>>> a node sequence of arbitrary size. When your node sequence happens to >>>> be of size 1, you should always use NodeModel instead of >>>> NodeListModel, because only NodeModel-s implement TemplateScalarModel >>>> and so can be treated as a single strings in the template. >>>> >>>> I have to add though that the DOM wrapper is a part of the code that >>>> I'm not familiar with, and that wasn't cleaned up by me either. So >>>> watch out. >>>> >>>>> Pradeep >>>>> >>>>> ________________________________________ >>>>> From: Daniel Dekany <[email protected]> >>>>> Sent: Monday, December 7, 2015 4:05 AM >>>>> To: Pradeep Murugesan >>>>> Cc: [email protected] >>>>> Subject: Re: Adding a new BuiltIn - previousSibling >>>>> >>>>> Sunday, December 6, 2015, 4:28:11 PM, Pradeep Murugesan wrote: >>>>> >>>>>> >>>>>> Hi Daniel, >>>>>> >>>>>> sorry for this huge gap.. Actually got caught up in Chennai >>>>>> floods ( Chennai, India). Just back to home town and safe. >>>>>> >>>>>> I have done the unit tests and the renaming of the files you >>>>>> suggested previously. Please review and let me know the changes. >>>>>> >>>>>> https://github.com/pradeepmurugesan/incubator-freemarker/commit/1db672a2ba3db1f08c594df663b4dd7e68d36d4a >>>>> >>>>> One random detail that I have spotted is: >>>>> node.getTextContent().trim().isEmpty(). It's not very efficient if you >>>>> think about it. Something like StringUtil.isTrimmableToEmpty would be >>>>> better, only with String argument of course. >>>>> >>>>>> I need to cover a case for which I need your inputs. >>>>>> >>>>>> Lets say we are in the last sibling and trying to access the next, >>>>>> same applies for previous as well what should we return ? null ? Kindly >>>>>> let me know your thoughts. >>>>> >>>>> "?previous" and "?next" should just return null. But "@@previous" and >>>>> "@@next" should behave like the other "@@" keys, that is, with XPath >>>>> logic, which says that the result is an empty set of nodes. Again >>>>> similarly to other "@@" keys and XPath expression, they should work >>>>> correctly on node sets that contains 0 or multiple nodes. >>>>> >>>>> -- >>>>> Thanks, >>>>> Daniel Dekany >>>>> >>>>>> Pradeep. >>>>>> ________________________________________ >>>>>> From: Daniel Dekany <[email protected]> >>>>>> Sent: Saturday, November 21, 2015 2:34 AM >>>>>> To: Pradeep Murugesan >>>>>> Cc: [email protected] >>>>>> Subject: Re: Adding a new BuiltIn - previousSibling >>>>>> >>>>>> Friday, November 20, 2015, 8:51:31 AM, Pradeep Murugesan wrote: >>>>>> >>>>>>> Hi Daniel, >>>>>>> >>>>>>> Took a long break due to some personal reasons. Sorry for the same. I >>>>>>> have a question in your email. >>>>>>> >>>>>>> What do you mean by >>>>>>> >>>>>>> "Also I guess inside the testPreviousSibling you don't really need >>>>>>> output capturing, nor ?trim. " >>>>>>> >>>>>>> I am not sure what you are coming to say there. We need to assert >>>>>>> somehow the expected o/p right ? >>>>>>> >>>>>>> so we can't assert against empty spaces since we don't know how >>>>>>> many spaces , So I thought of asserting the same after trimming the o/p. >>>>>> >>>>>> We don't need capturing for sure, I guess you see that now. As of >>>>>> trimming, that's a minor issue really, but in fact we know how many >>>>>> spaces are there, since we provide the XML. >>>>>> >>>>>>> Let me know if I am missing something. >>>>>>> >>>>>>> Pradeep. >>>>>>> ________________________________________ >>>>>>> From: Daniel Dekany <[email protected]> >>>>>>> Sent: Tuesday, November 10, 2015 2:44 AM >>>>>>> To: Pradeep Murugesan >>>>>>> Subject: Re: Adding a new BuiltIn - previousSibling >>>>>>> >>>>>>> Tuesday, November 3, 2015, 7:19:17 AM, Pradeep Murugesan wrote: >>>>>>> >>>>>>>> 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. >>>>>>> >>>>>>> You don't need to override getDateModel(). Just add "doc" to the data >>>>>>> model with the TemplateTest.addToDataModel. >>>>>>> >>>>>>> Loading the XML file via java.io.File API is not entirely correct, >>>>>>> especially not with that relative path ("build/test-classes/..."). You >>>>>>> don't know what the current directory will be on the CI server for >>>>>>> example. Also, though an extreme case, but it can also occur that a >>>>>>> test suite is ran from an unexploded jar (i.e., you don't even have >>>>>>> real files anywhere). Just like outside tests, the correct solution is >>>>>>> using Class.getResource or Class.getResourceAsStream to read >>>>>>> class-loader resources. >>>>>>> >>>>>>> Also I guess inside the testPreviousSibling you don't really need >>>>>>> output capturing, nor ?trim. >>>>>>> >>>>>>>> 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 >>>>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Thanks, >>>>>>> Daniel Dekany >>>>>> >>>>>> -- >>>>>> Thanks, >>>>>> Daniel Dekany >>>>>> >>>>> >>>>> >>>>> --- >>>>> This email has been checked for viruses by Avast antivirus software. >>>>> https://www.avast.com/antivirus >>>>> >>>> >>>> -- >>>> Thanks, >>>> Daniel Dekany >>>> >>>> >>>> --- >>>> This email has been checked for viruses by Avast antivirus software. >>>> https://www.avast.com/antivirus >>>> >>> >>> -- >>> Thanks, >>> Daniel Dekany >>> >>> >>> --- >>> This email has been checked for viruses by Avast antivirus software. >>> https://www.avast.com/antivirus >>> >>> >> >> -- >> Thanks, >> Daniel Dekany >> >> >> --- >> This email has been checked for viruses by Avast antivirus software. >> https://www.avast.com/antivirus >> >> > > > --- > This email has been checked for viruses by Avast antivirus software. > https://www.avast.com/antivirus --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
