Closed Bug 303312 Opened 19 years ago Closed 19 years ago

itemsets not refreshed if bound nodeset changes

Categories

(Core Graveyard :: XForms, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: smaug)

Details

(Keywords: fixed1.8)

Attachments

(3 files, 2 obsolete files)

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.
Attached file testcase (deleted) —
testcase that shows the problem. The repeat above each select1 should show the same items as the select1.
Assignee: aaronr → smaug
Attached patch proposed patch (for <select1>) (obsolete) (deleted) — Splinter Review
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)
Status: NEW → ASSIGNED
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.
Attachment #191969 - Flags: review?(aaronr)
Attached patch v2 (obsolete) (deleted) — Splinter Review
You're right, there shouldn't be need for Bind();
Attachment #191969 - Attachment is obsolete: true
Attachment #193077 - Flags: review?(aaronr)
Attachment #193077 - Flags: review?(aaronr) → review+
Attachment #193077 - Flags: review?(doronr)
Comment on attachment 193077 [details] [diff] [review] v2 Have to update this to support XBLized select too.
Attachment #193077 - Flags: review?(doronr)
(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:-)
(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 .
Attached patch Alternative solution (deleted) — Splinter Review
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)
Attached patch v3 (deleted) — Splinter Review
Er, why not just this? ;)
Attachment #193077 - Attachment is obsolete: true
Attachment #196509 - Flags: review?(aaronr)
Attachment #196509 - Flags: review?(allan)
Comment on attachment 196509 [details] [diff] [review] v3 seems nice and tight.
Attachment #196509 - Flags: review?(aaronr) → review+
Attachment #196509 - Flags: review?(allan) → review+
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)
Checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
This is not yet in branch. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
checked into branch 20051004
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Keywords: fixed1.8
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: