Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui:repeat clarification on attributes, such as offset and size #1713

Open
volosied opened this issue Nov 4, 2022 · 12 comments
Open

ui:repeat clarification on attributes, such as offset and size #1713

volosied opened this issue Nov 4, 2022 · 12 comments
Labels
EE12 mojarra-implemented API issue implemented by Mojarra
Milestone

Comments

@volosied
Copy link
Contributor

volosied commented Nov 4, 2022

There are behavior difference between Mojarra and MyFaces regarding ui:repeat.

The VLDDoc is vague, in my opinion. The main one I'm concerned about if offset="1" size="2", but I would appreciate any feedback.

For example:

        <ui:repeat id="repeat" value="#{['B1', 'B2', 'B3', 'B4', 'B5']}" var="row"   ...  >
            <h:outputText id="outputText" value="#{row}" />
        </ui:repeat> 

        
        Myfaces 4.0.0-RC2 (would be the same as 3.0, 2.3) :
        offset="2" : B3B4B5
        size = "3" : B1B2B3
        offset="2" begin="1" : ""
        begin="1" size="1" : LocationAwareFacesException: begin cannot be greater than end
        offset="1" size="2" : B2B3


        Mojarra 4.0.0: 
        offset="2" : B3B4B5
        size = "3" : B1B2B3
        offset="2" begin="1" :B2B3B4B5
        begin="1" size="1" : ""
        offset="1" size="2" : B2

These are just a few differences, but there are some more I've found (1). In general, the specification is vague and this has results in differences between the handling. I'm confused overall now in the proper behavior. In the simple cases of offset and size, things behave as expected. However, mixing the two produce different results.

Where does offset occur? From the begin value? From the first item in the collection? Should it be disregarded in some scenarios? What should occur if begin is a negative value?

I have similar questions for size. What does size mean? Is the simply the number of iterations to take? Or does it mean something else? Where does it start? Is size an index or a value? How does it relate to offset, begin, the first item in the collection, and the actual length of the collection? How is size different from end?

For size, the spec also mentions: If this value is less than the actual size of the collection, a FacesException must be thrown. but either implementation throws an exception?

Thanks!

  1. When should an empty model bee created? Should negative indices be supported (i.e. begin="-2") when iterating an empty collection (i.e begin=-2 end=3? Which has precedence if both end and size are specified?
@volosied volosied changed the title ui:repeat clarification on offset and size ui:repeat clarification on attributes, such as offset and size Nov 4, 2022
@volosied
Copy link
Contributor Author

volosied commented Nov 5, 2022

Found some information here which I'll need to review:

  • https://softwarecave.org/2014/04/18/facelets-uirepeat-tag/

    • Article says size is the number of iterations. and offset the index from which the iteration should start. Using this, it looks like MyFaces is correct with it's offset="1" size="2" handling since it outputs B2B3 (two iterations)

    • However, even MyFaces breaks down using these definitions if you throw step into the mix. For example, the code below produces B2, but I would expect it to produce B2B4. (offset skips B1, loop starts at B2 and steps by 2 to B4. and ends there since the iteration occurred twice) I suppose the problem here is how size is set. If size is treated as an index and it's set to end ( i.e. end = size -1) then the loop would stop at B3 and never reaches B4.

      <ui:repeat id="repeat" value="#{['B1', 'B2', 'B3', 'B4', 'B5']}" var="row" offset="1" size="2" step="2" >
        <h:outputText id="outputText" value="#{row}" />
      </ui:repeat>
      
  • https://github.com/jakartaee/faces/issues/1058 :

    • Mentions that purpose of the size attribute is to specify the number of elements or items to iterate over in the overall collection
  • https://github.com/jakartaee/faces/issues/1102#issuecomment-518592825 :

    Whilst the ui:repeat offset/size attributes have the same effect as c:forEach begin/end, during iteration the ui:repeat will still check DataModel#isRowAvailable(). In other words, presence of the value attribute referring the desired amount of items is still required. This is not what is being requested here.
      
    Further I also noticed in the Mojarra ui:repeat source code that it actually supports the literal begin/end attributes. The offset is just an overriding alias for begin, and the end defaults to size when unspecified. As those attributes have a clearer meaning, I will also document those attributes." 
    
  • https://issues.apache.org/jira/browse/MYFACES-3034

    • "On Mojarra 2.0.4, the size attribute is taken to mean the position of the last element to retrieve, not the number of elements to retrieve."

Code

@BalusC
Copy link
Member

BalusC commented Dec 3, 2022

History

Initially, ui:repeat had only offset/size/step attributes. However, this didn't allow to iterate a predefined amount of times. I.e. the following attempt to render 10 divs didn't work because it still requires the value attribute to be set.

<ui:repeat offset="1" size="10" var="i">
    <div>div number #{i}</div>
</ui:repeat>

In JSF 2.2, begin/end was added via eclipse-ee4j/mojarra#2244 allowing this to work based on how the <c:forEach> was implemented as per https://jakarta.ee/specifications/tags/1.2/tagdocs/c/foreach:

<ui:repeat begin="1" end="10" var="i">
    <div>div number #{i}</div>
</ui:repeat>

However, this was in hindsight not terribly well thought out. The primary mistake was that the begin attribute completely overlapped the existing offset attribute in the actual implementation. And the meaning of the size attribute collided with the end attribute. The original thought was that begin/end should only and only be used when the value attribute is not set, and that they should be ignored or even throw an exception when the value attribute is set. I think explicitly mentioning this in the spec would be the most cleanest way to go forward.

Current spec

  • begin: First item of the collection has index 0. If value not specified: Iteration begins with index set at the specified value.
  • end: If value specified: Iteration ends at the item located at the specified index (inclusive). If value not specified: Iteration ends when index reaches the specified value (inclusive).
  • offset: Read-write property setting the offset from the beginning of the collection from which to start the iteration. If not set, this offset is not considered and iteration will start at the beginning of the collection.
  • size: Read-write property setting the size of the collection to iterate. If this value is less than the actual size of the collection, a FacesException must be thrown.
  • step: Iteration will only process every step items of the collection, starting with the first one.

New spec proposal

Primary change: begin/end MAY NOT be used when value attribute is specified (even if it evaluates to null or empty) and the offset and size MUST require the value attribute (even if it evaluates to null or empty).

  • begin: If the value attribute is not specified: iteration begins with the specified number (inclusive) as item. If the value attribute is specified: a FacesException must be thrown. If the corresponding end attribute is not specified: use 0 as default. The corresponding end attribute may be less than the begin attribute: iteration will take place in a reversed manner.
  • end: If the value attribute is not specified: iteration ends with the specified number (inclusive) as item. If the value attribute is specified: a FacesException must be thrown. If the corresponding begin attribute is not specified: use 0 as default. The corresponding begin attribute may be greater than the end attribute: iteration will take place in a reversed manner.
  • offset: If the value attribute is specified: iteration begins with the specified number as index. If the value attribute is not specified: a FacesException must be thrown. If the offset attribute is not specified: use 0 as default. If the offset attribute is less than 0 or is greater than the size of the actual collection behind the value attribute: a FacesException must be thrown.
  • size: If the value attribute is specified: iteration ends when the specified number of times has been iterated (inclusive). If the value attribute is not specified: a FacesException must be thrown. If the size attribute is not specified: use the size of the actual collection behind the value attribute as default. If the sum of the size attribute and the offset attribute is less than 0 or is greater than the size of the actual collection behind the value attribute: a FacesException must be thrown. If the step attribute is specified: each skipped item must also be counted for the size attribute.
  • step: Iteration will only process every step items of the collection, starting with the first one. If the step attribute is less than 1: a FacesException must be thrown.

Optional: support a negative step. I.e. step="-1" will iterate backwards. This will be a new feature.

Future spec proposal

Merge offset into begin so that no FacesException will be thrown depending on the presence of the value attribute. But this will require a Major version release because of breaking changes. Also it should be looked at if end could be merged into size. But I think this will end up in less intuitive API.

TCK proposal

<ui:repeat begin="1" end="3" var="i"> #{i} </ui:repeat> must render 1 2 3
<ui:repeat begin="-3" end="3" var="i"> #{i} </ui:repeat> must render -3 -2 -1 0 1 2 3
<ui:repeat begin="3" end="-3" var="i"> #{i} </ui:repeat> must render 3 2 1 0 -1 -2 -3
<ui:repeat begin="-3" end="3" step="2" var="i"> #{i} </ui:repeat> must render -3 -1 1 3
<ui:repeat begin="-3" end="3" offset="1" var="i"> #{i} </ui:repeat> must throw FacesException (offset requires value attribute)
<ui:repeat begin="-3" end="3" size="2" var="i"> #{i} </ui:repeat> must throw FacesException (size requires value attribute)
<ui:repeat begin="3" end="3" var="i"> #{i} </ui:repeat> must render 3
<ui:repeat begin="3" var="i"> #{i} </ui:repeat> must render 3 2 1 0
<ui:repeat begin="-3" var="i"> #{i} </ui:repeat> must render -3 -2 -1 0
<ui:repeat begin="0" var="i"> #{i} </ui:repeat> must render 0
<ui:repeat end="3" var="i"> #{i} </ui:repeat> must render 0 1 2 3
<ui:repeat end="-3" var="i"> #{i} </ui:repeat> must render 0 -1 -2 -3
<ui:repeat end="0" var="i"> #{i} </ui:repeat> must render 0
<ui:repeat var="i"> #{i} </ui:repeat> must render nothing
<ui:repeat begin="1" end="3" value="#{['A', 'B', 'C', 'D', 'E']}" var="i"> #{i} </ui:repeat> must throw FacesException (begin and end disallow value attribute, instruct user to use offset/size instead)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="2"> #{i} </ui:repeat> must render C D E
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="-1"> #{i} </ui:repeat> must throw FacesException (offset less than 0)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="6"> #{i} </ui:repeat> must throw FacesException (offset greater than item count)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" size="2"> #{i} </ui:repeat> must render A B
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" size="-1"> #{i} </ui:repeat> must throw FacesException (size less than 0)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" size="6"> #{i} </ui:repeat> must throw FacesException (size greater than item count)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="2" size="2"> #{i} </ui:repeat> must render C D
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="2" size="4"> #{i} </ui:repeat> must throw FacesException (offset plus size greater than item count)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" step="2"> #{i} </ui:repeat> must render A C E
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" step="0"> #{i} </ui:repeat> must throw FacesException (step less than 1)
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="1" step="2"> #{i} </ui:repeat> must render B D
<ui:repeat value="#{['A', 'B', 'C', 'D', 'E']}" var="i" offset="1" step="2" size="2"> #{i} </ui:repeat> must render B

Thoughts? cc: @arjantijms and @tandraschko

@pizzi80
Copy link

pizzi80 commented Jul 3, 2023

+1 for Faces 5 proposal with fusion of begin / end with offset / size

Less things to learn, less obscure logic ...

In this way it's straightforward like an old style loop with index ...

@BalusC
Copy link
Member

BalusC commented Jul 22, 2023

@arjantijms @tandraschko @volosied +1 or -1 for 4.1 spec update proposal? And the support for a negative step?

@BalusC BalusC added this to the 4.1 milestone Jul 22, 2023
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jul 23, 2023
Improved spec wrt ui:repeat attributes
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jul 23, 2023
Fixed impl as per improved spec on ui:repeat attributes
@volosied
Copy link
Contributor Author

volosied commented Aug 2, 2023

Hi, a recent faces-dev email had discussions about changes between releases.

The 4.1 proposal here for ui:repeat looks fine to me, but it would introduce breaking changes via the new behavior (exceptions), and I'm not sure if that should go into 4.1, per the guidelines set in the document:

Minor version increments: Backwards compatible new behaviour.

Should this be pushed back into 5.0? @BalusC

@BalusC
Copy link
Member

BalusC commented Aug 2, 2023

Ok sure.

@BalusC
Copy link
Member

BalusC commented Oct 5, 2023

How about the support for a negative step?

Optional: support a negative step. I.e. step="-1" will iterate backwards. This will be a new feature.

This can be very useful at times. We now have the opportunity to implement it for 5.0.

BalusC added a commit that referenced this issue Oct 14, 2023
TODO: this isn't working, GlassFish 7 throws OSGI exception that Faces
SPI cannot be higher than 5.0.0
@BalusC BalusC added the mojarra-implemented API issue implemented by Mojarra label Sep 3, 2024
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Sep 16, 2024
change was projected for 4.1 but ultimately postponed to 5.0 because of
the breaking nature; jakartaee/faces#1713
BalusC added a commit that referenced this issue Dec 15, 2024
@arjantijms arjantijms added the EE12 label Jan 8, 2025
@tandraschko
Copy link

so whats left here?
I can see changes in MF

is just the backward -1 step left?

@BalusC
Copy link
Member

BalusC commented Jan 21, 2025

is just the backward -1 step left?

Yes.

@melloware
Copy link

@BalusC i don't see in your TCK examples above what is the expected if step="-1" ?

@BalusC
Copy link
Member

BalusC commented Jan 28, 2025

It currently throws FacesException for any step lower than 1.

It was just a proposal for a new feature because this was not formally/clearly supported by previous spec. See also #1713 (comment)

Can we agree we should support a negative step? If so then we can adjust 5.0 spec on this and adjust TCK test to cover this. If not then we just leave it as is.

Something like:

step: Iteration will only process every step items of the collection, starting with the first one. A negative step will iterate backwards. If the step attribute is not specified: use 1 as default. If the step attribute equals 0: a FacesException must be thrown.

BalusC added a commit that referenced this issue Jan 28, 2025
BalusC added a commit to eclipse-ee4j/mojarra that referenced this issue Jan 28, 2025
@BalusC
Copy link
Member

BalusC commented Jan 28, 2025

@melloware see above commits for example spec/impl/tck (although I think it's probably easier/clearer to say "reverse the collection" instead of "iterate backwards" and we also need to look closer at iteration status model and assert these as well in tck).

If we all agree ui:repeat should support negative step then I'll go ahead and create PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EE12 mojarra-implemented API issue implemented by Mojarra
Projects
None yet
Development

No branches or pull requests

6 participants