Closed
Bug 303312
Opened 19 years ago
Closed 19 years ago
itemsets not refreshed if bound nodeset changes
Categories
(Core Graveyard :: XForms, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aaronr, Assigned: smaug)
Details
(Keywords: fixed1.8)
Attachments
(3 files, 2 obsolete files)
(deleted),
application/xhtml+xml
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
aaronr
:
review+
allan
:
review+
|
Details | Diff | Splinter Review |
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.
testcase that shows the problem. The repeat above each select1 should show the
same items as the select1.
Assignee | ||
Updated•19 years ago
|
Assignee: aaronr → smaug
Assignee | ||
Comment 2•19 years ago
|
||
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•19 years ago
|
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.
Assignee | ||
Updated•19 years ago
|
Attachment #191969 -
Flags: review?(aaronr)
Assignee | ||
Comment 4•19 years ago
|
||
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+
Assignee | ||
Updated•19 years ago
|
Attachment #193077 -
Flags: review?(doronr)
Assignee | ||
Comment 5•19 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•19 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•19 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•19 years ago
|
||
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•19 years ago
|
||
Er, why not just this? ;)
Attachment #193077 -
Attachment is obsolete: true
Attachment #196509 -
Flags: review?(aaronr)
Assignee | ||
Updated•19 years ago
|
Attachment #196509 -
Flags: review?(allan)
Reporter | ||
Comment 10•19 years ago
|
||
Comment on attachment 196509 [details] [diff] [review]
v3
seems nice and tight.
Attachment #196509 -
Flags: review?(aaronr) → review+
Updated•19 years ago
|
Attachment #196509 -
Flags: review?(allan) → review+
Comment 11•19 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•19 years ago
|
||
Checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Whiteboard: xf-to-branch
Assignee | ||
Comment 13•19 years ago
|
||
This is not yet in branch. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 14•19 years ago
|
||
checked into branch 20051004
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•