Forum OpenACS Development: Nested Lists

Collapse
Posted by Benjamin Bytheway on
I noticed in a recent cvs log from donb that he mentioned some portion of portals would be easier if nested list tags were allowed. I wondered what was keeping them from being used and started digging into the code.

It turns out most of the functionality to use nested lists already exists in templating. The one problem keeping it from working is that the for loops use the same index variable. I fixed this in my site and nested loops immediately started working.

As I glanced through the templating code, I noticed an undocumented feature of lists was the ability to pass a list datastructure into the list by using a value="" parameter. You name this "new" list by using the name="" parameter, just as you would if the list originated from the tcl file. That's what makes everything work. Happily, this fix should be completely backward compatible.

Here's what nested lists would look like in an adp file:

<list name="outer_list">
    Outer Item Number: @outer_list:rownum@<br>

    <list name="inner_list" value="@outer_list:item@">
        item: @inner_list:item@<br>
    </list>

    <br>
</list>

With an list structure of: { {1 2 3 4} {a b c d} }, the output result is:

Outer Item Number: 1
item: 1
item: 2
item: 3
item: 4

Outer Item Number: 2
item: a
item: b
item: c
item: d

To me, the most exciting thing is that this makes it possible to have a list structure as part of a multirow. Like this:

<multiple name="test">
    @test.name@<br>

    <list name="foo" value="@test.mylist@">
        item: @foo:item@
    </list>

    <br>
</multiple>

I guess the question is, would this be considered a bug-fix, or a new feature? Is a TIP in order, or can it be commited to the upcoming 5-1 tree?

Collapse
2: Re: Nested Lists (response to 1)
Posted by Benjamin Bytheway on
I suppose I should add that the fix does enable nested lists that both originate from the .tcl file, such as:

<list name="list1">
    @list1:item@:

    <list name="list2">
        @list2:item@
    </list>
    <br>
</list>
Collapse
3: Re: Nested Lists (response to 1)
Posted by Malte Sussdorff on
I think this is something we are missing greately, therefore I don't think we should object getting this into 5-1 and therefore allowing Don to make use of this in his portal package as well. And for the record, I think this is a bug fix.
Collapse
4: Re: Nested Lists (response to 3)
Posted by Joel Aufrecht on
We use the new-feature/bug fix dichotomy to set a gross barrier on risky changes to a release branch, but the real issue is, it is likely to hinder the release? For example, Lars improved the error message for certain circumstances of ad_form, but within a day I had stumbled across a legit usage of ad_form which was now broken and we had to roll the change back. We have a diverse enough code base that it's really not ever 100% safe to change any programmatic behavior. Of course that's not a practical guideline for us. So, what I would like to see for changes of this type is:

somewhere between 1 and 10 new automated test cases, showing that the change works and doesn't break stuff. The test cases should include

  • desired new usage
  • vanilla old usage that doesn't use the new function
  • example usage copied from current code base in core
  • a different class of example, if possible, from core
  • different classes of examples from standard packages or contrib
The test cases would then go into a file called /packages/acs-templating/tcl/test/acs-templating-test.tcl and a test case would look something like this (this code won't work as-is because I'm not sure how to use the adp processor internally, but it should be a starting point)
aa_register_case -cats {api} list__nested {
    Test the list tag processor.

    @author Joel Aufrecht
} {

    set example1_adp {<list name="outer_list">
    Outer Item Number: @outer_list:rownum@<br>

    <list name="inner_list" value="@outer_list:item@">
        item: @inner_list:item@<br>
    </list>

    <br>
</list>}

    set desired1 {Outer Item Number: 1
item: 1
item: 2
item: 3
item: 4

Outer Item Number: 2
item: a
item: b
item: c
item: d}

    set example_1_preparse [template::adp_compile -string $example1_adp]

    # this will not work because adp_eval doesn't return anything
    # and because this may mess up the current page processing
    # this needs to do some stack manipulation ...
    set example_1_text [template::adp_eval $example_1_preparse]
                            
    aa_true "Nested list parses successfully" [string equal $example1_text $desired1]

}
Collapse
5: Re: Nested Lists (response to 1)
Posted by Don Baccus on
Thanks, Benjamin, I was going to look into this today!  I figured that it was using the same variable internally and that was killing nestability.

As it turns out...

"To me, the most exciting thing is that this makes it possible to have a list structure as part of a multirow."

(the name="foo" value="list of values" construct) ...

I realized after committing the change with the comment you noticed that a multirow with the list passed as part of it would let me work around the issue.  Actually it turns out in this case the multirow was cleaner anyway.

So ... Malte ... I don't need this bug fix to do the minor clean-up to portals I was seeking.  I'm conservative about these things, anyway, and would agree with Joel that we not put stuff in that's not absolutely necessary.  I argued similarly in Peter's thread about group administration.

Benjamin if you e-mail me your patch for nested lists I'll apply it to HEAD ...

Collapse
6: Re: Nested Lists (response to 1)
Posted by Don Baccus on
Hmm...

" We have a diverse enough code base that it's really not ever 100% safe to change any programmatic behavior."

True but enabling nested lists doesn't change any *existing* behavior.  automated tests are nice but I really hate to raise the bar on simple changes that can be shown to not change existing behavior ...

Collapse
7: Re: Nested Lists (response to 6)
Posted by Joel Aufrecht on
I'm thinking in terms of, where do we draw the bug fix/new feature line on release branches. "Enabling nested lists" doesn't change any existing behavior, but we also need to ensure that the code change does "enabling nested lists" and nothing else, and that the way it does "enabling nested lists" doesn't break anything. Of course any bug fix has the same problem, but there's a spectrum from "the table name is misspelled in the Oracle query" which is a pretty darned safe fix, to something more subtle like, "this API call is confusing to invoke." This change is in the middle of the spectrum - not because the desired change is likely to be backwards-incompatible, but because it's a change to the code behind some widely used functionality.

So if we are going to change riskier stuff on a release branch, I think we should mitigate the risk via both code review and regression testing, whenever possible. We should do this for the hairier code bugs as well as for the design bugs.

Collapse
8: Re: Nested Lists (response to 7)
Posted by Benjamin Bytheway on
I commited the bugfix to HEAD earlier this morning.  I hope to add documentation to acs-templating for the undocumented name=""/value="" functionality, as well as give examples of what works and what doesn't with nested <lists>.

The ONLY thing I changed was to modify the index variable in a for loop.

"for {set __ats_i.." ==> "for {set __ats_${name}_i..."

This has no impact on current behavior. I will add some test cases in the next few days, to demonstrate the viability of the fix.  Hopefully it can be proved and moved to the 5-1 branch for release.

Collapse
9: Re: Nested Lists (response to 1)
Posted by Don Baccus on
"but there's a spectrum from "the table name is misspelled in the Oracle query" which is a pretty darned safe fix" ...

I wrote my comment above knowing that the fix Benjamin was describing was to simply generate a unique name for a local variable, similar in impact to your table name example.

However ... note my comment about conservatism above.  My gut reaction to changes like this is "HEAD, please, not our release branch".  On the other hand, this one's extremely safe  so I won't object to it going into 5.1 if folks decide that's appropriate.

But in general we shouldn't be putting new features into the release branch once it's feature-frozen, no matter how trivial or minimal they seem to be.