Closed Bug 302496 Opened 19 years ago Closed 19 years ago

Add instance attribute to submission element

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: allan, Assigned: jhpedemonte)

References

()

Details

(Keywords: fixed1.8)

Attachments

(2 files, 5 obsolete files)

The current errata (E69a) adds an instance attribute to the submission element,
specifying which instance should be replaced on replace="instance"
Attached file data for testcase (obsolete) —
Attached file Testcase (obsolete) —
(In reply to comment #2)
> Created an attachment (id=190851) [edit]
> Testcase

Hmmm, I get: ERR: XForms Error (13): Could not parse new instance data ... is
the mime type wrong for the data?
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=190851) [edit] [edit]
> > Testcase
> 
> Hmmm, I get: ERR: XForms Error (13): Could not parse new instance data ... is
> the mime type wrong for the data?

I only took a second to look at this, but it looks like what is coming back from
our submission is an HTML page, not the page that you are expecting.  Since it
is HTML, I assume that is why you get the error output to the JS console.  I
haven't personally tried to do what you are doing....submitting to a XML file 
instead of a cgi-bin perl script or servlet.  If you can't host one, maybe worth
asking Leigh if he has one you can bounce off on xformstest.org.  I'd point you
to one of mine, but they are all behind a firewall :(
Attached file Testcase v2
This uses an external document instead.
Attachment #190850 - Attachment is obsolete: true
Attachment #190851 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
I wasn't too sure about the best way to handle the logic in
|GetSelectedInstanceData()|.  I needed a binding attr that would always be
ignored, such that it would always use the default expression; so I made it
pass an empty string as the binding attr.
Attachment #191272 - Flags: review?(aaronr)
Assignee: allan → jhpedemonte
Your GetSelectedInstanceData changes seems to be kinda hacky.  Did you give any
thought to just doing the xpath evaluations yourself?  Using EvaluateNodeBinding
was already overkill since submissions shouldn't need all the find context and
figuring out the model type of logic, for example.
Attached patch patch v2 (obsolete) — Splinter Review
Previous patch was completely wrong.  By going through |EvaluateNodeBinding()|,
this allowed a "bind" attr to override the "instance" attr.  So I did what
Aaron suggested, and call |EvaluateXPath()| myself.

I changed |GetSelectedInstanceData()| to |GetBoundInstanceData()|, since that
seems to better describe what the function does;  I then took the
|GetSelectedInstanceData()| name for my function.
Attachment #191272 - Attachment is obsolete: true
Attachment #191599 - Flags: review?(aaronr)
(In reply to comment #8)
> Created an attachment (id=191599) [edit]
> patch v2
> 
> Previous patch was completely wrong.  By going through |EvaluateNodeBinding()|,
> this allowed a "bind" attr to override the "instance" attr.  So I did what
> Aaron suggested, and call |EvaluateXPath()| myself.
> 
> I changed |GetSelectedInstanceData()| to |GetBoundInstanceData()|, since that
> seems to better describe what the function does;  I then took the
> |GetSelectedInstanceData()| name for my function.

I am soooo sorry.  I just now realized that @instance on submission replaces the
WHOLE instance document.  I didn't catch that the first time I read those
sections in the errata.  I thought that it specified which instance document to
use with the @ref (though looking back on that, it is such a stupid mistake on
my part because you can already do that).  I think that
nsXFormsUtilityService::GetInstanceDocumentRoot will give you everything that
you need without you having to dip into XPath.  See, that is what you get when
you ask me questions after I just get up (at noon :=)  Sorry again.
Attachment #191272 - Flags: review?(aaronr)
Attachment #191599 - Flags: review?(aaronr)
Attached patch patch v3 (obsolete) — Splinter Review
Patch that uses |nsXFormsUtilityService::GetInstanceDocumentRoot()|.
Attachment #191599 - Attachment is obsolete: true
Attachment #191725 - Flags: review?(aaronr)
Comment on attachment 191725 [details] [diff] [review]
patch v3

please change the error message.  Doesn't seem like if you saw the message that
you'd immediately know what the problem was with your form.  Doesn't have to be
exactly this, but something along the lines of: "Submission failed trying to
replace instance document '%S'.  Instance document doesn't exist in same model
as the submission element.

otherwise it looks good.  With new error message, r=me
Attachment #191725 - Flags: review?(aaronr) → review+
Talked to Aaron about this, and I was doing too much work in the latest patch. 
This one is better.
Attachment #191725 - Attachment is obsolete: true
Attachment #192401 - Flags: review?(aaronr)
Attachment #192401 - Flags: review?(smaug)
Comment on attachment 192401 [details] [diff] [review]
patch v4 (diff -w)

>Index: nsXFormsSubmissionElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp,v
>retrieving revision 1.37
>diff -u -6 -p -w -r1.37 nsXFormsSubmissionElement.cpp
>--- nsXFormsSubmissionElement.cpp	3 Aug 2005 14:30:54 -0000	1.37
>+++ nsXFormsSubmissionElement.cpp	11 Aug 2005 17:43:01 -0000
>@@ -486,39 +486,67 @@ nsXFormsSubmissionElement::LoadReplaceIn
>       nsXFormsUtils::ReportError(NS_LITERAL_STRING("instanceParseError"),
>                                  mElement);
>       return NS_ERROR_UNEXPECTED;
>     }
>   }
> 
>-  // get the nodeset that we are currently bound to
>-  nsCOMPtr<nsIDOMNode> data;
>+  // Get the appropriate instance node.  If the "instance" attribute is set,
>+  // then get that instance node.  Otherwise, get the one we are bound to.
>+  nsresult rv;
>   nsCOMPtr<nsIModelElementPrivate> model = GetModel();
>   NS_ENSURE_STATE(model);
>-  nsresult rv = GetSelectedInstanceData(getter_AddRefs(data));
>+  nsCOMPtr<nsIInstanceElementPrivate> instanceElement;
>+  nsAutoString value;
>+  mElement->GetAttribute(NS_LITERAL_STRING("instance"), value);
>+  if (!value.IsEmpty()){

nit: space before '{'

>+    rv = GetSelectedInstanceElement(value, model,
>+                                    getter_AddRefs(instanceElement));
>+  } else {
>+    nsCOMPtr<nsIDOMNode> data;
>+    rv = GetBoundInstanceData(getter_AddRefs(data));
>   if (NS_SUCCEEDED(rv)) {
>     nsCOMPtr<nsIDOMNode> instanceNode;
>     rv = nsXFormsUtils::GetInstanceNodeForData(data, model, 
>                                                getter_AddRefs(instanceNode));
>     NS_ENSURE_SUCCESS(rv, rv);
> 

nit: please fix indentation inside the else section

>+nsresult
>+nsXFormsSubmissionElement::GetSelectedInstanceElement(
>+                                            const nsString &aInstanceID,
>+                                            nsIModelElementPrivate *aModel,
>+                                            nsIInstanceElementPrivate **aResult)
>+{

nit: aInstanceID parameter should be nsAString like other functions.

>Index: nsXFormsSubmissionElement.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.h,v
>retrieving revision 1.17
>diff -u -6 -p -w -r1.17 nsXFormsSubmissionElement.h
>--- nsXFormsSubmissionElement.h	30 Jun 2005 22:32:44 -0000	1.17
>+++ nsXFormsSubmissionElement.h	11 Aug 2005 17:43:01 -0000
>@@ -95,13 +95,14 @@ public:
> 
>   NS_HIDDEN_(nsresult) LoadReplaceInstance(nsIChannel *);
>   NS_HIDDEN_(nsresult) LoadReplaceAll(nsIChannel *);
>   NS_HIDDEN_(nsresult) Submit();
>   NS_HIDDEN_(PRBool)   GetBooleanAttr(const nsAString &attrName, PRBool defaultVal = PR_FALSE);
>   NS_HIDDEN_(void)     GetDefaultInstanceData(nsIDOMNode **result);
>-  NS_HIDDEN_(nsresult) GetSelectedInstanceData(nsIDOMNode **result);
>+  NS_HIDDEN_(nsresult) GetBoundInstanceData(nsIDOMNode **result);
>+  NS_HIDDEN_(nsresult) GetSelectedInstanceElement(const nsString &aInstance, nsIModelElementPrivate *aModel, nsIInstanceElementPrivate **result);

nit: aInstance parameter should be nsAString like other functions.

With those, r=me
Attachment #192401 - Flags: review?(aaronr) → review+
Attachment #192401 - Flags: review?(smaug) → review+
checked in to trunk.
Whiteboard: xf-to-branch
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Removing xf-to-branch
Whiteboard: xf-to-branch
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: