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

Customizable repeated section iteration title #4117

Closed
ebruchez opened this issue Jul 3, 2019 · 22 comments
Closed

Customizable repeated section iteration title #4117

ebruchez opened this issue Jul 3, 2019 · 22 comments

Comments

@ebruchez
Copy link
Collaborator

ebruchez commented Jul 3, 2019

Following #4064, this additional requirement is to provide more flexibility for the title of each repeated section, including:

  • Providing a template for the title.
  • Filling the template with the value of a control within the iteration.

+1 from customer

See also #3911 about the "Add" button label.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Jul 3, 2019

Ideally, this would be a template within the "Section Settings" dialog.

Should we have two labels, a "main" label and a label for iterations subtitles? Probably.

Where would the label be stored? Maybe as a nested element:

<fr:section ...>
    <xf:label .../>
    <fr:iteration-label>
        <fr:param type="ExpressionParam">
            <fr:name>cool</fr:name>
            <fr:expr>42</fr:expr>
        </fr:param>
    </fr:iteration-label>
    <fr:grid>
        ...

The iteration label must also be localizable, which means it must be stored within resources. This means that Form Builder must be aware of it.

@avernet
Copy link
Collaborator

avernet commented Jul 3, 2019

Could we just have 1 label for the section (instead of <xf:label> and <fr:iteration-label>)? So one would always be able to use a template for a section title, whether repeated or not.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Jul 3, 2019

In wizard mode, that might make sense. But in non-wizard mode, or for nested subsections, I was thinking that you might want to see the main title for the section and then individual subtitles for iterations.

That might be overkill for now. But it remains that in effect, the label/title would function very differently in both cases if we have only one:

  • Wizard mode: title/label is repeated. If it uses templates, then the title controls/formulas evaluate in the context of each iteration.
  • Non-wizard mode: single title/label for all iterations. If it uses templates, then the title controls/formulas evaluate in the context of the section binding, probably.

It is annoying that the label would evaluate in two different ways depending on how you decide to represent the sections.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Jul 3, 2019

HTML support is required, like for any label/title.

@avernet
Copy link
Collaborator

avernet commented Jul 4, 2019

I am not sure to follow. Are you also thinking of introducing 2 levels of title for repeated sections? That is, you would have:

  • Children [from <xf:label>]
    • Child 1: Bart [from <fr:iteration-label>]
    • Child 2: Lisa [from <fr:iteration-label>]

If what you have in mind, is it really necessary?

@ebruchez
Copy link
Collaborator Author

ebruchez commented Jul 4, 2019

It is not a necessary feature for now, in the sense that nobody is asking for it. But it follows conceptually from the notion that a title either evaluates in the context of a section, or in the context of an iteration. I don't like the idea that something evaluates differently depending on whether you enable a different appearance for top-level repeated section iterations. We don't have other places where expressions evaluate differently depending on an appearance or presentation.

@avernet
Copy link
Collaborator

avernet commented Jul 4, 2019

Just to make sure I understand this well, the question is whether repeated sections are shown to users as, option A:

  • Children [from <xf:label>]
    • Child 1: Bart [from <fr:iteration-label>]
    • Child 2: Lisa [from <fr:iteration-label>]

Of just, option B:

  • Child 1: Bart [from <fr:iteration-label>]
  • Child 2: Lisa [from <fr:iteration-label>]

How top level repeated sections show in the wizard could be an orthogonal question, but I'd say that it would be nice if we have the same capability there, i.e. if we go with option A, the wizard would show the two levels Children / Bart, Lisa.

I like option A, but it adds more complexity (more UI). And if we go with option B, I don't really see an inconsistency in the way the XPath expressions in the label template are evaluated: if you have a single non-repeated section, XPath evaluates in the context of that section; if you have repeated sections, XPath evaluates in the context of the "current section" (which you can call iteration). Or am I missing something?

@ebruchez
Copy link
Collaborator Author

Either way, it doesn't seem we can avoid having a specific construct to indicate the iteration label, as opposed to having only <xf:label>. So we don't escape that extra complexity.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Aug 12, 2019

Steps:

  • runtime: create expression from fr:iteration-label
    • process expression
    • decide where to pass result
  • bug: Form Builder: shows some control values on the left
  • runtime: <fr:section>/<fr:repeater> uses expression within iterations
  • wizard: use expression within TOC iterations
  • use-paging shouldn't be enabled at design-time
  • bug: we see all iteration labels in the section at runtime
  • builder: read, edit and write template
  • dialog: use "Repetition Label" instead of "Iteration Label"
  • bug: insert iteration in the middle and title temporarily has incorrect control value
  • tests
  • doc
    • feature
    • xxf:value()'s 2nd parameter
  • P2: other functions should support 2nd parameter

@ebruchez
Copy link
Collaborator Author

ebruchez commented Aug 13, 2019

Possible syntax after runtime transformation, using an iteration-label attribute:

<fr:section
    id="my-repeated-section-control"
    bind="my-repeated-section-bind"
    repeat="content"
    min="1"
    iteration-label="xxf:r('my-repeated-section.iteration-label','fr-form-resources',map:merge(...))"
    template="instance('my-repeated-section-template')">
    <xf:label ref="$form-resources/my-repeated-section/label"/>

or:

  • iteration-label-ref
  • iteration-label-value
  • or iteration-label with AVT content

Another option is a nested element, but would it be any better? The main benefit I can see is ease of processing: xf:label remains xf:label, fr:iteration-label remains fr:iteration-label.

@ebruchez
Copy link
Collaborator Author

In the wizard, do we still show the section label:

  1. In the TOC?
  2. In the body?

We could show the section label in the TOC and indent the iteration label below. Similarly, we could show both label and iteration label in the body.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Aug 14, 2019

One issue now is how to get the iteration label in the wizard TOC.

I started by evaluating fr:iteration-label/@ref in the context of the TOC repeat. But that doesn't work, because that repeat is different from the repeat within fr:repeater, and therefore fr:control-typed-value() searches for all the values and always returns the first one.

Then I figured we could instead directly get the value of the iteration label, with:

array:get(
    fr:control-typed-value(
        '{$static-section-id}-repeater-iteration-label',
        false()
    ),
    xxf:repeat-position()
)

but that doesn't work either, because fr:control-typed-value() searches the control's bindings, and right now the iteration label has xf:output/@value and no binding.

Which raises the question of why we are using fr:control-typed-value() or why that function was done this way. It also supports getting the value from binds.

We should have an option, or another function, to obtain the values from the controls. We could also get the formatted values directly from the control. This is not the first time we have this issue, see:

@ebruchez
Copy link
Collaborator Author

With 2019.1, we have xxf:formatted-value(), but it follows a resolution different from fr:control-typed-value(). We should maybe introduce fr:control-formatted-value(), which would find controls like fr:control-typed-value(), but call formattedValue on the control instead.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Aug 15, 2019

Thinking about it, it's not crazy that the Form Runner functions use bound nodes since all Form Runner controls have bindings, except maybe the "Explanatory Text" control, which works differently anyway (storing resources).

What we heed here, that is, obtaining the value of an <xf:output> control without a binding, is not really in the realm of fr: functions. It should be an xxf: function.

Now the xxf: functions use XBLContainer.resolveObjectById to resolve relative to the location of the function call, delegating to Controls.resolveObjectById, which calls resolveControlsById by passing followIndexes = true.

The fr: functions use FormRunner.resolveTargetRelativeToActionSourceOpt, which first try resolveTargetRelativeToActionSourceFromControlsOpt, which calls Controls.resolveControlsById by passing the specified followIndexes. If that fails, binds are searched as well with resolveTargetRelativeToActionSourceFromControlsFromBindOpt.

So in both cases, we get to resolveControlsById (if we exclude searching binds). It's just that with the xxf: functions, we always have followIndexes = true.

One possibility is to allow passing followIndexes = false from the xxf: functions. This would make xxf:formatted-value() usable from fr:wizard to obtain the iteration label.

@ebruchez
Copy link
Collaborator Author

@ebruchez
Copy link
Collaborator Author

@ebruchez
Copy link
Collaborator Author

ebruchez commented Sep 4, 2019

Confirming insert bug where inserting causes the wizard TOC to show the same label for all repetitions, temporarily. After even a tab out the TOC shows the correct content:

Screen Shot 2019-09-04 at 11 51 17 AM

@ebruchez
Copy link
Collaborator Author

Investigating that bug. We are using xxf:value() in the wizard TOC to get the value of all detail-section-section-repeater-iteration-label, which is the <xf:output> control that shows the iteration title. The control obtains its value like this:

xxf:r(
    'detail-section.iteration-label',
    'fr-form-resources',
    map:merge(
        (
            map:entry(
                'traveler-name',
                for $a in fr:control-typed-value('traveler-name-readonly', false())
                    return if (array:size($a) = 0) then () else array:get($a, 1)
            )
        )
    )
)

Just after insert, fr:control-typed-value() runs twice. First, for detail-section-section-repeater-iteration-label⊙2, second, for detail-section-section-repeater-iteration-label⊙1. (Not sure why the order is reversed.)

But for detail-section-section-repeater-iteration-label⊙2, fr:control-typed-value() returns (`abc`,```), that is, two values! And the first one is used, therefore the extra abc. This is wrong.

After a further refresh, we don't have this problem.

@ebruchez
Copy link
Collaborator Author

The reason is that when creating the new iteration, the <xf:output>'s binding AND value are evaluated before the traveler-name-readonly control is indexed and evaluated.

This could be fixed if evaluating values was done in a different phase after evaluating bindings, since the fr:control-typed-value() only uses the control's binding. Ideally, there should be a dependency so that even if a control depends on another control's value, things would work.

Evaluating values could be done lazily, but we have to be careful because this depends on the XPath dependencies and that means that the state of dependencies must be ready. If it's during refresh anyway, that would be fine. Right?

@ebruchez
Copy link
Collaborator Author

Ideally, we should have a more encompassing dependency system which would allow us to determine a partial evaluation order as we do for the bind variables now. But that is a larger change.

For now, we'd like, ideally, a change which addresses this specific problem without introducing others.

For example, we could decide that we split out the evaluation of bindings and that of values during refresh. This would be largely compatible with what we have now and, hopefully, only enable more scenarios which don't work right now, like the one in this bug. But this is also very limited: it only addresses issues where a control value depends on finding a control appearing in the same repeat iteration and using its binding. Very limited, but it would work.

We could also make the evaluation of control values lazy. But we can only be a little bit lazy. We need to have the values evaluated before the refresh is done, otherwise values might change in unexpected ways: for example, refresh is done, model action runs and a function gets a control value, which evaluates at that time, and obtains the value from the data model, which might have changed since the last refresh. In this case, we could have surprising results, certainly ones that would be incompatible with what we have now.

If we go the lazy way, we also need to check dependencies before any event is dispatched, so that we check the right state of dependencies. So we might have:

  1. Evaluate binding.
  2. Check if value needs re-evaluation (pathmap) right there and if so set a flag (or just null).
  3. When all bindings are evaluated, make sure each control that needs evaluating its value is hit. The value is evaluated if the flag is set, and the value is then set and the flag cleared. If a control depends on another control's value, then the value will be evaluated as well. In the end, all control values are sure to be evaluated.

Differences with the first solution:

  • This covers more scenarios: dependency on bindings, but also on other values.
  • Might require less code.

@ebruchez
Copy link
Collaborator Author

ebruchez commented Sep 16, 2019

I worked on making the evaluation of value controls lazier. The problem I have now is that fr:wizard uses an intermediate <xf:var name="section-label">, and the evaluation of the variables is strict. This is harder to handle because we push variables on a stack, and we have to pass the value immediately. The value should be lazy too, à la Cats Eval.later, but that means changing things down to XPathCache and related.

An easier solution is to remove the use of a variable for this purpose.

ebruchez added a commit that referenced this issue Sep 17, 2019
ebruchez added a commit that referenced this issue Sep 17, 2019
ebruchez added a commit that referenced this issue Sep 17, 2019
ebruchez added a commit that referenced this issue Sep 17, 2019
@ebruchez
Copy link
Collaborator Author

Tentative fix is in. But there is another issue when reordering controls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants