372736 - Support Date and Time types for Range

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Merle Sterling]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; formsPlayer 1.4; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070216 Minefield/3.0a3pre

Separate bug for implementing date and time types for range. The date and time widgets depend on the dateTime widget from bug 370551.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[aaronr]

attach testcase, please

Comment 2

[Merle Sterling]

Attachment: testcase: date

Comment 3

[Merle Sterling]

Attachment: testcase: time

Comment 4

[Merle Sterling]

Attachment: testcase:date (XUL)

Comment 5

[Merle Sterling]

Attachment: testcase:time (XUL)

Comment 6

[Merle Sterling]

Attachment: patch

Comment 7

[alexander :surkov (:asurkov)]

I swear I reviewed this code! Merle, can you see any way to combine similar code because I have strongest deja vu :)

Comment 8

[Merle Sterling]

It should look familiar. :) The dateTime patch laid the foundation for the separate date and time. date and time share as much code as possible with dateTime.

Comment 9

[Merle Sterling]

Comment on attachment 257451 [details] [diff] [review]
patch

Canceling review for now. Going to rearrange the inheritance hiearchy slightly so we can save some code duplication for xsd:time.

Comment 10

[Merle Sterling]

Attachment: patch

date and time now inherit from dateTime so we save a little code duplication for the methods that calculate the min and max attributes for the components of a date and time.

Comment 11

[alexander :surkov (:asurkov)]

Merle, it looks you can have unique range binding ("xformswidgets-range-XXX") if you will make isInRange method more common. It looks all you need is this.control should have method(s) to return start/end/current values in unique way. Probably can you reorganize native widget to use control.start/control.end/... for this?

Comment 12

[Merle Sterling]

(In reply to comment #11)
> Merle, it looks you can have unique range binding ("xformswidgets-range-XXX")
> if you will make isInRange method more common. It looks all you need is
> this.control should have method(s) to return start/end/current values in unique
> way. Probably can you reorganize native widget to use
> control.start/control.end/... for this?

Yes, the range bindings are starting to look all the same. I suppose we could have one range binding named xformswidget-range-nonnumeric and have isInRange delegate to the control;ie isInRange of the binding will call this.control.isInRange().

Comment 13

[Merle Sterling]

Attachment: patch

Now using a single xformswidget-range-nonnumeric binding that can be used for all of these non-numeric range types.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 257714 [details] [diff] [review]
patch

I'm not sure about 'nonnumberic' name, probably 'date' or something else will be better.

please remove 'xf-value'
from 'range-date' binding

Comment 15

[Merle Sterling]

(In reply to comment #14)
> (From update of attachment 257714 [details] [diff] [review])
> I'm not sure about 'nonnumberic' name, probably 'date' or something else will
> be better.
I figured 'nonnumeric' was generic enough but am open to suggestions. Remember though that this one binding will be used for every other type of range (except numeric of course).

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 257714 [details] [diff] [review] [details])
> > I'm not sure about 'nonnumberic' name, probably 'date' or something else will
> > be better.
> I figured 'nonnumeric' was generic enough but am open to suggestions. Remember
> though that this one binding will be used for every other type of range (except
> numeric of course).
> 

Yes, but nonumeric is too common. The name should be related with date since we use it date types.

Comment 17

[Merle Sterling]

Alex, if you name the range binding -date then you need one named -time, -dateTime, -gDay, -gYear, -dayTimeDuration, etc and they are all identical except for the name. That is exactly what you were talking about in comment 11 because I originally had separate (identical) bindings for date, time, and dateTime.

I've already completed every other range (bugs 372737 and 372739) and they can all use the single binding by delegating isInRange to the widget.

Comment 18

[aaronr]

Comment on attachment 257714 [details] [diff] [review]
patch


>Index: resources/content/widgets-xul.xml
>===================================================================

>+  <!-- DATE RANGE-->
>+  <binding id="range-date"
>+           extends="chrome://xforms/content/widgets.xml#date">
>+
>+    <resources>
>+      <stylesheet src="chrome://xforms/skin/widgets-xul.css"/>
>+    </resources>
>+
>+    <content>
>+      <xul:label value="" anonid="minLabel"/>
>+      <xul:box class="xf-value range-box">
>+        <xul:label control="yearSpin" value="&xforms.date.year.label;"/>
>+        <xul:textbox type="number" size="3" anonid="yearSpin"
>+                     xbl:inherits="tabindex=mozType:tabindex"/>
>+      </xul:box>
>+      <xul:box class="range-box">
>+        <xul:label control="monthSpin" value="&xforms.date.month.label;"/>
>+        <xul:textbox type="number" size="1" anonid="monthSpin"
>+                     xbl:inherits="tabindex=mozType:tabindex"/>
>+      </xul:box>
>+      <xul:box class="range-box">
>+        <xul:label control="daySpin" value="&xforms.date.day.label;"/>
>+        <xul:textbox type="number" size="1" anonid="daySpin"
>+                     xbl:inherits="tabindex=mozType:tabindex"/>
>+      </xul:box>
>+      <xul:label value="" anonid="maxLabel"/>
>+    </content>

As Alex mentioned, you shouldn't have class="xf-range" here, should be on the xul:box in the anonymous content of the range element.  Please fix for range-datetime, too.
 
>+
>+    <implementation>
>+      <method name="updateLabels">
>+        <body>
>+        <![CDATA[
>+          this.minLabel.value = this.start + " >> ";
>+          this.maxLabel.value = " << " + this.end;
>+        ]]>
>+        </body>
>+      </method>

Hmmm, I'm wondering if maybe we shouldn't separate out the min and max separators so that someone can quick go in and change the >> and << in our chrome's DTD if they want something different rather than changing our JS code in multiple places.

updateLabels looks the same for both #date and #time.  Can it be moved to the #datetime binding so that it is in one central place?

>+  <!-- TIME RANGE-->
>+  <binding id="range-time"
>+           extends="chrome://xforms/content/widgets.xml#time">
>+

>+      <method name="isInRange">
>+        <parameter name="aValue"/>
>+        <body>
>+        <![CDATA[
>+          var startTime = this.getDate(this.start);
>+          var endTime = this.getDate(this.end);
>+          var currentTime = this.getDate(aValue);
>+          return startTime <= currentTime && currentTime <= endTime;
>+        ]]>
>+        </body>
>+      </method>

nit: Please put a comment here saying how you are going to get a time out of something called getDate.

>Index: resources/content/widgets.xml
>===================================================================

>+      <!-- Extract the seconds from a time -->
>+      <method name="getSeconds">
>+        <parameter name="aTime"/>
>+        <body>
>+          return parseFloat(aTime.substr(6,2));
>+        </body>
>+      </method>

Looks like you don't handle partial seconds (anything after the decimal point) for time or dateTime.  I guess we should.  Especially if the min is 12:32:31.555.  Now we'd allow the user to set 12:32:31 which is actually below the min.  And it wouldn't show as out of range (two bugs right there :-)  Please open a bug on supporting partial seconds if there isn't one open already.

>Index: resources/content/xforms.css
>===================================================================

>-  /* range type="xsd:dateTime" */
>+  /* range type="xsd:dateTime", "xsd:date", "xsd:time" */
> xul|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"],
>-html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"] {
>-  -moz-binding: url('chrome://xforms/content/range-xul.xml#xformswidget-range-datetime');
>+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"],
>+xul|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#date"],
>+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#date"],
>+xul|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#time"],
>+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#time"] {
>+  -moz-binding: url('chrome://xforms/content/range-xul.xml#xformswidget-range-nonnumeric');
> }

nit: I'm not a huge fan of 'nonnumeric' as a name, either.  Maybe xformswidget-range-datesandtimes?

Comment 19

[Merle Sterling]

(In reply to comment #18)
> >+
> >+    <implementation>
> >+      <method name="updateLabels">
> >+        <body>
> >+        <![CDATA[
> >+          this.minLabel.value = this.start + " >> ";
> >+          this.maxLabel.value = " << " + this.end;
> >+        ]]>
> >+        </body>
> >+      </method>
> Hmmm, I'm wondering if maybe we shouldn't separate out the min and max
> separators so that someone can quick go in and change the >> and << in our
> chrome's DTD if they want something different rather than changing our JS code in multiple places.
Sure, but what you propose? Adding something like range.minLabel.separator to xforms.dtd and having the JS code do this.start + &range.minLabel.separator?
> >Index: resources/content/widgets.xml
> >===================================================================
> >+      <!-- Extract the seconds from a time -->
> >+      <method name="getSeconds">
> >+        <parameter name="aTime"/>
> >+        <body>
> >+          return parseFloat(aTime.substr(6,2));
> >+        </body>
> >+      </method>
> Looks like you don't handle partial seconds (anything after the decimal point)
> for time or dateTime.  I guess we should.  Especially if the min is
> 12:32:31.555.  Now we'd allow the user to set 12:32:31 which is actually below
> the min.  And it wouldn't show as out of range (two bugs right there :-) 
> Please open a bug on supporting partial seconds if there isn't one open
> already.
We agreed  a long time ago not to support partial seconds in the first iteration. We have no idea if xforms authors will even find value in using these non-numeric range types. This is one case of many that could be done in further bugs if we decide there is value in doing so.


> >Index: resources/content/xforms.css
> >===================================================================
> >-  /* range type="xsd:dateTime" */
> >+  /* range type="xsd:dateTime", "xsd:date", "xsd:time" */
> > xul|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"],
> >-html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"] {
> >-  -moz-binding: url('chrome://xforms/content/range-xul.xml#xformswidget-range-datetime');
> >+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#dateTime"],
> >+xul|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#date"],
> >+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#date"],
> >+xul|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#time"],
> >+html|*:root range[mozType|typelist~="http://www.w3.org/2001/XMLSchema#time"] {
> >+  -moz-binding: url('chrome://xforms/content/range-xul.xml#xformswidget-range-nonnumeric');
> > }
> nit: I'm not a huge fan of 'nonnumeric' as a name, either.  Maybe
> xformswidget-range-datesandtimes?

I'm thinking ahead for the other types (bugs 372737 and 372739). If I have one range binding called range-datesandtimes it is only applicable to date, time, and dateTime. Range bindings for all of the gregorian types (gDay, gYear, etc) and durations (dayTimeDuration, yearMonthDuration) will be identical except for the name. Given that everyone is against 'code duplication' wouldn't it be better to have one single binding that is applicable to every non-mumeric type that we support?

We basically have two types of ranges: numeric (slider) and nonnumeric (all the rest).

Comment 20

[aaronr]

(In reply to comment #19)
> (In reply to comment #18)

> Sure, but what you propose? Adding something like range.minLabel.separator to
> xforms.dtd and having the JS code do this.start + &range.minLabel.separator?

That works for me, though storing it in a property on the base class and referencing it might be more efficient.  I'm not sure exactly what this.start + &range.minLabel.separator would end up doing.  Might go to the DTD to fetch it every time.


> > Looks like you don't handle partial seconds (anything after the decimal point)
> > for time or dateTime.  I guess we should.  Especially if the min is
> > 12:32:31.555.  Now we'd allow the user to set 12:32:31 which is actually below
> > the min.  And it wouldn't show as out of range (two bugs right there :-) 
> > Please open a bug on supporting partial seconds if there isn't one open
> > already.
> We agreed  a long time ago not to support partial seconds in the first
> iteration. We have no idea if xforms authors will even find value in using
> these non-numeric range types. This is one case of many that could be done in
> further bugs if we decide there is value in doing so.

That's fine.  Just open a bug on it.  Put in some text that it to say that the bug is just a placeholder until we get feedback requesting partial seconds support and testcases from real users.  That way I can reference the bug via a readme item as a known limitation.


> > > }
> > nit: I'm not a huge fan of 'nonnumeric' as a name, either.  Maybe
> > xformswidget-range-datesandtimes?
> 
> I'm thinking ahead for the other types (bugs 372737 and 372739). If I have one
> range binding called range-datesandtimes it is only applicable to date, time,
> and dateTime. Range bindings for all of the gregorian types (gDay, gYear, etc)
> and durations (dayTimeDuration, yearMonthDuration) will be identical except for
> the name. Given that everyone is against 'code duplication' wouldn't it be
> better to have one single binding that is applicable to every non-mumeric type
> that we support?
> 
> We basically have two types of ranges: numeric (slider) and nonnumeric (all the
> rest).
> 

I suggested xformswidget-range-datesandtimes as a coverall for the non-numeric ranges we currently wish to support.  That would include gYearMonth (a period of time), gMonth (a recurring period of time), duration (a span of time), etc.  I'd rather the binding name reflect what it is rather than what it isn't.  If we can't come up with a better name by tomorrow, then I'll live with nonnumeric.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> 
> > Sure, but what you propose? Adding something like range.minLabel.separator to
> > xforms.dtd and having the JS code do this.start + &range.minLabel.separator?
> 
> That works for me, though storing it in a property on the base class and
> referencing it might be more efficient.  I'm not sure exactly what this.start +
> &range.minLabel.separator would end up doing.  Might go to the DTD to fetch it
> every time.
> 

Probably should be this.start + "&range.minLabel.separator". Though it looks hackly.

> I suggested xformswidget-range-datesandtimes as a coverall for the non-numeric
> ranges we currently wish to support.  That would include gYearMonth (a period
> of time), gMonth (a recurring period of time), duration (a span of time), etc. 
> I'd rather the binding name reflect what it is rather than what it isn't.  If
> we can't come up with a better name by tomorrow, then I'll live with
> nonnumeric.
> 

I like more too datesandtimes than nonumeric.

Comment 22

[Merle Sterling]

Attachment: patch

Comment 23

[aaronr]

Comment on attachment 258437 [details] [diff] [review]
patch


>+      <!-- Start label separator -->
>+      <property name="startSeparator">
>+        <getter>
>+        <![CDATA[
>+          return " >> ";
>+        ]]>
>+        </getter>
>+      </property>
>+
>+      <!-- End label separator -->
>+      <property name="endSeparator">
>+        <getter>
>+        <![CDATA[
>+          return " << ";
>+        ]]>
>+        </getter>
>+      </property>
>+
>+

this isn't exactly what I had in mind.  Someone would have to write .js to override.  I'd like them to just be able to change a .properties or .dtd file.  But I talked to mkaply and found out that I was wrong in my previous post.  You can't reference a DTD value from JS like that.  To get a string value in JS you'd have to use nsIStringBundle to get the value from a .properties file.  Seems like overkill to do all of that interface and service stuff just to initialize a couple of strings, though.

If you want to use a DTD (which is probably the most efficient way), I'd say you could do this by setting the initial value to minLabel and maxLabel to be the separator value and then during the constructor, store them off into a field.  And then use the properties like you have now, but grab the separator from the field instead of hard coding it.

the rest of the changes look good.

Comment 24

[Merle Sterling]

Attachment: patch

Using DTD for start and end label separators.

Comment 25

[aaronr]

Attachment: patch checked in

patch that I checked into trunk.  Same as previous, just removed an extraneous space at the end of a line.

Comment 26

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16