tableOfContents questions.

Thu May 26 19:11:22 UTC 2011

Thanks for the in depth code review. I'll reply to both e-mails here. Sorry about the e-mail mix up, by the way.  I think that's a good reason why I shouldn't be sending out e-mails late at night :) Another reason for not sending e-mails late at night is that I forgot to cc the list, which I'm doing this time. 

Please see my replies inline below.

Also, I'm still having the problem with the skipped level being filled by the parent heading. See the bottom of the e-mail for an example.

I've pushed up lots of refactoring to my github branch, but didn't get to the unit tests yet. :( this is very sad. I'll try to get to them asap. Probably should have started with them.

 https://github.com/jobara/infusion/tree/FLUID-4209

Thanks
Justin

On 2011-05-26, at 1:44 AM, Antranig Basman wrote:

> Hi there o KINGGG... it was tolerable although these kinds of things wear me down a lot. I would prefer to be INNN with a CATTT.
> 
> Thanks for sending along your TOCCK for review, here are my comments:
> 
> Line 43: This should be a call to fluid.transform rather than fluid.each.

I should have spotted that one. This seems so obvious now that you've mentioned it. I suppose my reasoning would have been along the lines of the fact that it was also performing DOM manipulation to insert the anchor. 

> 
> Line 44: Factor this out as a "headingTextToAnchor" function. Should probably distinguish throughout that "anchors" is an array of anchor URLs rather than just the names of them. Perhaps better to operate on a list of {url: url, id: id} rather than just a plain list of strings even though it seems a bit petty to do this just for the sake of a present or missing "#" :P This at least would enable you get the "insertAnchor" operation out of the loop which at present is an undesirable side-effect.
> 
> The component entry "tocAnchors" doesn't seem to serve a clear function to component users as it stands... at the least, it might be better for it to take a form that might help users to find these controls in the DOM. By the sheerest accident, the selector which they would use is textually identical to the anchor URL, but it would be sleazy to exploit this explicitly :)

I've removed the potential sleaze by using the object structure you recommended above. I've also pulled out the code from within the loop into a function called "headingTextToAnchor" and move the sanitization code into an invoker. Also renamed tocAnchors to anchorInfo.

> 
> Line 60, etc. - there's no real benefit using the long form "{fluid.tableOfContents}" here since you are SURE that the short form also matches and there is nothing intervening.

I think I understand what you mean here.. basically I just converted anything that was {fluid.tableOfContents} to {tableOfContents}. I'm not sure i fully understand when you can and cannot use the short vs long form.

> 
> Line 91: It would be better if this could be expressed as a "pure function", that is, one that doesn't depend on the DOM existing, since it is so close to this condition already. If the user has "prepared" the headings argument by ensuring it is just a list of strings, this function could be more reusable (as well as on the server).
> 
> Further:
> 
> Line 91: Any particular reason for this function to consume its argument, "headings"? I think it is probably positively immoral and might cause trouble by consuming an upstream value in the caller. This did happen once before :) Then you can just save on lines 131-132. Functions which destroy their arguments are "unexpected" and best not to provide for. See previous comment on reforming "headings" in any case. Now I read this function more closely I realise the previous comment needs some refinement. It may seem heretical, but this might be a function which is more clearly expressed non-recursively :) I recommend splitting into two functions as a non-recursive 2-step process:
> 
> step 1) transform DOM headings + anchors into a flat structure [{text: text, url: url, level: level}]

This is funny, I actually had a function that produced a structure that looked identical to this. However I just merged it into the current function to save looping through the data. I'll just resurrect the code, but make some adjustments to it.

https://github.com/jobara/infusion/commit/74b9097aa659a9f128eb1a0fb9a5a80350ba86d6#L0L117

> step 2) hierarchicalise this structure as a pure "model transformation", scanning through it looking for upticks and downticks in the value of "level"
> 
> What happens in the case that the sequence in the document is something perverse like
> <h3>
> <h4>
> <h1>
> ?

This case actually worked in the current state. This was handled by the while loop and recursion. So the while loop would handle all headings at a give level, with and recursion to handle lower levels. I tried, but couldn't conceptualize how to do this without recursion. I've settled on basically keeping the same function but moving it down a level so that it is an internal function called within the main toModel method. This way I can provide a buffer in between to preserve the incoming array. I did also make some other refinements here and there.

> 
> The advantage of the refactoring I describe is that lots of test cases can be issued against the "step 2" function in isolation.

It should need a DOM anymore and should  be usable to run tests against. 

> 
> Line 95: Slight inconsistency of style here - on this line you check >0 whereas on 106 you simply use falsity for what is really the case - you should centralise on one form (I would prefer the former)

I switched to checking if it was > 0.

> Line 121: any particular reason to avoid returning []? I think it is a reasonable encoding of a condition of "having no headings".

I've gotten rid of that bit.

> 
> Line 124: Probably better expressed as "micro-component" with the literal array in defaults. You never know when someone might want to customise it (even though it is just 2 lines, they would probably be more comfortable just seeing data)

Converted this to a component.

> 
> Line 158: I can only heartily regret this necessity as a result of framework deficiencies :P It is a really excellent effort at making the best of a bad job... line 195 is just awful though, I recommended to JURA that he not tried to manipulate unexpanded trees but I guess there is just not very much to be done. It will just have to stand as an incentive to implement ANTIGENS as fast as possible...
> 
> Line 207: The constant 6 should not be coded here but derived from the same configuration we put into the headingLevel component.

So I thought about this a bit, and I don't think it quite fits. At least not the way I've structured the headingLevel component (which i now call headingCalculator). All it's concerned with is telling what level a given heading has. Also if someone were to swap in a different component here, there could be a totally different means of calculating the level that may not show up like an array in the configuration. Actually I might have to write another function if we want to support HTML5 headings. Which are based on nesting level within sections on the page, rather that specifically by heading tag. 

It tried to think of a way to calculate this, but could really think of anything really desirable. Please let me know if you have more suggestions about this. For the timebeing. I've set a "maxLevel" option in the levels componet's defaults. Figure this is at least a bit better.

> 
> Line 225: Presumably the vestigial "events" can go

oops yes.. removed that one.
> 
> The component is hugely improved over its previous implementation and should be much less work to convert over to its final form once the framework is improved.
> 
> Cheers,
> Boz
> 
> 
> On 25/05/2011 20:24, Justin Obara wrote:
>> Hi Antranig,
>> 
>> Hope you had a good day JA-SIGing.
>> 
>> So I'm wondering if you will have some time to take a quick look at the work I've done so far for the tableOfContents component.
>> 
>> I've tried to rewrite it in the new IoC and protocomponent style, and fixing up markup and etc. along the way. Please feel free to let me know of any irrational, insane, and/or immoral aspects of it.
>> 
>> code:
>> https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/components/tableOfContents/js/TableOfContents.js
>> 
>> template:
>> https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/components/tableOfContents/html/TableOfContents.html
>> 
>> A demo you can try.
>> https://github.com/jobara/infusion/blob/FLUID-4209/src/webapp/standalone-demos/table-of-contents/html/TableOfContents.html
>> 
>> I have yet to fix up and expand the unit tests, but this will be done before I make an official pull request. The reason for this, which admittedly may not be a good one, is that from the start I had been experimenting and had not really settled on anything. I've actually gone back and rewritten the model builder, the prototree, and even the template several times. I suppose better unit tests may have helped some of these rewrites.
>> 
>> Anyways, the major issue that I'm having now, is that when I have a case where the headings are not in consecutive order I get one repeated to fill in the gap. You can see this if you run the demo.
>> 
>> example from demo:
>> 
>> - Mammals
>> 	- Humans
>> 		- Humans
>> 			- CATTS
>> 
>> should have been:
>> 
>> - Mammals
>> 	- Humans
>> 			-CATTS
>> 
>> I think that both the model and the prototree are correct. Although I could have missed something. However, I'm a bit stumped as to why the one Heading gets repeated to fill in the missing level.
>> 
>> I'm hoping you can point me in the right direction to solve this issue, as well as providing some guidance on the architecture of the component, or more correctly collection of components, as a whole.
>> 
>> Thanks
>> Justin
> 