289534 - Controls using index() are not refreshed on index change

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

Comment 1

[Allan Beaufour]

Attachment: Testcase

The output in this testcase is not updated

Comment 2

[Allan Beaufour]

Hmmm, fixing this is not fun.

The repeat elements sets/changes the repeat-index. We then need to figure out
which controls use "index()" in their XPath expression, and then these need to
be rebound and refreshed. My best idea is to:
* let the repeat elements fire an event [1] on themselves when they change the
repeat-index [2]
* let the XPath parser/evaluator return the indexes used in an expression
* let the controls register themselves as listeners on the repeats which indexes
 they are using, and rebind+refresh themselves on the event

[1] Using events should make stuff work even though either a control or a repeat
is removed dynamically.

[2] The rationale for firing the event on the repeat element itself is that
index() is not bound to a specific model and the model could be change by
scripting. 

The spec says:
"The implementation data structures for tracking computational dependencies are
rebuilt or updated as a result of this action."
(http://www.w3.org/TR/xforms/slice9.html#action-setRepeatCursor)
And I've been told that this implicitly means whenever the repeat-index is
changed, even though it's written for setindex. But doing a full rebuild
everytime a repeat-index changes? Not really.

Comment 3

[Allan Beaufour]

Attachment: Rough patch

Here's a rough edition of a patch for the above. Problems:
1) I cannot yet get the events working (as usual :( ) 
2) The context for the index() function is wrong. The collection and evaluation
of the index parameter should be done by the evaluator, so the evaluated @id is
returned instead of the xpath expression.

Comment 4

[Allan Beaufour]

Attachment: Patch

This one works. I removed the event-business. There was really no need for it.
The patch still have two problems though:
* the context in evaluation of the index() parameter is wrong, the xpath
analyzer needs to handle it, not the parser.

* it does not handle bind elements using index() -- this will be another bug.

Comment 5

[Allan Beaufour]

Attachment: Patch v2, using analyzer

This one uses the analyzer to get the index ids so the context is right for the
evaluation. Should handle all controls.

Issues:
* Bind elements are still missing but I'll do another bug for that.
* We still have problems with using index() expressions, which I think is
caused by index() being evaluated to early. ie. before the controls is inserted
in the ocument, or something like that. Another bug for that too.

Added-value features :)
* Fixed a stupid bug in the parser regarding now()-detection
* Reconstructed nsXFormsUtils::GetNodeContext() so it always returns a context.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 181764 [details] [diff] [review]
Patch v2, using analyzer

First part of the review:

>+NS_IMETHODIMP
>+nsXFormsRepeatElement::AddIndexUser(nsIXFormsControl *aControl)
>+{
>+  return mIndexUsers.IndexOf(aControl) == -1 ? mIndexUsers.AppendObject(aControl) : NS_OK;
>+}

mIndexUsers.AppendObject() returns PRBool, not nsresult.

>+
>+NS_IMETHODIMP
>+nsXFormsRepeatElement::RemoveIndexUser(nsIXFormsControl *aControl)
>+{
>+  return mIndexUsers.RemoveObject(aControl);
>+}

mIndexUsers.RemoveObject() returns PRBool, not nsresult.

>+
>+NS_IMETHODIMP
>+nsXFormsRepeatElement::IndexHasChanged()
>+{
>+  ///
>+  /// @bug We need to handle \<bind\> elements too (XXX)
>+
>+  // copy the index array, as index users might add/remove themselves when
>+  // they are rebound and refreshed().
>+  nsCOMArray<nsIXFormsControl> indexes = mIndexUsers;

Does the '=' operator really work here?



> void
> nsXFormsXPathParser::FunctionCall()
> {
>-  if (!mUsesDynamicFunc) {
>-    nsDependentSubstring fname = Substring(mScanner.Expression(), mScanner.Offset() + 1, mScanner.Offset() + mScanner.Length() + 1);
>-    if (fname.Equals(NS_LITERAL_STRING("now"))) {
>-      mUsesDynamicFunc = PR_TRUE;
>-    }
>+  nsDependentSubstring fname = Substring(mScanner.Expression(),
>+                                         mScanner.Offset() + 1,
>+                                         mScanner.Length());

I assume the Substring is right this time ;)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 181764 [details] [diff] [review]
Patch v2, using analyzer


>-  if (mModel) {
>-    mModel->AddFormControl(this);
>-    if (aModel) {
>-      NS_ADDREF(*aModel = mModel);
>+  if (!mModel)
>+    return rv;

NS_ENSURE_TRUE(model, rv);


>+  /**
>+   * Array of repeat-elements of which the control uses repeat-index.
>+   */
>+  nsCOMArray<nsIXFormsRepeatElement>  mIndexesUsed;

nsCOMArray is ok too, but our form controls are getting quite heavy
and indexes may not be used very often, so nsSmallVoidArray could be one
option. Anyway nsCOMArray is good enough for now.


>+    if (aNode->mIsIndex) {
>+      nsDependentSubstring indexExpr = Substring(xp, 6, xp.Length() - 7);

Don't hardcode 6 and 7, use #defines. That would make it easier to read this
code.

Comment 8

[Allan Beaufour]

Attachment: Patch v2 w/smaug's comments fixed

(In reply to comment #6)
> (From update of attachment 181764 [details] [diff] [review] [edit])
> >+  // copy the index array, as index users might add/remove themselves when
> >+  // they are rebound and refreshed().
> >+  nsCOMArray<nsIXFormsControl> indexes = mIndexUsers;
> 
> Does the '=' operator really work here?

AFAIK a COMArray can be initialized with another, yes. I've changed it to 
nsCOMArray<nsIXFormsControl> indexes(mIndexUsers)
so it's more obvious that it's an initialization.

> >+  nsDependentSubstring fname = Substring(mScanner.Expression(),
> >+					     mScanner.Offset() + 1,
> >+					     mScanner.Length());
> 
> I assume the Substring is right this time ;)

Grrrrr! ;-) (so do I :) ).

Fixed the rest.

Comment 9

[Allan Beaufour]

Comment on attachment 181850 [details] [diff] [review]
Patch v2 w/smaug's comments fixed

Doron, 2nd. r? (I wonder if this flag works)

Comment 10

[Doron Rosenberg (IBM)]

Comment on attachment 181850 [details] [diff] [review]
Patch v2 w/smaug's comments fixed

r=me

Comment 11

[Allan Beaufour]

Checked in.