Support gregorian calendar types for Range

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
a year 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

(11 attachments, 5 obsolete attachments)

(Assignee)

Description

11 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 gregorian calendar types for range: gYearMonth, gYear, gMonthDay, gDay, gMonth.

Reproducible: Always
(Assignee)

Updated

11 years ago
Blocks: 316353

Comment 1

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

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 years ago
Created attachment 257420 [details]
testcase: gDay
(Assignee)

Comment 3

11 years ago
Created attachment 257421 [details]
testcase: gMonth
(Assignee)

Comment 4

11 years ago
Created attachment 257422 [details]
testcase: gMonthDay
(Assignee)

Comment 5

11 years ago
Created attachment 257423 [details]
testcase: gYear
(Assignee)

Comment 6

11 years ago
Created attachment 257424 [details]
testcase: gYearMonth
(Assignee)

Comment 7

11 years ago
Created attachment 257431 [details]
testcase: gDay (XUL)
(Assignee)

Comment 8

11 years ago
Created attachment 257432 [details]
testcase: gMonth (XUL)
(Assignee)

Comment 9

11 years ago
Created attachment 257433 [details]
testcase: gMonthDay (XUL)
(Assignee)

Comment 10

11 years ago
Created attachment 257434 [details]
testcase: gYear (XUL)
(Assignee)

Comment 11

11 years ago
Created attachment 257435 [details]
testcase: gYearMonth (XUL)
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 12

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

Updated

11 years ago
Attachment #257576 - Flags: review?(surkov.alexander)
(Assignee)

Updated

11 years ago
Depends on: 372736
(Assignee)

Comment 13

11 years ago
Created attachment 257729 [details] [diff] [review]
patch

Updating patch based on current version of patch 372736.
Attachment #257576 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #257729 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Updated

11 years ago
Attachment #258772 - Flags: review?(aaronr)

Comment 15

11 years ago
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.
Attachment #258772 - Flags: review?(aaronr)

Comment 16

11 years ago
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 ;)
Attachment #258772 - Flags: review?(surkov.alexander)
(Assignee)

Comment 17

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

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

Comment 19

11 years ago
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

11 years ago
(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?
> 

(Assignee)

Comment 21

11 years ago
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.
Attachment #258772 - Attachment is obsolete: true
Attachment #259148 - Flags: review?(aaronr)

Comment 22

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

Comment 23

11 years ago
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.
Attachment #259148 - Attachment is obsolete: true
Attachment #259349 - Flags: review?(surkov.alexander)

Comment 24

11 years ago
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

11 years ago
Comment on attachment 259349 [details] [diff] [review]
patch

r=me with fixed spinLabel fixed issue.
Attachment #259349 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 26

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

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

Comment 28

11 years ago
Created attachment 259686 [details] [diff] [review]
patch for check-in
Attachment #259349 - Attachment is obsolete: true

Comment 29

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

Comment 30

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