Last Comment Bug 372737 - Support gregorian calendar types for Range
: Support gregorian calendar types for Range
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
Mentors:
Depends on: 372736
Blocks: 316353
  Show dependency treegraph
 
Reported: 2007-03-05 12:00 PST by Merle Sterling
Modified: 2007-04-23 16:38 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase: gDay (1.08 KB, text/xml)
2007-03-05 14:18 PST, Merle Sterling
no flags Details
testcase: gMonth (1.09 KB, text/xml)
2007-03-05 14:18 PST, Merle Sterling
no flags Details
testcase: gMonthDay (1.13 KB, text/xml)
2007-03-05 14:19 PST, Merle Sterling
no flags Details
testcase: gYear (1.07 KB, text/xml)
2007-03-05 14:19 PST, Merle Sterling
no flags Details
testcase: gYearMonth (1.14 KB, text/xml)
2007-03-05 14:20 PST, Merle Sterling
no flags Details
testcase: gDay (XUL) (1004 bytes, application/vnd.mozilla.xul+xml)
2007-03-05 15:10 PST, Merle Sterling
no flags Details
testcase: gMonth (XUL) (1010 bytes, application/vnd.mozilla.xul+xml)
2007-03-05 15:11 PST, Merle Sterling
no flags Details
testcase: gMonthDay (XUL) (1.01 KB, application/vnd.mozilla.xul+xml)
2007-03-05 15:12 PST, Merle Sterling
no flags Details
testcase: gYear (XUL) (1019 bytes, application/vnd.mozilla.xul+xml)
2007-03-05 15:12 PST, Merle Sterling
no flags Details
testcase: gYearMonth (XUL) (1.03 KB, application/vnd.mozilla.xul+xml)
2007-03-05 15:13 PST, Merle Sterling
no flags Details
patch (37.94 KB, patch)
2007-03-06 15:49 PST, Merle Sterling
no flags Details | Diff | Review
patch (52.05 KB, patch)
2007-03-07 12:48 PST, Merle Sterling
no flags Details | Diff | Review
patch (29.98 KB, patch)
2007-03-16 05:50 PDT, Merle Sterling
no flags Details | Diff | Review
patch (27.51 KB, patch)
2007-03-20 15:48 PDT, Merle Sterling
aaronr: review+
Details | Diff | Review
patch (31.71 KB, patch)
2007-03-22 13:06 PDT, Merle Sterling
surkov.alexander: review+
Details | Diff | Review
patch for check-in (31.67 KB, patch)
2007-03-26 08:19 PDT, Merle Sterling
no flags Details | Diff | Review

Description Merle Sterling 2007-03-05 12:00:42 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 gregorian calendar types for range: gYearMonth, gYear, gMonthDay, gDay, gMonth.

Reproducible: Always
Comment 1 aaronr 2007-03-05 12:12:34 PST
attach testcases, please
Comment 2 Merle Sterling 2007-03-05 14:18:10 PST
Created attachment 257420 [details]
testcase: gDay
Comment 3 Merle Sterling 2007-03-05 14:18:44 PST
Created attachment 257421 [details]
testcase: gMonth
Comment 4 Merle Sterling 2007-03-05 14:19:14 PST
Created attachment 257422 [details]
testcase: gMonthDay
Comment 5 Merle Sterling 2007-03-05 14:19:44 PST
Created attachment 257423 [details]
testcase: gYear
Comment 6 Merle Sterling 2007-03-05 14:20:18 PST
Created attachment 257424 [details]
testcase: gYearMonth
Comment 7 Merle Sterling 2007-03-05 15:10:40 PST
Created attachment 257431 [details]
testcase: gDay (XUL)
Comment 8 Merle Sterling 2007-03-05 15:11:27 PST
Created attachment 257432 [details]
testcase: gMonth (XUL)
Comment 9 Merle Sterling 2007-03-05 15:12:07 PST
Created attachment 257433 [details]
testcase: gMonthDay (XUL)
Comment 10 Merle Sterling 2007-03-05 15:12:47 PST
Created attachment 257434 [details]
testcase: gYear (XUL)
Comment 11 Merle Sterling 2007-03-05 15:13:32 PST
Created attachment 257435 [details]
testcase: gYearMonth (XUL)
Comment 12 Merle Sterling 2007-03-06 15:49:56 PST
Created attachment 257576 [details] [diff] [review]
patch
Comment 13 Merle Sterling 2007-03-07 12:48:14 PST
Created attachment 257729 [details] [diff] [review]
patch

Updating patch based on current version of patch 372736.
Comment 14 Merle Sterling 2007-03-16 05:50:52 PDT
Created attachment 258772 [details] [diff] [review]
patch
Comment 15 aaronr 2007-03-16 16:21:56 PDT
Comment on attachment 258772 [details] [diff] [review]
patch

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

Everything in range-gyear, range-gmonth and range-gday are pretty much the same except for small differences in the anonymous content.  Or am I wrong?  Can't you make one a base class and have the others extend it (they'd each have their own anonymous content, of course)?  The base class really doesn't need to know anything about year/month/day.  Just how to set labels, min, max and value attributes, etc.  gYear would also need to override updateValue so that it formats the number with 4 digits.

Rest of the patch looks good to me.  Removing from my review queue until I hear from Merle.
Comment 16 alexander :surkov 2007-03-19 00:32:28 PDT
Comment on attachment 258772 [details] [diff] [review]
patch

(In reply to comment #15)

> Rest of the patch looks good to me.  Removing from my review queue until I hear
> from Merle.
> 

Removing me too until Aaron is happy with the patch ;)
Comment 17 Merle Sterling 2007-03-19 09:36:17 PDT
(In reply to comment #15)
> (From update of attachment 258772 [details] [diff] [review])
> >Index: resources/content/widgets-xul.xml
> >===================================================================
> Everything in range-gyear, range-gmonth and range-gday are pretty much the same
> except for small differences in the anonymous content.  Or am I wrong?  Can't
> you make one a base class and have the others extend it (they'd each have their
> own anonymous content, of course)?  The base class really doesn't need to know
> anything about year/month/day.  Just how to set labels, min, max and value
> attributes, etc.  gYear would also need to override updateValue so that it
> formats the number with 4 digits.
There is a base class - gregorian - and the others do extend it. We could move updateLabels to the base class but that is about it.

The utility methods like 'getYear' could be removed if I use parseFloat directly in the multiple places it is used but I chose not do it that way.

Because you cannot inherit anonymous content you end up with every class having properties for minLabel and maxLabel unless we put those properties/setters/getters in the base class that does not have any anonymous content. That saves some code duplication but is in effect having the base class 'know' that its subclasses will have anonymous content named 'minLabel' and 'maxLabel'. 

gYear, gMonth, and gDay are the same in the sense that each has only one spinbutton but I don't think we want one single class for all of them with one spinbutton with a generic name like 'dateValue' - so they are all separate and are named accordingly - yearSpin, daySpin, monthSpin, etc.

Comment 18 aaronr 2007-03-19 10:44:31 PDT
(In reply to comment #17)
> (In reply to comment #15)
> > (From update of attachment 258772 [details] [diff] [review] [details])
> > >Index: resources/content/widgets-xul.xml
> > >===================================================================
> > Everything in range-gyear, range-gmonth and range-gday are pretty much the same
> > except for small differences in the anonymous content.  Or am I wrong?  Can't
> > you make one a base class and have the others extend it (they'd each have their
> > own anonymous content, of course)?  The base class really doesn't need to know
> > anything about year/month/day.  Just how to set labels, min, max and value
> > attributes, etc.  gYear would also need to override updateValue so that it
> > formats the number with 4 digits.
> There is a base class - gregorian - and the others do extend it. We could move
> updateLabels to the base class but that is about it.
> 
> The utility methods like 'getYear' could be removed if I use parseFloat
> directly in the multiple places it is used but I chose not do it that way.
> 
> Because you cannot inherit anonymous content you end up with every class having
> properties for minLabel and maxLabel unless we put those
> properties/setters/getters in the base class that does not have any anonymous
> content. That saves some code duplication but is in effect having the base
> class 'know' that its subclasses will have anonymous content named 'minLabel'
> and 'maxLabel'. 
> 

I guess that I can see that a base class probably shouldn't assume anything about anonymous content.

> gYear, gMonth, and gDay are the same in the sense that each has only one
> spinbutton but I don't think we want one single class for all of them with one
> spinbutton with a generic name like 'dateValue' - so they are all separate and
> are named accordingly - yearSpin, daySpin, monthSpin, etc.
> 

I'd go so far as to say that these have nothing to do with dates at all.  All of these classes do nothing more than manage a single spinbox and the properties, attributes, etc, that go along with it.  So instead of a spinbutton with a name like 'dateValue', I'd suggest it should be a name like 'spinValue'.  I could see  a custom controls author might like a spinbox widget like this to extend.  Kind of like a numeric range that isn't a slider.
Comment 19 Merle Sterling 2007-03-19 11:03:12 PDT
My point was that having a single class that manages one spinbutton (regardless of its name) may not be the right way to go. You still have a need for utility methods to parse the string values into floats, unless we just call parseFloat everywhere it is necessary.

So do we want to collapse gDay, gYear, gMonth into one single class and do away with having appropriately named utility methods like getYear, getMonth, etc?
Comment 20 aaronr 2007-03-19 11:39:29 PDT
(In reply to comment #19)
> My point was that having a single class that manages one spinbutton (regardless
> of its name) may not be the right way to go. You still have a need for utility
> methods to parse the string values into floats, unless we just call parseFloat
> everywhere it is necessary.

Having a method to parse strings into floats would be perfect for a numeric range spin-based widget.  So instead of calling it getDay, you could call it getNumericValue, makeFloat, or whatever you wish.  Then for each class you just need to override UpdateValue so that you format the value string to the proper length for day, month and year.  I personally think that is the way to go.  I don't mind losing the day/month/year specific variable names and method names because I'm basically getting a non-slider numeric range for free.

> 
> So do we want to collapse gDay, gYear, gMonth into one single class and do away
> with having appropriately named utility methods like getYear, getMonth, etc?
> 

Comment 21 Merle Sterling 2007-03-20 15:48:42 PDT
Created attachment 259148 [details] [diff] [review]
patch

New patch with single range-numeric-spin widget as base class for gDay, gMonth, and gYear because they are all relatively similar.
Comment 22 aaronr 2007-03-22 12:53:29 PDT
Comment on attachment 259148 [details] [diff] [review]
patch


You set the spinbox size for range-gyear to be 2 but the year spinbox for gYearMonth is set to 3.  Just use the one that looks better in both cases and use that value for both, please.  I'm just thinking if someone put a gyear next to a gYearMonth, it would look odd if the year fields weren't the same size.


>Index: resources/locale/en-US/xforms.dtd
>===================================================================

> 
>-<!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">
> 
> <!-- &#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.date.year.label        "&#34;Year&#34;">
>+<!ENTITY xforms.date.month.label       "&#34;Month&#34;">
>+<!ENTITY xforms.date.day.label         "&#34;Day&#34;">
> <!ENTITY xforms.range.start.separator "<![CDATA[&#34; >> &#34;]]>">
> <!ENTITY xforms.range.end.separator   "<![CDATA[&#34; << &#34;]]>">

I think exchanging "Year" for "&#34;Year&#34;", for example, will cause us problems.  You'll probably need both sets of values since one can be used with xul:label's @value, for instance, and the other should probably only be used in a field or some other JS block element.

with those fixed, r=me
Comment 23 Merle Sterling 2007-03-22 13:06:35 PDT
Created attachment 259349 [details] [diff] [review]
patch

Slight rework of names for labels and fields in the DTD. The ones for xul:label end in .label and the ones for xbl fields (which must be quoted) end .field.
Comment 24 alexander :surkov 2007-03-23 10:08:58 PDT
Comment on attachment 259349 [details] [diff] [review]
patch


>+      <property name="spinLabel">
>+        <getter>
>+          if (!this._spinLabel) {
>+            this._spinLabel = this.ownerDocument.
>+              getAnonymousElementByAttribute(this, "anonid", "spinLabel");
>+          }
>+          return this._spinLabel;
>+        </getter>
>+        <setter>
>+          this.spinLabel.value = val;
>+        </setter>
>+      </property>

It's wrong property must not return values of different types.
Comment 25 alexander :surkov 2007-03-23 10:22:28 PDT
Comment on attachment 259349 [details] [diff] [review]
patch

r=me with fixed spinLabel fixed issue.
Comment 26 Merle Sterling 2007-03-26 06:54:27 PDT
(In reply to comment #24)
> (From update of attachment 259349 [details] [diff] [review])
> >+      <property name="spinLabel">
> >+        <getter>
> >+          if (!this._spinLabel) {
> >+            this._spinLabel = this.ownerDocument.
> >+              getAnonymousElementByAttribute(this, "anonid", "spinLabel");
> >+          }
> >+          return this._spinLabel;
> >+        </getter>
> >+        <setter>
> >+          this.spinLabel.value = val;
> >+        </setter>
> >+      </property>
> It's wrong property must not return values of different types.

Please explain. Would you rather there be no setter and this.spinLabel.value = val be called directly from the method that needs to set the label?
Comment 27 alexander :surkov 2007-03-26 07:29:50 PDT
(In reply to comment #26)
> (In reply to comment #24)
> > (From update of attachment 259349 [details] [diff] [review] [details])
> > >+      <property name="spinLabel">
> > >+        <getter>
> > >+          if (!this._spinLabel) {
> > >+            this._spinLabel = this.ownerDocument.
> > >+              getAnonymousElementByAttribute(this, "anonid", "spinLabel");
> > >+          }
> > >+          return this._spinLabel;
> > >+        </getter>
> > >+        <setter>
> > >+          this.spinLabel.value = val;
> > >+        </setter>
> > >+      </property>
> > It's wrong property must not return values of different types.
> 
> Please explain. Would you rather there be no setter and this.spinLabel.value =
> val be called directly from the method that needs to set the label?
> 

Either
<getter>
  // ...
  return this._spinLabel.value;

or
readonly property 'spinLabel' that returns an element and this.spinLabel.value = "blabla";

It's up to you.
Comment 28 Merle Sterling 2007-03-26 08:19:25 PDT
Created attachment 259686 [details] [diff] [review]
patch for check-in
Comment 29 alexander :surkov 2007-03-27 07:29:50 PDT
checked in
Comment 30 aaronr 2007-04-23 16:38:48 PDT
checked into 1.8 branch on 2007-04-12
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.