itemsets not refreshed if bound nodeset changes

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
10 months ago

People

(Reporter: aaronr, Assigned: smaug)

Tracking

({fixed1.8})

Trunk
x86
All
fixed1.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

12 years ago
Created attachment 191541 [details]
testcase

testcase that shows the problem.  The repeat above each select1 should show the
same items as the select1.
(Assignee)

Updated

12 years ago
Assignee: aaronr → smaug
(Assignee)

Comment 2

12 years ago
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?
Attachment #191969 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

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

Updated

12 years ago
Attachment #191969 - Flags: review?(aaronr)
(Assignee)

Comment 4

12 years ago
Created attachment 193077 [details] [diff] [review]
v2

You're right, there shouldn't be need for Bind();
Attachment #191969 - Attachment is obsolete: true
Attachment #193077 - Flags: review?(aaronr)
(Reporter)

Updated

12 years ago
Attachment #193077 - Flags: review?(aaronr) → review+
(Assignee)

Updated

12 years ago
Attachment #193077 - Flags: review?(doronr)
(Assignee)

Comment 5

12 years ago
Comment on attachment 193077 [details] [diff] [review]
v2

Have to update this to support XBLized select too.
Attachment #193077 - Flags: review?(doronr)

Comment 6

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

Comment 7

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

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

Comment 9

12 years ago
Created attachment 196509 [details] [diff] [review]
v3

Er, why not just this?	;)
Attachment #193077 - Attachment is obsolete: true
Attachment #196509 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #196509 - Flags: review?(allan)
(Reporter)

Comment 10

12 years ago
Comment on attachment 196509 [details] [diff] [review]
v3

seems nice and tight.
Attachment #196509 - Flags: review?(aaronr) → review+

Updated

12 years ago
Attachment #196509 - Flags: review?(allan) → review+

Comment 11

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

Comment 12

12 years ago
Checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Whiteboard: xf-to-branch
(Assignee)

Comment 13

12 years ago
This is not yet in branch. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 14

12 years ago
checked into branch 20051004
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Updated

12 years ago
Keywords: fixed1.8
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.