Status

defect
P3
normal
RESOLVED FIXED
14 years ago
3 years ago

People

(Reporter: smaug, Assigned: surkov)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Patch coming.
Posted patch v1 (obsolete) — Splinter Review
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 on attachment 194106 [details] [diff] [review]
v1

bah, doesn't work properly with nested repeats.
Attachment #194106 - Attachment is obsolete: true
Posted patch a better patch (obsolete) — Splinter Review
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 on attachment 201284 [details] [diff] [review]
a better patch

And repeat must be optimized a bit :(
Attachment #201284 - Attachment is obsolete: true
*** Bug 317518 has been marked as a duplicate of this bug. ***
(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.
(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.
Blocks: 325566
(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?
(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...
Priority: -- → P3
Blocks: 331452
Posted patch rough patch (obsolete) — Splinter Review
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 :).
Attachment #218672 - Flags: review?(allan)
(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?
(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)?
(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.
(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.
Blocks: 331209
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.
Attachment #218672 - Flags: review?(allan) → review-
Posted patch patch (obsolete) — Splinter Review
I ran quickly thought tests from http://beaufour.dk/xftst/Repeat and I couldn't find problems that aren't presented without patch.
Attachment #218672 - Attachment is obsolete: true
Attachment #219245 - Flags: review?(allan)
*** Bug 334789 has been marked as a duplicate of this bug. ***
Blocks: 334916
Assignee: Olli.Pettay → surkov
Status: NEW → ASSIGNED
Blocks: 330022
Posted patch patch2 (obsolete) — Splinter Review
Attachment #219245 - Attachment is obsolete: true
Attachment #219567 - Flags: review?(allan)
Attachment #219245 - Flags: review?(allan)
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.
Attachment #219567 - Flags: review?(allan) → review-
(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

Posted patch patch3 (obsolete) — Splinter Review
I removed the supporting of repeat aligning. I prefer to reopen the existing bug 317518.
Attachment #219585 - Flags: review?(allan)
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.
Attachment #219585 - Flags: review?(allan) → review-
Posted patch patch4Splinter Review
> 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.
Attachment #219567 - Attachment is obsolete: true
Attachment #219585 - Attachment is obsolete: true
Attachment #219707 - Flags: review?(allan)
Blocks: 317518
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.
Attachment #219707 - Flags: review?(allan)
Attachment #219707 - Flags: review?(Olli.Pettay)
Attachment #219707 - Flags: review+
Comment on attachment 219707 [details] [diff] [review]
patch4

Looks good to me
Attachment #219707 - Flags: review?(Olli.Pettay) → review+
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Keywords: fixed1.8.1
Keywords: fixed1.8.0.5
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.