UIRepeat: Improve detection of repeating parents

[melloware]

Originally Reported at PrimeFaces: primefaces/primefaces#7777

PrimeFaces has a custom p:repeat component to fix an issue with Mojarra ui:repeat that is not present in MyFaces.

Reproducer:
pf-7777.zip

mvn clean jetty:run -Pmojarra22 = broken
mvn clean jetty:run -Pmojarra23 = broken
mvn clean jetty:run -Pmyfaces22 = WORKS!
mvn clean jetty:run -Pmyfaces23 = WORKS!

The issue comes from this custom code in PF Repeat:

PrimeFaces:

private boolean isNestedInIterator() {
        UIComponent parent = this;
        while (null != (parent = parent.getParent())) {
            if (parent instanceof javax.faces.component.UIData || parent.getClass().getName().endsWith("UIRepeat")
                    || (parent instanceof UITabPanel && ((UITabPanel) parent).isRepeating())) {
                return true;
            }
        }
        return false;
    }

Mojarra:

 private boolean isNestedInIterator() {
        UIComponent parent = this.getParent();
        while (parent != null) {
            if (parent instanceof UIData || parent instanceof UIRepeat) {
                return true;
            }
            parent = parent.getParent();
        }
        return false;
    }

MyFaces doesn't do this at all and it works for the use case above. Any ideas or suggestions on how to improve UIRepeat for Mojarra so PF can remove its custom p:repeat ???

cc @BalusC @arjantijms

[arjantijms]

Thanks for the rapport, need to take a closer look at this, as it's all very subtle (and brittle, unfortunately).

[BalusC]

There's clearly need for a common interface or abstract class for all those iterating components like UIIterable.

[melloware]

I think a either a Marker interface like UIIterable or if it had a single method like boolean isRepeating(); would be great. I think its more appropriate than an Abstract Class. But thanks for considering it!

cc @tandraschko

[tandraschko]

i think it makes sense in general (but prefer a interface) but it would also make sense to improve the Mojarra algorithm, so that check isnt even needed like in myfaces

[BalusC]

but it would also make sense to improve the Mojarra algorithm, so that check isnt even needed like in myfaces

I ran <ui:repeat> related integration tests of Mojarra using MyFaces and I found the IT of #3837 (originally 3833 at java.net issue tracker) to be failing when using MyFaces 2.3.9 instead of Mojarra 2.3.17. So I think the Mojarra algorithm is still relevant in such case.

For the record, this is the (simplified) use case:

view:

<h:form id="form">
    <ui:repeat value="#{issue3833Bean.lists}" var="list">
        <h:selectOneListbox valueChangeListener="#{issue3833Bean.listener}" size="2">
            <f:selectItems value="#{list}" />
            <f:ajax render="form:message" />
        </h:selectOneListbox>
    </ui:repeat>
    <h:panelGroup id="message" layout="block">
        <h:outputText value="#{issue3833Bean.message}" />
    </h:panelGroup>
</h:form>

model:

@Named
@RequestScoped
public class Issue3833Bean {

    private static List<String> list1 = Arrays.asList("1", "2");
    private static List<String> list2 = Arrays.asList("3", "4");
    private static List<List<String>> lists = Arrays.asList(list1, list2);

    private String message;

    public void listener(ValueChangeEvent e) {
        message = (e.getOldValue() + " -> " + e.getNewValue());
    }

    public List<List<String>> getLists() {
      return lists;
    }

    public String getMessage() {
        return message;
    }
}

reproducer:

1. click "1", message should say "null -> 1"
2. click "3", message should say "null -> 3"
3. click "2", message should say "1 -> 2", but MyFaces said "null -> 2"
4. click "4", message should say "3 -> 4", but MyFaces said "null -> 4"

[tandraschko]

let me check that case

[BalusC]

I feel like simply scanning the client ID for an iteration index pattern is sufficient for now. A numeric component ID is already disallowed by spec and iterable component implementations as far as I know already follow the same client ID pattern of "clientId:[0-9]+:childId".

[tandraschko]

i can verify the issue but i feel it could be fixed without introducing the "isNestedInIterator" check but i need some time to debug

[BalusC]

I found this reproducer to be also failing when <h:dataTable> is used in Mojarra 2.3.17 and the issue 3833 IT to be failing as well for <h:dataTable> of MyFaces 2.3.9. I'll have to reapply the same fix in UIData for now.

[BalusC]

i can verify the issue but i feel it could be fixed without introducing the "isNestedInIterator" check

Would be great indeed if that's possible.

[tandraschko]

please see:
https://issues.apache.org/jira/browse/MYFACES-4425

it seems that in MF the state isnt always saved - only in some special cases

i normal cases, the "value" is restored from the ValueBinding, which isnt available in this case
i could fix it by always save it but this mean more viewstate usage, even not required in 99% of the cases
this is issue propably never occured
in 2.0 and 2.1 we NEVER saved any state for UIRepeat

[BalusC]

Great. I'll look if I can retrofit MyFaces state saving approach into Mojarra's ui:repeat (and h:dataTable!).

[BalusC]

Great. I'll look if I can retrofit MyFaces state saving approach into Mojarra's ui:repeat (and h:dataTable!).

It was less trivial than expected so I've postponed that for later.

The initial PR is now at least merged into 2.3/3.0/4.0.