Last Comment Bug 372739 - Support duration types for Range
: Support duration types for Range
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
:
Mentors:
Depends on:
Blocks: 316353 376392
  Show dependency treegraph
 
Reported: 2007-03-05 12:03 PST by Merle Sterling
Modified: 2016-07-15 14:46 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase: dayTimeDuration (1.14 KB, text/xml)
2007-03-05 14:22 PST, Merle Sterling
no flags Details
testcase: yearMonthDuration (1.15 KB, text/xml)
2007-03-05 14:23 PST, Merle Sterling
no flags Details
testcase: dayTimeDuration (XUL) (1.05 KB, application/vnd.mozilla.xul+xml)
2007-03-05 15:20 PST, Merle Sterling
no flags Details
testcase: yearMonthDuration (XUL) (1.03 KB, application/vnd.mozilla.xul+xml)
2007-03-05 15:21 PST, Merle Sterling
no flags Details
patch (12.66 KB, patch)
2007-03-16 06:21 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
testcase2: dayTimeDuration and yearMonthDuration in xhtml (2.23 KB, application/xhtml+xml)
2007-03-16 17:55 PDT, aaronr
no flags Details
yearMonthDuration patch (19.45 KB, patch)
2007-03-30 09:10 PDT, Merle Sterling
surkov.alexander: review+
aaronr: review+
Details | Diff | Splinter Review
yearMonthDuration patch for check-in (19.91 KB, patch)
2007-04-02 16:12 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
yearMonthDuration patch (20.12 KB, patch)
2007-04-02 17:19 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review
testcase: dayTimeDuration (1.13 KB, text/xml)
2007-04-03 12:14 PDT, Merle Sterling
no flags Details
testcase: dayTimeDuration (XUL) (1.05 KB, application/vnd.mozilla.xul+xml)
2007-04-03 12:15 PDT, Merle Sterling
no flags Details
testcase: dayTimeDuration (less than 1 full day) (2.29 KB, text/xml)
2007-04-05 07:12 PDT, Merle Sterling
no flags Details
testcase: dayTimeDuration (multiple days) (2.29 KB, text/xml)
2007-04-05 07:16 PDT, Merle Sterling
no flags Details
dayTimeDuration patch (22.01 KB, patch)
2007-04-05 07:22 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
dayTimeDuration patch (24.17 KB, patch)
2007-04-06 18:41 PDT, Merle Sterling
surkov.alexander: review+
Details | Diff | Splinter Review
dayTimeDuration patch (24.05 KB, patch)
2007-04-09 11:12 PDT, Merle Sterling
no flags Details | Diff | Splinter Review
dayTimeDuration patch (24.38 KB, patch)
2007-04-09 16:54 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
dayTimeDuration patch (25.55 KB, patch)
2007-04-12 22:46 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review

Description Merle Sterling 2007-03-05 12:03:56 PST
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 duration types for range: dayTimeDuration and yearMonthDuration.

Reproducible: Always
Comment 1 aaronr 2007-03-05 12:13:18 PST
attach testcases, pleaase
Comment 2 Merle Sterling 2007-03-05 14:22:41 PST
Created attachment 257425 [details]
testcase: dayTimeDuration
Comment 3 Merle Sterling 2007-03-05 14:23:12 PST
Created attachment 257426 [details]
testcase: yearMonthDuration
Comment 4 Merle Sterling 2007-03-05 15:20:51 PST
Created attachment 257436 [details]
testcase: dayTimeDuration (XUL)
Comment 5 Merle Sterling 2007-03-05 15:21:28 PST
Created attachment 257437 [details]
testcase: yearMonthDuration (XUL)
Comment 6 Merle Sterling 2007-03-16 06:21:45 PDT
Created attachment 258775 [details] [diff] [review]
patch
Comment 7 aaronr 2007-03-16 17:49:42 PDT
Comment on attachment 258775 [details] [diff] [review]
patch

Your anonymous content is way too restrictive.  You are only handling one field for each duration -> seconds for dayTimeDuration and months for yearMonthDuration.  What about the other possible values?  P12DT12H12M12S is a valid dayTimeDuration and P12Y12M is a valid yearMonthDuration.

I'll attach a testcase you can work against.

>Index: resources/locale/en-US/xforms.dtd
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.dtd,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 xforms.dtd
>--- resources/locale/en-US/xforms.dtd	15 Mar 2007 21:41:37 -0000	1.11
>+++ resources/locale/en-US/xforms.dtd	16 Mar 2007 13:20:49 -0000
>@@ -70,15 +70,19 @@
> 
> <!ENTITY xforms.date.year.label        "Year">
> <!ENTITY xforms.date.month.label       "Month">
> <!ENTITY xforms.date.day.label         "Day">
> <!ENTITY xforms.datetime.hours.label   "Hours">
> <!ENTITY xforms.datetime.minutes.label "Minutes">
> <!ENTITY xforms.datetime.seconds.label "Seconds">
> 
>+<!ENTITY xforms.daytimeduration.label "Seconds">
>+<!ENTITY xforms.yearmonthduration.label "Months">
>+
> <!-- &#34; evaluates to a single quote
>     (http://www.w3.org/TR/html401/sgml/entities.html)
>     Since we are using these to initialize xbl fields, we need to quote them
>     or we'll get an assortment of errors.
> -->
> <!ENTITY xforms.range.start.separator "<![CDATA[&#34; >> &#34;]]>">
> <!ENTITY xforms.range.end.separator   "<![CDATA[&#34; << &#34;]]>">
>+<!ENTITY xforms.datetime.seconds.label "Seconds">

Looks like you already have the xforms.datetime.seconds.label entity, right?
Comment 8 aaronr 2007-03-16 17:55:00 PDT
Created attachment 258863 [details]
testcase2: dayTimeDuration and yearMonthDuration in xhtml
Comment 9 alexander :surkov 2007-03-16 23:00:13 PDT
Comment on attachment 258775 [details] [diff] [review]
patch

cancelling until new patch
Comment 10 Merle Sterling 2007-03-19 09:42:12 PDT
(In reply to comment #7)
> (From update of attachment 258775 [details] [diff] [review])
> Your anonymous content is way too restrictive.  You are only handling one field
> for each duration -> seconds for dayTimeDuration and months for
> yearMonthDuration.  What about the other possible values?  P12DT12H12M12S is a
> valid dayTimeDuration and P12Y12M is a valid yearMonthDuration.
The spec says 'the value space for dayTimeDuration is the set of fractional second values' and likewise for yearMonthDuration is is the set of integer month values. Thus there is only one field and the duration has to be collapsed into a range of seconds or months respectively.

I will have to verify it but I think the validator only supports the forms PTSS.sss for dayTimeDuration and PnM for yearMonthDuration and that is why the patch only includes support for those formats. 
Comment 11 aaronr 2007-03-19 10:33:30 PDT
(In reply to comment #10)
> (In reply to comment #7)
> > (From update of attachment 258775 [details] [diff] [review] [details])
> > Your anonymous content is way too restrictive.  You are only handling one field
> > for each duration -> seconds for dayTimeDuration and months for
> > yearMonthDuration.  What about the other possible values?  P12DT12H12M12S is a
> > valid dayTimeDuration and P12Y12M is a valid yearMonthDuration.
> The spec says 'the value space for dayTimeDuration is the set of fractional
> second values' and likewise for yearMonthDuration is is the set of integer
> month values. Thus there is only one field and the duration has to be collapsed
> into a range of seconds or months respectively.
> 

You are absolutely right in that is how the values are treated and compared.  However, if you look in the XForms schema, you can see what expressions are valid for these types: http://www.w3.org/MarkUp/Forms/2002/XForms-Schema.xsd

> I will have to verify it but I think the validator only supports the forms
> PTSS.sss for dayTimeDuration and PnM for yearMonthDuration and that is why the
> patch only includes support for those formats. 
> 

Looking at nsXFormsSchemaValidator.cpp, it looks like it tries to handle all the different possible fields of the duration.  We will certainly have a problem with anything that extends/restricts the xforms types, though, until we ship the xforms schema with our chrome (bug 343122)
Comment 12 Merle Sterling 2007-03-19 11:11:13 PDT
(In reply to comment #11)
> Looking at nsXFormsSchemaValidator.cpp, it looks like it tries to handle all
> the different possible fields of the duration.  
That's not the way I see it.

dayTimeDuration:     // if years/months exist, invalid
The validator wants nothing but seconds, hence PTss.sss

yearMonthDuration:     // check if no days/hours/minutes/seconds/fractionseconds were set
The validator wants nothing but months, hence PnM.

The XBL range for duration types handles only what is currently considered valid. If the definition of what is a valid duration changes in the future, then of course we can update the ranges.

Comment 13 Merle Sterling 2007-03-30 09:10:15 PDT
Created attachment 260141 [details] [diff] [review]
yearMonthDuration patch
Comment 14 alexander :surkov 2007-03-31 11:18:17 PDT
Comment on attachment 260141 [details] [diff] [review]
yearMonthDuration patch


>+  <!-- YEARMONTHDURATION RANGE-->
>+  <binding id="range-yearmonthduration"

>+      <!-- Extract the minutes from a duration -->
>+      <method name="getMinutes">
>+        <parameter name="aDuration"/>
>+        <body>
>+        <![CDATA[
>+          // Minutes is represented by the designator 'M', but so is
>+          // Months, so first extract the time portion of the duration.
>+          var indexT = aDuration.indexOf("T");
>+          var time = aDuration.substring(0, indexT);
>+          return this.getDurationValue(aDuration, "M");

should be this.getDurationValue(time, "M") I guess.

>+      <method name="getDurationValue">
>+        <parameter name="aDuration"/>
>+        <parameter name="aDesignator"/>
>+        <body>
>+        <![CDATA[
>+          var duration = 0;
>+          // Find the designator.
>+          var index = aDuration.indexOf(aDesignator);
>+          if (index != -1) {
>+            // Find the starting index of the value by searching
>+            // backwards until we encounter another designator.
>+            var i = index - 1;
>+            while (!this.isDesignator(aDuration.charCodeAt(i)))
>+              --i;
>+
>+            // The value is between the two designators.
>+            duration = parseFloat(aDuration.substring(i + 1, index));
>+          }

I'm thinking regexp will be more clear here. Something like this:

var regexpRep = aDesignator + "\d+[PYMDTHS]";
re = new RegExp(regexpRep);
durationValue = aDuration.match(re);

Also this helps to remove not nice isDesignator() method.
Comment 15 Merle Sterling 2007-04-02 12:50:35 PDT
(In reply to comment #14)
> >+      <method name="getDurationValue">
> >+        <parameter name="aDuration"/>
> >+        <parameter name="aDesignator"/>
> >+        <body>
> >+        <![CDATA[
> >+          var duration = 0;
> >+          // Find the designator.
> >+          var index = aDuration.indexOf(aDesignator);
> >+          if (index != -1) {
> >+            // Find the starting index of the value by searching
> >+            // backwards until we encounter another designator.
> >+            var i = index - 1;
> >+            while (!this.isDesignator(aDuration.charCodeAt(i)))
> >+              --i;
> >+
> >+            // The value is between the two designators.
> >+            duration = parseFloat(aDuration.substring(i + 1, index));
> >+          }
> I'm thinking regexp will be more clear here. Something like this:
> var regexpRep = aDesignator + "\d+[PYMDTHS]";
> re = new RegExp(regexpRep);
> durationValue = aDuration.match(re);
> Also this helps to remove not nice isDesignator() method.

Ok, so I tried using RegExp and the pattern can be simplified to "\\d+" + aDesignator. It works, but is incredibly SLOW! It takes at least half a second to change the value and the rapid spinning of the spinbuttons is gone.

RegExp may be too heavyweight of a solution here. There is no perceptible delay in changing the values with my method that does it manually.


Comment 16 aaronr 2007-04-02 16:07:38 PDT
with the fix for getMinutes that surkov mentioned (and minus the slow regexp), r=me.
Comment 17 Merle Sterling 2007-04-02 16:12:41 PDT
Created attachment 260395 [details] [diff] [review]
yearMonthDuration patch for check-in

Fix getMinutes method of duration base class.
Comment 18 aaronr 2007-04-02 16:18:08 PDT
Comment on attachment 260395 [details] [diff] [review]
yearMonthDuration patch for check-in

looks fine, I'll check it in.
Comment 19 aaronr 2007-04-02 16:33:42 PDT
Comment on attachment 260395 [details] [diff] [review]
yearMonthDuration patch for check-in

changing r+ to r-, found error with its handling of the yearMonth testcases.  I've notified Merle.
Comment 20 Merle Sterling 2007-04-02 17:19:24 PDT
Created attachment 260401 [details] [diff] [review]
yearMonthDuration patch
Comment 21 aaronr 2007-04-02 17:54:30 PDT
Comment on attachment 260401 [details] [diff] [review]
yearMonthDuration patch

looks good.  Tests good.
Comment 22 aaronr 2007-04-03 09:58:33 PDT
(In reply to comment #21)
> (From update of attachment 260401 [details] [diff] [review])
> looks good.  Tests good.
> 

checked in yearMonthDuration patch for msterlin last night.
Comment 23 Merle Sterling 2007-04-03 12:14:28 PDT
Created attachment 260497 [details]
testcase: dayTimeDuration

Revised dayTimeDuration testcase with the correct duration format.
Comment 24 Merle Sterling 2007-04-03 12:15:31 PDT
Created attachment 260498 [details]
testcase: dayTimeDuration (XUL)

Revised dayTimeDuration (XUL) testcase with the correct duration format.
Comment 25 Merle Sterling 2007-04-05 07:12:38 PDT
Created attachment 260701 [details]
testcase: dayTimeDuration (less than 1 full day)

Testcase for dayTimeDuration where the duration is less than 1 full day
and the time values span from late hours in the start day to late hours in the end day (so hours in the start is > hours in the end). The range is a valid dayTimeDuration but is tricky to handle properly.

The dayTimeDuration code should also handle durations of less than 1 day when the day values are the same.
Comment 26 Merle Sterling 2007-04-05 07:16:20 PDT
Created attachment 260702 [details]
testcase: dayTimeDuration (multiple days)

This testcase shows a range of multiple days with overlapping hours, minutes, and seconds and is a good test for how the dayTimeDuration range calculates the min and max attributes for the spinbuttons when a change in any one of them affects all of the others.
Comment 27 Merle Sterling 2007-04-05 07:22:17 PDT
Created attachment 260703 [details] [diff] [review]
dayTimeDuration patch

Patch for dayTimeDuration range. Use the dayTimeDuration testcases to exercise it. The testcases cover all of the difficult cases where the flexibility of the spec makes dealing with the ranges very complicated.

Spinbuttons for each individual component of a range type worked out ok for all of the other types but dayTimeDuration is in a league of its own and is practically incomprehensible despite verbose comments.
Comment 28 aaronr 2007-04-06 03:56:16 PDT
Comment on attachment 260703 [details] [diff] [review]
dayTimeDuration patch

Double check my comments, but I think that they are correct.

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

>+  <!-- DAYTIMEDURATION RANGE-->
>
>+      <method name="setSpinbuttonMinMax">

>+
>+          // Min and Max for the hours spinbutton depends on the day range.
>+          if (endDays - startDays == 0) {

nit: why not just test endDays == startDays?  One less operation.

>+            // Range is less than one full day but the days of the start
>+            // and end are the same.
>+            // Example: start=P1DT0H0M0S, end=P1DT23H59M259S
>+            minHours = startHours;
>+            maxHours = endHours;
>+
>+            if (currentHours == startHours) {
>+              // Example: start=P1DT0H0M0S, end=P1DT0H59M59S
>+              minMinutes = startMinutes;
>+              maxMinutes = endMinutes;

maxMinutes = endMinutes is only true if currentHours = endHours.  But then you minMinutes assignment will be off.  Both are true only if startHours == endHours.

>+
>+              if (startMinutes == endMinutes) {
>+                // Current day is the start day, current hour is the start
>+                // hour, and current minute is the start minute; ie this is
>+                // a simple range of seconds only.
>+                // Example: start=P0DT0H0M10S, end=P0DT0H0M20S

Your comment is off.  currentDay = startDay, etc. does not mean that this is a range of seconds only.  Only if endDay = startDay, endHour=StartHour, endMinute = startMinute.

>+                minSeconds = startSeconds;
>+                maxSeconds = endSeconds;
>+              } else {
>+                if (currentMinutes == startMinutes) {
>+                  // Current minutes is the start minutes of the start day and
>+                  // start hour.Seconds can range from the start seconds to the
>+                  // maximum number of seconds in a minute.
>+                  minSeconds = startSeconds;
>+                  maxSeconds = 59;
>+                } else if (currentMinutes == endMinutes) {
>+                  //  Current minutes is the end minutes of the start day and
>+                  // start hour. Seconds can range from 0 to the number of
>+                  // seconds in the end duration.
>+                  minSeconds = 0;
>+                  maxSeconds = endSeconds;
>+                } else {
>+                  // Current minutes is between the start and end minutes of
>+                  // the start day/start hour.
>+                  minSeconds = 0;
>+                  maxSeconds = 59;
>+                }
>+              }
>+            } else {
>+              // Current hours value is between the start and end hours values
>+              // of the same day. Minutes and seconds can range between their
>+              // miminimum and maximum values.
>+              // Example: start=P1DT0H0M0S, end=P1DT1H59M59S
>+              minMinutes = 0;
>+              maxMinutes = 59;
>+              minSeconds = 0;
>+              maxSeconds = 59;
>+            }
>+          } else if (endDays - startDays == 1) {
>+            // Range is less than one full day but the days of the start
>+            // and end are different.
>+            // Example: start=P0D23H59M59S, end=P1D20H30M210S
>+            if (currentDays == startDays) {
>+              // Current day value is equal to the start day.
>+              // The min hour is the start hours and the max hours is the
>+              // maximum number of hours in a day.
>+              minHours = startHours;
>+              maxHours = 23;
>+              // Minutes can range between the minutes of the start duration
>+              // and the maximum number of minutes in an hour.
>+              minMinutes = startMinutes;
>+              maxMinutes = 59;
>+              // Seconds can range between the seconds of the start duration
>+              // and the maximum number of seconds in a minute.
>+              minSeconds = startSeconds;
>+              maxSeconds = 59;
>+            } else if (currentDays == endDays) {
>+              // Current day value is equal to the end day.
>+              // The min hour is the minimum number of hours in a day and the
>+              // max hours is the hours of the end duration.
>+              minHours = 0;
>+              maxHours = endHours;
>+
>+              if (currentHours == endHours) {
>+                // Current hour is the end hour of the end day.
>+                // Minutes can range between the minimum number of minutes in
>+                // an hour to the minutes of the end duration.
>+                minMinutes = 0;
>+                maxMinutes = endMinutes;
>+                // Seconds can range from the minimum number of seconds in a
>+                // minute to the seconds of the end duration.
>+                minSeconds = 0;
>+                maxSeconds = endSeconds;


This doesn't seem right.  If I have end of 1D2H3M4S and curr value is 1D2H, then max min is 3, but max seconds could be any value until cur min == 3.

>+
>+          // Set the min and max for the days, hours, minutes,
>+          // and seconds spinbuttons.
>+          this.daySpin.min = startDays;
>+          this.daySpin.max = endDays;
>+          this.hoursSpin.min = minHours;
>+          this.hoursSpin.max = maxHours;
>+          this.minutesSpin.min = minMinutes;
>+          this.minutesSpin.max = maxMinutes;
>+          this.secondsSpin.min = minSeconds;
>+          this.secondsSpin.max = maxSeconds;
>+
>+          debugger;

please remove the debugger; statement

>+
>+      <property name="daySpin" readonly="true">
>+        <getter>
>+          if (!this._daySpin) {
>+            this._daySpin = this.ownerDocument.
>+              getAnonymousElementByAttribute(this, "anonid", "daySpin");
>+          }
>+          return this._daySpin;
>+        </getter>
>+      </property>

you are missing the _daySpin field
Comment 29 Merle Sterling 2007-04-06 18:38:54 PDT
(In reply to comment #28)
> >+            // Range is less than one full day but the days of the start
> >+            // and end are the same.
> >+            // Example: start=P1DT0H0M0S, end=P1DT23H59M259S
> >+            minHours = startHours;
> >+            maxHours = endHours;
> >+
> >+            if (currentHours == startHours) {
> >+              // Example: start=P1DT0H0M0S, end=P1DT0H59M59S
> >+              minMinutes = startMinutes;
> >+              maxMinutes = endMinutes;
> maxMinutes = endMinutes is only true if currentHours = endHours.  But then
> your minMinutes assignment will be off.  Both are true only if startHours ==
> endHours.
Good catch, you are correct. 
> >+
> >+              if (startMinutes == endMinutes) {
> >+                // Current day is the start day, current hour is the start
> >+                // hour, and current minute is the start minute; ie this is
> >+                // a simple range of seconds only.
> >+                // Example: start=P0DT0H0M10S, end=P0DT0H0M20S
> Your comment is off.  currentDay = startDay, etc. does not mean that this is > a range of seconds only.  Only if endDay=startDay, endHour=StartHour,
> and endMinutes=startMinutes.
Actually it does mean that it is a range of seconds only because the code above it will have already determined that days and hours are equal. I agree it is confusing, so I have reworked the logic to test explicitly for startHours=endHours and startMinutes=endMinutes. As a side effect, the logic for a range of minutes and seconds only is easier to follow.

> >+          } else if (endDays - startDays == 1) {
> >+            .........................
> >+              if (currentHours == endHours) {
> >+                // Current hour is the end hour of the end day.
> >+                // Minutes can range between the minimum number of minutes in
> >+                // an hour to the minutes of the end duration.
> >+                minMinutes = 0;
> >+                maxMinutes = endMinutes;
> >+                // Seconds can range from the minimum number of seconds in a
> >+                // minute to the seconds of the end duration.
> >+                minSeconds = 0;
> >+                maxSeconds = endSeconds;
> This doesn't seem right.  If I have end of 1D2H3M4S and curr value is 1D2H,
> then max min is 3, but max seconds could be any value until cur min == 3.
It is correct. That example would be handled by the first test to determine if startDays == endDays.

Thanks for taking the time to look at the logic. I think I have all the cases covered now. I'm adding a few more examples in the comments and clarifying some points. I hope nobody minds verbose comments because if I have to look at this code in the future I'm sure I'd be totally lost without the help of comments. I prefer too many comments vs not enough every time.
Comment 30 Merle Sterling 2007-04-06 18:41:18 PDT
Created attachment 260869 [details] [diff] [review]
dayTimeDuration patch

Fixing the problems found by Aaron and adding more examples in the comments. The patch handles all of the testcases perfectly so I'm fairly confident it is ready to go.
Comment 31 alexander :surkov 2007-04-07 08:10:43 PDT
Comment on attachment 260869 [details] [diff] [review]
dayTimeDuration patch


>+      <method name="updateFields">
>+        <body>
>+        <![CDATA[
>+          var value = this.value;
>+          var days = this.getDays(value);
>+          var hours = this.getHours(value);
>+          var minutes = this.getMinutes(value);
>+          var seconds = this.getSeconds(value);
>+
>+          this.daySpin.value = days;
>+          this.hoursSpin.value = hours;
>+          this.minutesSpin.value = minutes;
>+          this.secondsSpin.value = seconds;

Why not this.daySpin.value = this.getDays(value) ?

>+
>+      <method name="setSpinbuttonMinMax">
>+        <body>

If you would split this huge method onto several then it would be fine. I really don't like big methods, it's hard readable code. Though it's up to you.
Comment 32 Merle Sterling 2007-04-09 09:13:08 PDT
Splitting setSpinbuttonMinMax into individual methods that calculate min/max for each field like the other range types do was not feasible for dayTimeDuration. A change in one spinbutton affects nearly all of the others and each method was large and had to look up all of the attributes and extract the same fields from the duration string.

The one method is large but you can follow the logic from top to bottom and we do the minimum number of attribute lookups and field extractions (getDays, getHours, etc).
Comment 33 Merle Sterling 2007-04-09 11:12:53 PDT
Created attachment 261023 [details] [diff] [review]
dayTimeDuration patch

Simplify updateFields method per surkov's last comment.
Comment 34 aaronr 2007-04-09 15:32:01 PDT
Comment on attachment 261023 [details] [diff] [review]
dayTimeDuration patch

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

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

>+
>+      <method name="setSpinbuttonMinMax">

>+            } else if (currentHours == endHours) {
>+              // Current hours value is equal to the end hours. The min minutes
>+              // is minimum number of minutes in an hour and the max minutes
>+              // is the minutes of the end duration. The min seconds is the
>+              // minimum number of seconds in a minute and the max seconds is
>+              // the seconds of the end duration.
>+              // Example: start=P1DT0H0M0S, end=P1DT1H59M59S
>+              minMinutes = 0;
>+              maxMinutes = endMinutes;
>+              minSeconds = 0;
>+              maxSeconds = maxSeconds;

I assume that this should be maxSeconds = endSeconds?

>+            } else {
>+              // Current hours value is between the start and end hours values
>+              // of the same day. Minutes and seconds can range between their
>+              // miminimum and maximum values.
>+              // Example: start=P1DT0H0M0S, end=P1DT10H59M59S
>+              minMinutes = 0;
>+              maxMinutes = 59;
>+              minSeconds = 0;
>+              maxSeconds = 59;
>+            }
>+          } else if (endDays - startDays == 1) {
>+            // Range is less than one full day but the days of the start
>+            // and end are different.
>+            // Example: start=P0D23H59M59S, end=P1D20H30M210S

nit: I assume end should be 21S or 10S, but not 210S.

>+            // OR
>+            // The range is greater than or equal to 1 day but less than
>+            // 2 days and the start and end days are different.
>+            // Example: start=P0D0H0M0S, end=P1D23H10M10S
>+            // Example: start=P0D0H0M0S, end=P1D0H0M0S
>+
>+            // Hours, minutes, and seconds depend on the current day.
>+            if (currentDays == startDays) {
>+              // Current day value is equal to the start day.
>+              // The min hour is the start hours and the max hours is the
>+              // maximum number of hours in a day.
>+              minHours = startHours;
>+              maxHours = 23;
>+              // Minutes can range between the minutes of the start duration
>+              // and the maximum number of minutes in an hour.
>+              minMinutes = startMinutes;
>+              maxMinutes = 59;
>+              // Seconds can range between the seconds of the start duration
>+              // and the maximum number of seconds in a minute.
>+              minSeconds = startSeconds;
>+              maxSeconds = 59;
>+            } else if (currentDays == endDays) {
>+              // Current day value is equal to the end day.
>+              // The min hour is the minimum number of hours in a day and the
>+              // max hours is the hours of the end duration.
>+              minHours = 0;
>+              maxHours = endHours;
>+
>+              // Minutes and seconds depend on the current hours.
>+              if (currentHours == endHours) {
>+                // Current hour is the end hour of the end day.
>+                // Minutes can range between the minimum number of minutes in
>+                // an hour to the minutes of the end duration.
>+                minMinutes = 0;
>+                maxMinutes = endMinutes;
>+                // Seconds can range from the minimum number of seconds in a
>+                // minute to the seconds of the end duration.
>+                minSeconds = 0;
>+                maxSeconds = endSeconds;

I still think this is wrong.  How about with the example of start=P0D23H59M59S, end=P1D20H30M21S.  I could never set the current range value to P1D20H29M59S because my maxSeconds would be endSeconds which is 21.
Comment 35 Merle Sterling 2007-04-09 16:53:17 PDT
(In reply to comment #34)
> I still think this is wrong.  How about with the example of
> start=P0D23H59M59S, end=P1D20H30M21S.  I could never set the current range 
> value to P1D20H29M59S because my maxSeconds would be endSeconds which is 21.
Yes, you're right. Have to take into account the current minutes to determine the range of valid seconds.


Comment 36 Merle Sterling 2007-04-09 16:54:06 PDT
Created attachment 261082 [details] [diff] [review]
dayTimeDuration patch
Comment 37 aaronr 2007-04-10 16:51:17 PDT
I was testing this patch prior to checkin and found a bug with the " testcase: dayTimeDuration (less than 1 full day)" testcase.

1) load the testcase
2) change the days to 0.  Everything will shift to their min values as they should.  However the selected value doesn't reflect this fact (the hours are wrong).
3) change days back to 1.  The day, hours and minutes will correctly adjust to their max values, but the seconds stays at 59 which is beyond the range.  The selected value will also be wrong (minutes and seconds are wrong).

changing r+ to r- on the patch until this is fixed.
Comment 38 Merle Sterling 2007-04-12 22:43:33 PDT
(In reply to comment #37)
> I was testing this patch prior to checkin and found a bug with the " testcase:
> dayTimeDuration (less than 1 full day)" testcase.
Example: start=P0D23H59M59S, end=P1D20H30M10S

This is tricky because of the range of less than one day where the start duration is near the end of the first day and the end duration is in the middle of the next day. This leads to the case where the current hours/minutes can initially be greater than the hours/minutes of the end day and it is not sufficient to just check if the current hours is equal to the end hours to determine the minutes and seconds range when the day changes. [Initially hours will be 23 when you change the day from 0 to 1 and 23 is already greater than the end hours of 20].

The fix is to check if the hours and/or minutes are greater than or equal to current hours/minutes. The same needs to be done when the range is greater than one day.

Additionally, the xul numberbox implementation relies on the _validateValue method when the min and max attributes are changed. It would only change its value if the new min < current min or new max > current max. This doesn't work for ranges like dayTimeDuration where a change in one spinbutton like 'days' affects hours, minutes, and seconds as well. In the example testcase, the value will still be '20' because 20 < 23 and validateValue is not called to change the value even though we changed the min=max=23. The min/max/validateValue methods need to check if the new value is <= for min or >= for max because we are effectively 'looking ahead' because a change in one spinbutton affects others.

Comment 39 Merle Sterling 2007-04-12 22:46:03 PDT
Created attachment 261439 [details] [diff] [review]
dayTimeDuration patch

Fixed and working for all testcases.
Comment 40 aaronr 2007-04-15 01:05:02 PDT
checked in dayTimeDuration patch into trunk for msterlin
Comment 41 aaronr 2007-04-23 16:40:35 PDT
checked into 1.8 branch on 2007-04-12 (yearMonthDuration patch)
checked into 1.8 branch on 2007-04-15 (dayTimeDuration patch)
checked into 1.8.0 branch on 2007-04-16

Note You need to log in before you can comment on or make changes to this bug.