The default bug view has changed. See this FAQ.

Support Date and Time types for Range

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
10 years ago
9 months ago

People

(Reporter: Merle Sterling, Assigned: Merle Sterling)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
x86
Windows XP
fixed1.8.0.12, fixed1.8.1.4
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Blocks: 316353
Depends on: 370551

Comment 1

10 years ago
attach testcase, please
Assignee: xforms → msterlin
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

10 years ago
Created attachment 257417 [details]
testcase: date
(Assignee)

Comment 3

10 years ago
Created attachment 257418 [details]
testcase: time
(Assignee)

Comment 4

10 years ago
Created attachment 257428 [details]
testcase:date (XUL)
(Assignee)

Comment 5

10 years ago
Created attachment 257429 [details]
testcase:time (XUL)

Updated

10 years ago
Attachment #257428 - Attachment mime type: text/xml → application/vnd.mozilla.xul+xml
(Assignee)

Updated

10 years ago
Attachment #257429 - Attachment mime type: text/xml → application/vnd.mozilla.xul+xml
(Assignee)

Comment 6

10 years ago
Created attachment 257451 [details] [diff] [review]
patch
Attachment #257451 - Flags: review?(surkov.alexander)

Comment 7

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

Comment 8

10 years ago
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.
(Assignee)

Updated

10 years ago
Attachment #257451 - Flags: review?(aaronr)
(Assignee)

Comment 9

10 years ago
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.
Attachment #257451 - Flags: review?(surkov.alexander)
Attachment #257451 - Flags: review?(aaronr)
(Assignee)

Comment 10

10 years ago
Created attachment 257592 [details] [diff] [review]
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.
Attachment #257592 - Flags: review?(surkov.alexander)

Comment 11

10 years ago
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?
(Assignee)

Comment 12

10 years ago
(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().

(Assignee)

Updated

10 years ago
Attachment #257592 - Flags: review?(surkov.alexander)
(Assignee)

Comment 13

10 years ago
Created attachment 257714 [details] [diff] [review]
patch

Now using a single xformswidget-range-nonnumeric binding that can be used for all of these non-numeric range types.
Attachment #257451 - Attachment is obsolete: true
Attachment #257592 - Attachment is obsolete: true
Attachment #257714 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Blocks: 372737

Comment 14

10 years ago
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
Attachment #257714 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 15

10 years ago
(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

10 years ago
(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.
(Assignee)

Comment 17

10 years ago
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.

(Assignee)

Updated

10 years ago
Attachment #257714 - Flags: review?(aaronr)

Comment 18

10 years ago
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?
Attachment #257714 - Flags: review?(aaronr) → review-
(Assignee)

Comment 19

10 years ago
(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

10 years ago
(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

10 years ago
(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.
(Assignee)

Comment 22

10 years ago
Created attachment 258437 [details] [diff] [review]
patch
Attachment #257714 - Attachment is obsolete: true
Attachment #258437 - Flags: review?(aaronr)

Comment 23

10 years ago
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.
Attachment #258437 - Flags: review?(aaronr) → review-
(Assignee)

Comment 24

10 years ago
Created attachment 258600 [details] [diff] [review]
patch

Using DTD for start and end label separators.
Attachment #258437 - Attachment is obsolete: true
Attachment #258600 - Flags: review?(aaronr)

Updated

10 years ago
Attachment #258600 - Flags: review?(aaronr) → review+

Comment 25

10 years ago
Created attachment 258715 [details] [diff] [review]
patch checked in

patch that I checked into trunk.  Same as previous, just removed an extraneous space at the end of a line.
Attachment #258600 - Attachment is obsolete: true

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 26

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.