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

attach testcases, pleaase

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 on attachment 258775 [details] [diff] [review] patch cancelling until new patch

(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.

(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)

(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 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.

(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.

with the fix for getMinutes that surkov mentioned (and minus the slow regexp), r=me.

Fix getMinutes method of duration base class.

Comment on attachment 260395 [details] [diff] [review] yearMonthDuration patch for check-in looks fine, I'll check it in.

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 on attachment 260401 [details] [diff] [review] yearMonthDuration patch looks good. Tests good.

(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.

Revised dayTimeDuration testcase with the correct duration format.

Revised dayTimeDuration (XUL) testcase with the correct duration format.

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.

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.

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 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

(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.

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 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.

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).

Simplify updateFields method per surkov's last comment.

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.

(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.

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.

(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.

Fixed and working for all testcases.

checked in dayTimeDuration patch into trunk for msterlin

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