278207 - select only writes one of multiple selected values to instance data

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Allan Beaufour]

If I select multiple items in a select, it should write the values to the bound
instance node space seperated. It does not, it only writes one of them.

Comment 1

[Allan Beaufour]

Attachment: testcase

Try selecting both "Chocolate" and "Vanilla" and hit Refresh.

Comment 2

[aaronr]

Attachment: proposed fix

Here is a proposed fix.  Basically if I detect that there is already a value in
the instance data node, I append each item's value to that value (having zero'd
out the node before starting).	I also moved all of the event dispatching
(recalc, etc.) out to nsXFormsSelectElement::Refresh so that we only do it
once.

Comment 3

[Allan Beaufour]

Comment on attachment 172860 [details] [diff] [review]
proposed fix

1) The patch is out of date with the current tree

2) Your approach lets each item element check the instance data node, append a
new value, and change the instance node. How about parsing a nsAString to
writeSelectedItems() and let nsXFormsSelectElement write the instance data
once?

Comment 4

[aaronr]

Attachment: second attempt

incorporates Allan's comments

Comment 5

[aaronr]

Attachment: updated patch

updated patch.	Chaged so that we call model.SetNodeValue on mBoundNode instead
of the text node otherwise we'll have errors in model->ValidateNode.

Comment 6

[aaronr]

Comment on attachment 172926 [details] [diff] [review]
updated patch

forgot to run dos2unix against it

Comment 7

[aaronr]

Attachment: updated patch with windows line endings removed

removed windows line endings

Comment 8

[Allan Beaufour]

Comment on attachment 172932 [details] [diff] [review]
updated patch with windows line endings removed

>+++ nsIXFormsSelectChild.idl	31 Jan 2005 06:52:14 -0000
...
>-  void writeSelectedItems(in nsIDOMNode container);
>+  void writeSelectedItems(in nsIDOMNode container, out AString stringBuffer);

1) You do not use container anywhere do you?
2) stringBuffer is "inout".

>-nsXFormsChoicesElement::WriteSelectedItems(nsIDOMNode *aContainer)
>+nsXFormsChoicesElement::WriteSelectedItems(nsIDOMNode *aContainer, 
>+                                           nsAString  &stringBuffer)

aStringBuffer

>+++ nsXFormsItemElement.cpp	31 Jan 2005 06:52:17 -0000
...
>-nsXFormsItemElement::WriteSelectedItems(nsIDOMNode *aContainer)
>+nsXFormsItemElement::WriteSelectedItems(nsIDOMNode *aContainer, 
>+                                        nsAString  &stringBuffer)

aStringBuffer

>+++ nsXFormsItemSetElement.cpp	31 Jan 2005 06:52:18 -0000
...
>-nsXFormsItemSetElement::WriteSelectedItems(nsIDOMNode *aContainer)
>+nsXFormsItemSetElement::WriteSelectedItems(nsIDOMNode *aContainer, 
>+                                           nsAString  &stringBuffer)

aStringBuffer

>--- nsXFormsSelectElement.cpp	29 Jan 2005 11:37:33 -0000	1.6
...
>+#include "nsIDOMText.h"

Where do you use this?

>+    nsXFormsUtils::SetNodeValue(mBoundNode, NS_LITERAL_STRING(""));

No need for this if container parameter is not used.

>+
>+    nsCOMPtr<nsIModelElementPrivate> model = 
>+                                        nsXFormsUtils::GetModel(mElement, 0);

model is saved in nsXFormsControlStub now, as mModel. So just add '|| !mModel'
to the previous mBoundNode check, and use mModel.

Besides from the above minor issues, it "works-for-me", so with the above fixed
r=me.

What does not work though, is selecting no elements. If you use the testcase,
and deselect everything the input field is not cleared, but that's not an issue
with this bug, so bug 280504 created.

Comment 9

[aaronr]

Attachment: update with Allan's last comments

>>+++ nsIXFormsSelectChild.idl		 31 Jan 2005 06:52:14 -0000
>...
>>-  void writeSelectedItems(in nsIDOMNode container);
>>+  void writeSelectedItems(in nsIDOMNode container, out AString
stringBuffer);
>
>1) You do not use container anywhere do you?
>2) stringBuffer is "inout".

1) I don't use container.  Yet.  I don't really know exactly what this will
look like once have have itemset selection working.  I think a whole nodeset
will have to be copied into the container.  I'll leave it for now and remove it
when the itemset bug is fixed if we don't need it (bug 278277).
2) AString can't be used as an inout.  Gets an error message as inout

Fixed other suggestions.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 172944 [details] [diff] [review]
update with Allan's last comments

Heh, oops.  I'd intended that a selected item would append itself to the text
node child of the bound node, not overwrite it.  However, your approach is
better because it does less work for each selected item and also fixes the case
where we're bound to an attribute node instead of an element node.  Comments
below:

>--- nsIXFormsSelectChild.idl	15 Jan 2005 01:04:02 -0000	1.2
>+++ nsIXFormsSelectChild.idl	31 Jan 2005 11:32:02 -0000
>-  void writeSelectedItems(in nsIDOMNode container);
>+  void writeSelectedItems(in nsIDOMNode container, out AString stringBuffer);

Just a little IDL nitpicking... I'd prefer:

  AString writeSelectedItems(in nsIDOMNode container);


>--- nsXFormsSelectElement.cpp	29 Jan 2005 11:37:33 -0000	1.6
>+++ nsXFormsSelectElement.cpp	31 Jan 2005 11:32:09 -0000
>+    // clear out whatever was in the instance data before.  
>+    // We'll fill it below.
>+    nsXFormsUtils::SetNodeValue(mBoundNode, NS_LITERAL_STRING(""));

Use EmptyString() instead of NS_LITERAL_STRING(""), it's a bit cheaper.

>+    nsString stringBuffer;

Use an nsAutoString here, that will avoid doing any heap allocation if the data
is less than 64 bytes.

>+      rv = nsXFormsUtils::DispatchEvent(modelNode, eEvent_Recalculate);
>+      NS_ENSURE_SUCCESS(rv, rv);

One consequence of bailing on an error return here is that (I believe) if a
script handler for the "recalculate" event causes a JS exception, you will get
an error return here and not do the revalidate and refresh.  Is that what we
want?

Comment 11

[Brian Ryner (not reading)]

(In reply to comment #10)
> >--- nsIXFormsSelectChild.idl	15 Jan 2005 01:04:02 -0000	1.2
> >+++ nsIXFormsSelectChild.idl	31 Jan 2005 11:32:02 -0000
> >-  void writeSelectedItems(in nsIDOMNode container);
> >+  void writeSelectedItems(in nsIDOMNode container, out AString stringBuffer);
> 
> Just a little IDL nitpicking... I'd prefer:
> 
>   AString writeSelectedItems(in nsIDOMNode container);
> 

Actually, disregard that.  What you have is fine, given that |inout| doesn't work.

Comment 12

[aaronr]

Comment on attachment 172944 [details] [diff] [review]
update with Allan's last comments

new patch coming

Comment 13

[aaronr]

Attachment: fix with suggestions

took Brian's suggestions except:

He second guessed the .idl change since it really is no difference.
We are still going to bail if the rv is no good.  Brian doesn't think that the
default handler will run if a script handler returns has an exception, so if
the default handler doesn't run, no sense doing the revalidate and refresh.

This patch also contains the fix I mentioned for 280504

Comment 14

[aaronr]

*** Bug 280504 has been marked as a duplicate of this bug. ***

Comment 15

[Brian Ryner (not reading)]

checked in