Last Comment Bug 303312 - itemsets not refreshed if bound nodeset changes
: itemsets not refreshed if bound nodeset changes
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-03 16:37 PDT by aaronr
Modified: 2005-10-10 20:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (3.52 KB, application/xhtml+xml)
2005-08-03 16:39 PDT, aaronr
no flags Details
proposed patch (for <select1>) (14.48 KB, patch)
2005-08-08 07:59 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
v2 (14.41 KB, patch)
2005-08-18 11:13 PDT, Olli Pettay [:smaug]
aaronr: review+
Details | Diff | Review
Alternative solution (21.06 KB, patch)
2005-09-15 03:25 PDT, Peter Nunn
no flags Details | Diff | Review
v3 (7.30 KB, patch)
2005-09-18 02:44 PDT, Olli Pettay [:smaug]
aaronr: review+
allan: review+
Details | Diff | Review

Description aaronr 2005-08-03 16:37:47 PDT
Unlike repeats who do update their contents when the bound nodeset changes, it
doesn't look like itemsets refresh themselves when the bound nodeset changes. 
Or if they do, they don't reflect this change back to the select1 to show a new
set of options available to the user.
Comment 1 aaronr 2005-08-03 16:39:15 PDT
Created attachment 191541 [details]
testcase

testcase that shows the problem.  The repeat above each select1 should show the
same items as the select1.
Comment 2 Olli Pettay [:smaug] 2005-08-08 07:59:47 PDT
Created attachment 191969 [details] [diff] [review]
proposed patch (for <select1>)

This one works with <select1>.
<select> part should be fixed when XBLizing it.
Aaron, what do you think?

Doron, could you check that XBLized <select> works properly with <itemset>s?
Comment 3 aaronr 2005-08-09 01:21:44 PDT
Comment on attachment 191969 [details] [diff] [review]
proposed patch (for <select1>)


> NS_IMETHODIMP
>Index: nsXFormsItemSetElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsItemSetElement.cpp,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 nsXFormsItemSetElement.cpp
>--- nsXFormsItemSetElement.cpp	3 Aug 2005 18:43:44 -0000	1.7
>+++ nsXFormsItemSetElement.cpp	8 Aug 2005 14:49:45 -0000

> NS_IMETHODIMP
> nsXFormsItemSetElement::Bind()
> {
>+  mModel = nsXFormsUtils::GetModel(mElement);
>+  if (mModel)
>+    mModel->AddFormControl(this);
>+
>   return NS_OK;
> }

Any reason that you need to set up mModel and add the form control 
to the model here?  ProcessNodeBinding (which you call in Refresh()) 
does the same thing already, doesn't it?

Rest of the patch looks good.
Comment 4 Olli Pettay [:smaug] 2005-08-18 11:13:18 PDT
Created attachment 193077 [details] [diff] [review]
v2

You're right, there shouldn't be need for Bind();
Comment 5 Olli Pettay [:smaug] 2005-08-23 12:27:20 PDT
Comment on attachment 193077 [details] [diff] [review]
v2

Have to update this to support XBLized select too.
Comment 6 Peter Nunn 2005-09-14 18:37:06 PDT
(In reply to comment #5)
> (From update of attachment 193077 [details] [diff] [review] [edit])
> Have to update this to support XBLized select too.
> 

Came accross this same problem.  Have also looked at the whole model/itemset
issue in selects and select1 elements.  I felt that the itemset should be bound
to the model as a control, and in fact if you don't then nothing seems to work
the way it should.
I think that all elements that are bound need to be bound to the model. 
Consider when a select1 is bound to a model called 'mdl1' and the itemset is
bound to a model called 'mdl2'.  If mdl2 is rebuilt and a rebind required the
control which is bound to another model will not be rebound.
If I understand this thread correctly the proposal is to use the select1 to
refresh the itemset, which does not work.
Will post a patch with my changes which will work in all cases (he says wishfully:-)
Comment 7 Olli Pettay [:smaug] 2005-09-14 23:52:28 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 193077 [details] [diff] [review] [edit] [edit])
> > Have to update this to support XBLized select too.
> > 
> 
> Came accross this same problem.  Have also looked at the whole model/itemset
> issue in selects and select1 elements.  I felt that the itemset should be bound
> to the model as a control, and in fact if you don't then nothing seems to work
> the way it should.

Ofc. This bug is lefttover from the XTF <select> which did things in a quite
different way.

> If I understand this thread correctly the proposal is to use the select1 to
> refresh the itemset, which does not work.

No, that is not the proposal. Idea is to make itemset a normal control -
that is the reason to inherit nsXFormsDelegateStub.

> Will post a patch with my changes which will work in all cases (he says
wishfully:-)

Feel free to do that - though this is still assigned to me and 
I know that I should fix this asap. If you don't post a patch, I will do it soon  .

Comment 8 Peter Nunn 2005-09-15 03:25:38 PDT
Created attachment 196141 [details] [diff] [review]
Alternative solution

Here is the patch I worked out.  Had to move some methods on interfaces to get
an interface for the model to call to refresh the itemset.  Orginal interfaces
seemed to assume that the bound control will always have a visual element.
I took the approach that the interfaces should have a slightly different
heirarchy, where the visual context (nsIXFormsControl) interface should inherit
from the binding context (nsIXFormsContextControl) interface which deals with
binding only.  This allows the model to deal with binding and refreshing
without the target needing to be a visual control, so I moved the methods that
were needed to bind to the base interface (nsIXFormsContextControl) and tried
to leave the visual methods on the nsIXFormsControl interface.
Probably still needs work as I have to confess I still cannot say I really
understand some of the design decisions that went into the interfaces.
We have not found anything broken in our testing, so hopefully no other issues
have been introduced.
Comment 9 Olli Pettay [:smaug] 2005-09-18 02:44:23 PDT
Created attachment 196509 [details] [diff] [review]
v3

Er, why not just this?	;)
Comment 10 aaronr 2005-09-19 18:41:49 PDT
Comment on attachment 196509 [details] [diff] [review]
v3

seems nice and tight.
Comment 11 Allan Beaufour 2005-09-21 07:59:59 PDT
Comment on attachment 196141 [details] [diff] [review]
Alternative solution

I'm clearing the r flag, as we are going for attachment 196509 [details] [diff] [review]. If you
disagree, please re-set it.
Comment 12 Olli Pettay [:smaug] 2005-09-21 08:16:40 PDT
Checked in
Comment 13 Olli Pettay [:smaug] 2005-09-28 14:00:13 PDT
This is not yet in branch. Reopening.
Comment 14 aaronr 2005-10-06 17:19:34 PDT
checked into branch 20051004

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