Status

Core Graveyard
XForms
P3
normal
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: smaug, Assigned: surkov)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
Patch coming.
(Reporter)

Comment 1

12 years ago
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.
(Reporter)

Comment 2

12 years ago
Comment on attachment 194106 [details] [diff] [review]
v1

bah, doesn't work properly with nested repeats.
Attachment #194106 - Attachment is obsolete: true
(Reporter)

Comment 3

12 years ago
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?
(Reporter)

Comment 4

12 years ago
Comment on attachment 201284 [details] [diff] [review]
a better patch

And repeat must be optimized a bit :(
Attachment #201284 - Attachment is obsolete: true

Comment 5

12 years ago
*** Bug 317518 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

12 years ago
(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

12 years ago
(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.

Updated

12 years ago
Blocks: 325566

Comment 8

11 years ago
(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

11 years ago
(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...

Updated

11 years ago
Priority: -- → P3

Updated

11 years ago
Blocks: 331452
(Assignee)

Comment 10

11 years ago
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 :).
Attachment #218672 - Flags: review?(allan)
(Assignee)

Comment 11

11 years ago
Created attachment 218674 [details] [diff] [review]
xul testcase for orients

Comment 12

11 years ago
(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?
(Assignee)

Comment 13

11 years ago
(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)?
(Assignee)

Comment 14

11 years ago
(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.
(Reporter)

Comment 15

11 years ago
(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.

Updated

11 years ago
Blocks: 331209

Comment 16

11 years ago
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-
(Assignee)

Comment 17

11 years ago
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.
Attachment #218672 - Attachment is obsolete: true
Attachment #219245 - Flags: review?(allan)
(Assignee)

Comment 18

11 years ago
*** Bug 334789 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Blocks: 334916
(Assignee)

Updated

11 years ago
Assignee: Olli.Pettay → surkov
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Updated

11 years ago
Blocks: 330022
(Assignee)

Comment 19

11 years ago
Created attachment 219567 [details] [diff] [review]
patch2
Attachment #219245 - Attachment is obsolete: true
Attachment #219567 - Flags: review?(allan)
Attachment #219245 - Flags: review?(allan)

Comment 20

11 years ago
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-
(Reporter)

Comment 21

11 years ago
(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

(Assignee)

Comment 22

11 years ago
Created attachment 219585 [details] [diff] [review]
patch3

I removed the supporting of repeat aligning. I prefer to reopen the existing bug 317518.
Attachment #219585 - Flags: review?(allan)

Comment 23

11 years ago
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-
(Assignee)

Comment 24

11 years ago
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.
Attachment #219567 - Attachment is obsolete: true
Attachment #219585 - Attachment is obsolete: true
Attachment #219707 - Flags: review?(allan)
(Assignee)

Updated

11 years ago
Blocks: 317518

Comment 25

11 years ago
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+
(Reporter)

Comment 26

11 years ago
Comment on attachment 219707 [details] [diff] [review]
patch4

Looks good to me
Attachment #219707 - Flags: review?(Olli.Pettay) → review+

Comment 27

11 years ago
Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.