Last Comment Bug 306247 - XBLize xforms:repeat
: XBLize xforms:repeat
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
Mentors:
: 334789 (view as bug list)
Depends on:
Blocks: 317518 325566 330022 331209 331452 334916
  Show dependency treegraph
 
Reported: 2005-08-28 10:51 PDT by Olli Pettay [:smaug]
Modified: 2006-06-06 06:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (21.84 KB, patch)
2005-08-28 10:56 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
a better patch (17.07 KB, patch)
2005-10-29 13:07 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
rough patch (15.80 KB, patch)
2006-04-17 03:45 PDT, alexander :surkov
allan: review-
Details | Diff | Review
xul testcase for orients (1.49 KB, patch)
2006-04-17 03:47 PDT, alexander :surkov
no flags Details | Diff | Review
patch (16.80 KB, patch)
2006-04-20 19:52 PDT, alexander :surkov
no flags Details | Diff | Review
patch2 (18.43 KB, patch)
2006-04-23 20:57 PDT, alexander :surkov
allan: review-
Details | Diff | Review
patch3 (18.67 KB, patch)
2006-04-24 03:30 PDT, alexander :surkov
allan: review-
Details | Diff | Review
patch4 (18.80 KB, patch)
2006-04-24 19:34 PDT, alexander :surkov
allan: review+
bugs: review+
Details | Diff | Review

Description Olli Pettay [:smaug] 2005-08-28 10:51:05 PDT
Patch coming.
Comment 1 Olli Pettay [:smaug] 2005-08-28 10:56:34 PDT
Created attachment 194106 [details] [diff] [review]
v1

This XBLizes <repeat> in a bit similar way, which was done for
<itemset>. I haven't tested this properly yet, especially nested
repeats need testing.

Allan, could you take a look at this patch, does it look somewhat right to you?

I hope the patch applies properly.
Comment 2 Olli Pettay [:smaug] 2005-08-28 11:34:26 PDT
Comment on attachment 194106 [details] [diff] [review]
v1

bah, doesn't work properly with nested repeats.
Comment 3 Olli Pettay [:smaug] 2005-10-29 13:07:13 PDT
Created attachment 201284 [details] [diff] [review]
a better patch

This doesn't crash ;). And creates right kind of UI also when
nested repeats are used. But still problems with repeat-index :(
Allan, maybe you have some comments?
Comment 4 Olli Pettay [:smaug] 2005-10-30 13:29:27 PST
Comment on attachment 201284 [details] [diff] [review]
a better patch

And repeat must be optimized a bit :(
Comment 5 Allan Beaufour 2005-11-24 06:13:50 PST
*** Bug 317518 has been marked as a duplicate of this bug. ***
Comment 6 alexander :surkov 2005-11-24 19:16:17 PST
(In reply to comment #1 bug 317518)
> XBLizing repeat would solve that. See bug 306247
> 

Yes, I guess it should. But it's needed to remember about it. So the patch should let to specify desired layout.
Comment 7 Allan Beaufour 2005-11-28 02:11:43 PST
(In reply to comment #6)
> (In reply to comment #1 bug 317518)
> > XBLizing repeat would solve that. See bug 306247
> > 
> 
> Yes, I guess it should. But it's needed to remember about it. So the patch
> should let to specify desired layout.

We could implement an appearance="vertical"/minimal, whatever. But that's a detail, as long as we have it defined in XBL the form author can define his own custom control for it.
Comment 8 Allan Beaufour 2006-03-16 01:29:20 PST
(In reply to comment #3)
> Created an attachment (id=201284) [edit]
> a better patch
> 
> This doesn't crash ;). And creates right kind of UI also when
> nested repeats are used. But still problems with repeat-index :(
> Allan, maybe you have some comments?

I looked briefly on this. Except for the fact that you forgot to include the IDL file in the second patch :), then the problem is the nested repeat handling of indexes, right?
Comment 9 Allan Beaufour 2006-03-20 03:07:23 PST
(In reply to comment #8)
> (In reply to comment #3)
> > Created an attachment (id=201284) [edit]
> > a better patch
> > 
> > This doesn't crash ;). And creates right kind of UI also when
> > nested repeats are used. But still problems with repeat-index :(
> > Allan, maybe you have some comments?
> 
> I looked briefly on this. Except for the fact that you forgot to include the
> IDL file in the second patch :), then the problem is the nested repeat 
> handling of indexes, right?

That might actually be bug 331081...
Comment 10 alexander :surkov 2006-04-17 03:45:55 PDT
Created attachment 218672 [details] [diff] [review]
rough patch

It's rougth patch. I have a unclear feel that it's not very good (I'm about bind() method). I should get more expierence with repeat to do a more finest patch :).
Comment 11 alexander :surkov 2006-04-17 03:47:42 PDT
Created attachment 218674 [details] [diff] [review]
xul testcase for orients
Comment 12 Allan Beaufour 2006-04-17 08:51:18 PDT
(In reply to comment #10)
> Created an attachment (id=218672) [edit]
> rough patch
> 
> It's rougth patch. I have a unclear feel that it's not very good (I'm about
> bind() method). I should get more expierence with repeat to do a more finest
> patch :).

What's the difference between and the previous patches?
Comment 13 alexander :surkov 2006-04-17 19:17:03 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > Created an attachment (id=218672) [edit]
> > rough patch
> > 
> > It's rougth patch. I have a unclear feel that it's not very good (I'm about
> > bind() method). I should get more expierence with repeat to do a more finest
> > patch :).
> 
> What's the difference between and the previous patches?
> 

The difference is that there are no changes in xforms model part (in nsPostRefresh) and I changed nsRepeatElement::Bind() method. Now repeat element is deffered if model is not ready. But I doubt that it is proper way to change Bind() method. Therefore I called the patch as a rough.

I have one more question why is repeat element inherited from nsXFormsDelegateStub class (not just from nsXFormsBindableControlStub)?
Comment 14 alexander :surkov 2006-04-17 19:18:14 PDT
(In reply to comment #12)
> 
> What's the difference between and the previous patches?
> 

For sure I added into the patch repeat for xul context support.
Comment 15 Olli Pettay [:smaug] 2006-04-18 01:29:08 PDT
(In reply to comment #13)
> I have one more question why is repeat element inherited from
> nsXFormsDelegateStub class (not just from nsXFormsBindableControlStub)?
> 

That way this.delegate.widgetAttached() can be called in constructor.
Comment 16 Allan Beaufour 2006-04-20 04:40:06 PDT
Comment on attachment 218672 [details] [diff] [review]
rough patch

You need smaug's change to the nsPostRefresh() handling, or you will crash on nested repeats.

>+  /**
>+   * Returns either the anonymous content of the repeat or null;
>+   */
>+  already_AddRefed<nsIDOMElement> AnonymousContent();

I think it would be better if it was called GetAnonymousContent(). 1) It actually tells what the function does, and 2) it is a getter so it should be called that :)

>+already_AddRefed<nsIDOMElement>
>+nsXFormsRepeatElement::AnonymousContent()
>+{
>+  nsIDOMElement* anon = nsnull;
>+  nsCOMPtr<nsIXFormsRepeatUIElement> uiElement(do_QueryInterface(mElement));
>+  if (uiElement) {
>+    // AddRefs
>+    uiElement->GetAnonymousRepeatContent(&anon);

Please add "// addrefs" to this line.
Comment 17 alexander :surkov 2006-04-20 19:52:15 PDT
Created attachment 219245 [details] [diff] [review]
patch

I ran quickly thought tests from http://beaufour.dk/xftst/Repeat and I couldn't find problems that aren't presented without patch.
Comment 18 alexander :surkov 2006-04-20 22:27:08 PDT
*** Bug 334789 has been marked as a duplicate of this bug. ***
Comment 19 alexander :surkov 2006-04-23 20:57:16 PDT
Created attachment 219567 [details] [diff] [review]
patch2
Comment 20 Allan Beaufour 2006-04-24 01:29:59 PDT
Comment on attachment 219567 [details] [diff] [review]
patch2

> --- mozilla.orig/extensions/xforms/nsIXFormsRepeatUIElement.idl	1970-01-01 08:00:00.000000000 +0800

> @@ -0,0 +1,53 @@
> + * The Initial Developer of the Original Code is
> + * Olli Pettay.
> + * Portions created by the Initial Developer are Copyright (C) 2005

2006

> +  /**
> +   * anonymousRepeatContent is used as a parent element for
> +   * the generated \<contextcontainer\> elements.

Generated content will be inserted as children of this element.

> --- mozilla.orig/extensions/xforms/nsXFormsRepeatElement.cpp	2006-03-30 14:45:57.000000000 +0900

> @@ -821,13 +812,36 @@ nsXFormsRepeatElement::Refresh()
>      // refresh (either using delete or through script, so check the index
>      // value
>      SanitizeIndex(&mCurrentIndex);
> -  } else if (!mParent && mMaxIndex) {
> +  } else if (mMaxIndex) {
>      // repeat-index has not been initialized, set it.
> -    GetStartingIndex(&mCurrentIndex);
> +    if (!mParent) {
> +      GetStartingIndex(&mCurrentIndex);
> +    } else {

This is only needed for nested repeats, so:
> +    } else if (mLevel > 1) {
should avoid all this if it is not necessary.

You also need a comment explaining what you do here.

> +      nsCOMPtr<nsIXFormsRepeatItemElement> context = nsnull;
> +      nsCOMPtr<nsIDOMNode> parent = nsnull;

You do not need to initialize nsCOMPtrs.

> +      nsCOMPtr<nsIDOMNode> temp = do_QueryInterface(mElement);
> +      NS_ENSURE_STATE(temp);
> +
> +      while (!context) {
> +        temp.swap(parent);

I would rather have parent initialized to mElement, and then temp.swap(parent) at the end of the loop. Easier to read.

> +        rv = parent->GetParentNode(getter_AddRefs(temp));

This can set temp to null, but still return NS_OK. So you need to check that temp has a value, or you might end up deref'ing a null.

> +        NS_ENSURE_SUCCESS(rv, rv);
> +        context = do_QueryInterface(parent);

Shouldn't you be QI'ing temp, and not parent?

> +      }
> +
> +      PRBool hasIndex = PR_FALSE;
> +      context->GetIndexState(&hasIndex);
> +      if (hasIndex) {
> +        PRUint32 index = 0;
> +        GetStartingIndex(&index);
> +        SetIndex(&index, PR_FALSE);
> +      }
> +      return NS_OK;
> +    }
>    }

I'm wondering why this is suddenly necessary though. But repeat is a big mess, and I been meaning to refactor it for a long time. But this bug should not be delayed, so doing this better can wait.
 

> --- mozilla.orig/extensions/xforms/resources/content/xforms-xul.xml	2006-04-04 17:11:26.000000000 +0900

> +
> +  <!-- REPEAT -->
> +  <binding id="xformswidget-repeat"
> +           extends="chrome://xforms/content/xforms.xml#xformswidget-repeat-base">
> +    <content>
> +      <xul:box hidden="true">
> +        <children/>
> +      </xul:box>
> +      <xul:vbox flex="1" xbl:inherits="orient" anonid="insertion"/>

I do not like exposing non-standard attributes on XForms elements. Is there a CSS way of controling the orient? If there is, then that should be used, or we should have two appearance implementations.
Comment 21 Olli Pettay [:smaug] 2006-04-24 01:34:13 PDT
(In reply to comment #20)
> 
> I do not like exposing non-standard attributes on XForms elements. Is there a
> CSS way of controling the orient? If there is, then that should be used, or we
> should have two appearance implementations.
> 

-moz-box-orient

Comment 22 alexander :surkov 2006-04-24 03:30:13 PDT
Created attachment 219585 [details] [diff] [review]
patch3

I removed the supporting of repeat aligning. I prefer to reopen the existing bug 317518.
Comment 23 Allan Beaufour 2006-04-24 05:23:39 PDT
Comment on attachment 219585 [details] [diff] [review]
patch3

>+    } else if (mLevel > 1) {
>+      // Set repeat-index for inner repeats. If parent <contextcontainer/>
>+      // element is selected then mCurrentIndex is setted on starting index.
>+
>+      nsCOMPtr<nsIDOMNode> temp;
>+      nsCOMPtr<nsIDOMNode> parent = do_QueryInterface(mElement);
>+      NS_ENSURE_STATE(parent);
>+      nsCOMPtr<nsIXFormsRepeatItemElement> context;
>+
>+      while (!context) {
>+        temp->GetParentNode(getter_AddRefs(parent));
>+        context = do_QueryInterface(parent);
>+        temp.swap(parent);
>+      }

This is sloppy. What will temp always be in the first call?

Initialize temp instead of parent, do not ignore the return value, check the temp value in the loop condition, and assert that context is set on loop exit.
Comment 24 alexander :surkov 2006-04-24 19:34:52 PDT
Created attachment 219707 [details] [diff] [review]
patch4

> This is sloppy. What will temp always be in the first call?
> 
> Initialize temp instead of parent, do not ignore the return value, check the
> temp value in the loop condition, and assert that context is set on loop exit.
> 

I'm sorry for thoughtlessness.
Comment 25 Allan Beaufour 2006-04-26 01:52:40 PDT
Comment on attachment 219707 [details] [diff] [review]
patch4

>+      nsCOMPtr<nsIDOMNode> temp = do_QueryInterface(mElement);
>+      NS_ENSURE_STATE(temp);

nsIDOMElement is (C++) inherited from nsIDOMNode, so you can assign directly.

r=me with that fix. But let's run it through smaug first.
Comment 26 Olli Pettay [:smaug] 2006-04-26 02:18:38 PDT
Comment on attachment 219707 [details] [diff] [review]
patch4

Looks good to me
Comment 27 Allan Beaufour 2006-04-26 02:55:00 PDT
Fixed on trunk

Note You need to log in before you can comment on or make changes to this bug.