332127 - Expose switch-state to script

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[Allan Beaufour]

How about implementing this for switch elements:
[scriptable, uuid(...)]
interface nsIXFormsSwitchAccessors : nsIXFormsAccessors
{
  DOMElement getSelected();
};

?

Comment 1

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> How about implementing this for switch elements:
> [scriptable, uuid(...)]
> interface nsIXFormsSwitchAccessors : nsIXFormsAccessors
> {
>   DOMElement getSelected();
> };

Probalby setSelected(in DOMElement) method will be nice too. Are there ways to select a xf:case from a script now or to get selected xf:case?

I have unclear feel about scriptable interfaces names for xforms controls. Delegate controls have nsIXFormsXXXAccessors interface (like xf:range), other controls have nsIXFormsXXXUIElement interfaces (like xf:itemset). Thease interface names are so different. It looks unfine for me.

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> (In reply to comment #0)
> > How about implementing this for switch elements:
> > [scriptable, uuid(...)]
> > interface nsIXFormsSwitchAccessors : nsIXFormsAccessors
> > {
> >   DOMElement getSelected();
> > };
> 
> Probalby setSelected(in DOMElement) method will be nice too.

I'm always opposed to duplicate functionality which can be achieved through other means. You can use toggle for that... but I guess it is a bit quirky to have to go that way. So hmmmm, although I am a bit reluctant :)
{
  attribute DOMElement selectedCase;
};

> Are there ways to select a xf:case from a script now or to get selected
> xf:case?

Nope.

> I have unclear feel about scriptable interfaces names for xforms controls.
> Delegate controls have nsIXFormsXXXAccessors interface (like xf:range), other
> controls have nsIXFormsXXXUIElement interfaces (like xf:itemset). Thease
> interface names are so different. It looks unfine for me.

Hmmm, yeah, you are right. We need to draw the line better. The above is probably for the UI-part of things. In my view:
* UIs: has to do with the control and states of the control itself
* Accessors: has to do with the states and value of the bound node of the control

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> I'm always opposed to duplicate functionality which can be achieved through
> other means. You can use toggle for that... but I guess it is a bit quirky to
> have to go that way. So hmmmm, although I am a bit reluctant :)

It is not so big dublicating ;) because I guess access to some functionality by script and by some controls using are quite different things in some cases. Though probably I have inertness thinking and I try to move work standarts used in xul/xhmlt into xforms. If I understand right then one of famous cool thing of xforms is that xforms doesn't need in script. And threafore I'm not right from that point of view :).

> {
>   attribute DOMElement selectedCase;
> };

Fine. Btw it means we don't need in nsIXFormsSwitchElement any more.

> 
> > I have unclear feel about scriptable interfaces names for xforms controls.
> > Delegate controls have nsIXFormsXXXAccessors interface (like xf:range), other
> > controls have nsIXFormsXXXUIElement interfaces (like xf:itemset). Thease
> > interface names are so different. It looks unfine for me.
> 
> Hmmm, yeah, you are right. We need to draw the line better. The above is
> probably for the UI-part of things. In my view:
> * UIs: has to do with the control and states of the control itself
> * Accessors: has to do with the states and value of the bound node of the
> control
> 

As far as I know the described names convention is there now. :)

In general since 'Accessors' is used to work with controls (including states and value of instance data (of control too for sure :)) then I prefer to use 'Accessors' names for 'UI' interfaces too.

Comment 4

[Allan Beaufour]

Comments from the rest of you?

Comment 5

[aaronr]

(In reply to comment #4)
> Comments from the rest of you?
> 

Being able to get the selected case via script?  Not a bad idea since there is no quick way to get the information currently, but I don't see a use case.  If the form author needs to know what the currently selected case is, wouldn't they already be using instance data to track it so that they could make simple decisions using XPath and the instance value?  And if they are already doing this, then why would they need the script?  Wouldn't they just go look in the instance data to see?

Surkov and Allan have both made good points about the naming convention of our interfaces that we use.  In the end we need to have a clear message as to what interfaces we are going to support being used from user script and which we are not.  I'm ok with saying we'll support the nsIXFormsXxxxxAccessor interfaces and no others.  But once we say it, we need to be consistent with our naming.

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> Being able to get the selected case via script?  Not a bad idea since there is
> no quick way to get the information currently, but I don't see a use case.

I do not have a good "real form" use case for you. I just thought about it, because I need it for testing. To test the selected case I have, as you wrote, to set up a lot of stuff, which means that I end up testing a lot more than I should, ie. making it a bad test.

Maybe somebody else has a use case for it.

Comment 7

[aaronr]

(In reply to comment #6)
> (In reply to comment #5)
> > Being able to get the selected case via script?  Not a bad idea since there is
> > no quick way to get the information currently, but I don't see a use case.
> 
> I do not have a good "real form" use case for you. I just thought about it,
> because I need it for testing. To test the selected case I have, as you wrote,
> to set up a lot of stuff, which means that I end up testing a lot more than I
> should, ie. making it a bad test.
> 
> Maybe somebody else has a use case for it.
> 


No reason we can't expose an interface privately and move it to a supported accessor-type interface if we find people wanting it.  The only use cases I could come up were internal ones like you said.  I couldn't think of anything a user would want to do with it.  But I'm often short sighted :=)

Comment 8

[Allan Beaufour]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Being able to get the selected case via script?  Not a bad idea since there is
> > > no quick way to get the information currently, but I don't see a use case.
> > 
> > I do not have a good "real form" use case for you. I just thought about it,
> > because I need it for testing. To test the selected case I have, as you wrote,
> > to set up a lot of stuff, which means that I end up testing a lot more than I
> > should, ie. making it a bad test.
> > 
> > Maybe somebody else has a use case for it.
> 
> No reason we can't expose an interface privately and move it to a supported
> accessor-type interface if we find people wanting it.

Internally, as in "not scriptable"? I need it from script :)

> The only use cases I could come up were internal ones like you said. 
> I couldn't think of anything a user would want to do with it.  But I'm often 
> short sighted :=)

Well, me too, I have no use case either :)

Comment 9

[aaronr]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Being able to get the selected case via script?  Not a bad idea since there is
> > > > no quick way to get the information currently, but I don't see a use case.
> > > 
> > > I do not have a good "real form" use case for you. I just thought about it,
> > > because I need it for testing. To test the selected case I have, as you wrote,
> > > to set up a lot of stuff, which means that I end up testing a lot more than I
> > > should, ie. making it a bad test.
> > > 
> > > Maybe somebody else has a use case for it.
> > 
> > No reason we can't expose an interface privately and move it to a supported
> > accessor-type interface if we find people wanting it.
> 
> Internally, as in "not scriptable"? I need it from script :)
> 

Internally, as in 'use at your own risk', we don't document the interface publicly, we don't encourage script authors to use it, we put the function in an interface that has 'private' or 'internal' in the name... stuff like that.  Until someone needs it and has a compelling use case :=)

Comment 10

[Allan Beaufour]

I've created bug 332695 for an interface naming discussion.

Comment 11

[Allan Beaufour]

Attachment: Patch

This patch extends the nsIXFormsSwitchElement. If we go on with my suggestions in bug 332695, we should rename it to nsIXFormsSwitchDelegate instead though.

The interface is "private" in the sense that you have to do an explicit QI to it, to access its functions.

Comment 12

[aaronr]

Comment on attachment 217153 [details] [diff] [review]
Patch

>Index: nsXFormsSwitchElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSwitchElement.cpp,v
>retrieving revision 1.11
>diff -u -p -U8 -r1.11 nsXFormsSwitchElement.cpp
>--- nsXFormsSwitchElement.cpp	4 Apr 2006 08:12:38 -0000	1.11
>+++ nsXFormsSwitchElement.cpp	4 Apr 2006 15:07:16 -0000

>+// nsIXFormsSwitchElement
>+
>+NS_IMETHODIMP
>+nsXFormsSwitchElement::GetSelected(nsIDOMElement **aCase)
>+{
>+  NS_ENSURE_ARG(aCase);
>+  *aCase = nsnull;
>+  if (mSelected) {
>+    NS_ADDREF(*aCase = mSelected);
>+  }
>+  return NS_OK;
>+}
>+
>+

Should probably use NS_ENSURE_ARG_POINTER instead of NS_ENSURE_ARG here to be consistent with mozilla.  Slightly more descriptive return code.  Also, I think you can make this simpler.  How about:

nsXFormsSwitchElement::GetSelected(nsIDOMElement **aCase)
{
  NS_ENSURE_ARG_POINTER(aCase);
  NS_IF_ADDREF(*aCase = mSelected);
  return NS_OK;
}

I think that this expands out to do the same thing that you were doing.

with that, r=me

Comment 13

[Allan Beaufour]

(In reply to comment #12)
> nsXFormsSwitchElement::GetSelected(nsIDOMElement **aCase)
> {
>   NS_ENSURE_ARG_POINTER(aCase);
>   NS_IF_ADDREF(*aCase = mSelected);
>   return NS_OK;
> }
> 
> I think that this expands out to do the same thing that you were doing.

It does, I was just suddenly in doubt about whether it would null out *aCase, but of course it does that....

Comment 14

[Allan Beaufour]

Attachment: Patch w/Aaron's comment fixed

Comment 15

[Doron Rosenberg (IBM)]

Comment on attachment 217307 [details] [diff] [review]
Patch w/Aaron's comment fixed

Nit: you have "// classname" then newline, and then one place no newline.  Make up your mind!


> 
>+// nsXFormsSwitchElement
>+
> void
> nsXFormsSwitchElement::SetFocus(nsIDOMElement* aDeselected,
>                                nsIDOMElement* aSelected)
> {
>   if (aDeselected == aSelected)
>     return;
> 
>   nsCOMPtr<nsIDOMDocument> domDoc;
>@@ -375,16 +388,17 @@ nsXFormsSwitchElement::CaseChanged(nsIDO
>     PRBool selected;
>     caseElem->GetInitialSelectedState(&selected);
>     if (selected) {
>       SetSelected(element, PR_TRUE);
>     }
>   }
> }
> 
>+// nsIXFormsControl
> NS_IMETHODIMP
> nsXFormsSwitchElement::IsEventTarget(PRBool *aOK)
> {
>   *aOK = PR_FALSE;
>   return NS_OK;
> }
> 
> NS_HIDDEN_(nsresult)

Comment 16

[Allan Beaufour]

Attachment: Checked-in version

Comment 17

[Allan Beaufour]

Fixed on trunk