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

attach testcases, please

Updating patch based on current version of patch 372736.

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

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

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

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?

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

New patch with single range-numeric-spin widget as base class for gDay, gMonth, and gYear because they are all relatively similar.

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

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 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 on attachment 259349 [details] [diff] [review] patch r=me with fixed spinLabel fixed issue.

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

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

checked in

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