Closed Bug 260120 Opened 19 years ago Closed 17 years ago
improvements to datepicker and timepicker
datepicker (and timepicker) should not use mDisabled, but just the disabled attribute Instead of checking for keycode 13, they could use the destructor to check the input.
but what used to work appears to work now but with no dependences on dateUtils, no localization support needed, no "evals", disabled has been cleaned up, and quite a bit of other little things thrown in here and there as well as standardize the naming and spacing conventions. it moved over to toolkit pretty smoothly too and worked in unprivileged xul code over there.
you need shaver's toLocaleFormat patch for the attached patch to work.
Depends on: 291494
This patch looks similar to bug 92174 comment 40 and afterward, though I see the css files have been consolidated. I assume the toLocalFormat patch you mentioned is from bug 291494 ? Looks like date format is localized but date parsing is not localized, so the date parser won't work in most locales. preventBubble method seems superfluous. (I think this bug was meant as a bug for miscellaneous tiny fixes, not major patches.) (This patch addresses several issues. Was the intent just to save a checkpoint "in preparation", or to generate comment and review? Generally, incremental bugs and patches are preferred, so it is easier to test them and later back out individual changes if it turns out one of the changes introduces a bug somewhere else.)
This method seems superflouous: why not call preventBubble directly on the event instead of calling this method? (Maybe it was for testing, so you could turn off all the preventBubble's at once?) + <method name="preventBubble"> + <parameter name="anEvent"/> + <body> + <![CDATA[ + // avoid calling popup handlers of higher popups in DOM (datepicker) + anEvent.preventBubble(); + ]]> + </body> + </method>
This patch uses Date.parse + <method name="parseTextBoxDate"> + <parameter name="aRefresh"/> + <body> + <![CDATA[ + var parsed = Date.parse(this.kTextBox.value) Date parse appears to be defined here: http://lxr.mozilla.org/seamonkey/source/js/src/jsdate.c#741 It calls date_parseString, defined here: http://lxr.mozilla.org/seamonkey/source/js/src/jsdate.c#552 It is not localized, for example it looks for only the english strings defined here http://lxr.mozilla.org/seamonkey/source/js/src/jsdate.c#440 The current code uses DateFormater (french spelling?) http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/datetimepickers/datepicker.xbl#230 The DateFormater constructor probes the current date format to find the order of year month day and stores indexes. http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/dateUtils.js#156 DateFormater.parseShortDate uses the stored indexes to parse the date. http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/dateUtils.js#467 Does that help?
When is the estimated deadline?
gekacheka - thanks for clarifying.. and sorry for being a bit dense. I've fixed the date parsing issue (using dateUtil's code) and made the change for preventBubble. I also did a bit of refactoring to move the date/time parsing/formatting and common functionality into a datetimepicker-base. I unfortunately don't have a specific time frame for 1.8b2 yet.
hopefully this will help with review - timepicker in particular was heavily modified, so I'm not sure how much good it will do, but I know this is a big patch and it's hard to grasp what all changed. if there's anything else I can do to help make it easier, please let me know. Thanks!
Files need directory names. Several datetimepickers.css are defined only as --- /dev/null 2005-04-29 14:10:13.116000000 -0400 +++ datetimepickers.css 2005-04-29 13:50:39.558739200 -0400 So the later versions overwrite the earlier ones in the patch, and the patch does not put any of them in the correct place for testing.
Sorry. This is the same file but with the paths added. I justed tested this against a clean tree. It should resolve the issue.
A solution that continues to use the Mozilla ScriptableDateFormat service would be better for users. * The patch with toLocaleFormat uses strftime "%x" for date, which may produce long format dates in many locales. strftime just enables two formats: %x and an optional alternate %Ex. It does not specify if one format is long or short. Before Calendar was updated to use the scriptable date format service, this was a source of many complaints from windows users, along the lines of "it should just use my operating system settings like other programs". On windows, the user has two customizable settings for date, a long format and short format. For the date picker, the current code uses the short format so it takes up little space (no long spelled out words to type), typically "4/3/05" in the US, "3/4/05" in many european countries, and "2005-04-03" (ISO) in some other places, but it is user-settable independent of locale. (The long format typically spells out the month name and the weekday, so it is typically much larger than fits in a small field, such as "Wednesday, September 29, 2005".) Mozilla ScriptableDateFormat provides dateFormatShort that the current code uses. It makes clear that a short format date is intended. * The patch hardcodes a time format. Many locales use 24 hour time, and time formats often have no spaces. + <method name="formatTime"> + <parameter name="aValue"/> + <body> + <![CDATA[ + // get H:M AM/PM - we need to strip the leading 0 out of %I + return aValue.toLocaleFormat("%I:%M %p").replace(/^0/, ""); Instead it should use the operating system time format. The current code does so via the ScriptableDateFormat service.
Issues with ScriptableDateFormat: The reason we moved away from scriptabledateformat for this patch is because of issues with using it from unprivileged code. This is, of course, not an issue with calendar specifically, however in toolkit (and in particular if this code is ever used for XForms/WebForms 2), it's a big deal. Formatting Date: I understand the concern about what would happen on a locale that used a long date format as their preference, however I don't think we should ignore that setting either. Also note: unix uses the same code inside scriptabledateformat for short dates. See http://lxr.mozilla.org/seamonkey/source/intl/locale/src/unix/nsDateTimeFormatUnix.cpp#202 Formatting Time: on unix, strftime is used by the scriptabledateformat to generate the locale specific time using strftime in a similar fashion to what I did. See http://lxr.mozilla.org/seamonkey/source/intl/locale/src/unix/nsDateTimeFormatUnix.cpp#226 The only difference is that it first checks locale for 24 hour and if AM/PM should be first. Would an acceptable solution be to "probe" the locale format using toLocaleFormat("%X") to determine if 24H notation should be used and the position of AM/PM so that the output matches scriptabledateformat on unix? if that sounds like a plan, let me know and I'll implement it shortly. If there's anything else that needs to be updated while I'm at it, just let me know. Thanks!
(In reply to comment #14) > Probe idea sounds ok. Suggest that it also omit space in locales that do > not use spaces. I'll see what I can do on the spaces. If it's a common thing, we may want to submit a bug on scriptabledateformat > (I don't understand why using the date format service requires privileges.) I can probe to see if the scriptabledateformat is available and use it instead of toLocaleFormat if it is, but that means we could end up with different behavior between privileged and unprivileged code on some platforms. It shouldn't be much overhead to the code. > XForms is currently supplied as an extension, > but Calendar is also supplied as an extension.) XForms itself isn't the issue, it's the xforms documents that are unprivileged. > Other issues: > > * skin/datetimepickers/datetimepickers.css doesn't seem to be loaded. > (time picker grids have no shading or borders) I'll clean this up. <resource.. is the standard in /toolkit, so I'd prefer that be the only thing needed. It looks like it's just a path problem. > * the documentation could be restored for > - Requires: > <?xml-stylesheet type="text/css" > href="chrome://calendar/content/datetimepickers/datetimepickers.css" ?> I really didn't want this line. It'll only be required until it moves to toolkit as then it will be "defined" in xul.css. And I didn't want to have to deal with removing it when it went to toolkit. > * html looks inappropriate in datetimepickers.xml > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" > + "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> > + This is needed for the « and » entities. Are there alternatives? > * Minor: In timepicker, « and » for "more" and "less" were a bit > mysterious, particularly since they don't point the same direction picker > expands/contracts. Maybe ∧ (^) and ∨ (v) would be better > since they point in vertical directions. (Δ and ∇ are > also possible, though not as symmetrical in some fonts.) I think I'll check with our resident UI experts on IRC and go by their decision. I have no preference personally - just didn't want to have to deal with localizing less/more. > Thanks for the diff, it was helpful. The timebox shortening is nicer. > Thanks for the event cleanup. No problem. I'll get these changes out shortly. Thanks! -Andrew
(In reply to comment #14) > * Minor: In timepicker, « and » for "more" and "less" were a bit > mysterious, particularly since they don't point the same direction picker > expands/contracts. Maybe ∧ (^) and ∨ (v) would be better > since they point in vertical directions. (Δ and ∇ are > also possible, though not as symmetrical in some fonts.) The « and » symbols are fairly commonly used in UIs to indicate "expand " and "collapse", and also to indicate "see more options" (think of "advanced" controls in MS apps, for instance). I think that part of the problem is that they appear aligned to the bottom-left. It probably works better with them at the bottom-right: 1 2 3 4 5 6 7 8 9 10 11 12 :00 :15 :45 :55 >>
This should address all the outstanding issues (unless already addressed in the bug comments). I'm a bit too tired to try to work out the diff's for the individual files. I'll try to get to that soon if you all need it. The only thing that's different than planned is that I decided I didn't like trying to check for ScriptableDateFormat because of the inconsistency issues mentioned. If we find that this is error prone and needs improving.. I'd rather improve it than hack in scriptabledate only when it's available. Thanks!
TimeFormat: Use initTimeFormat probe code, remove probeDisplayFormat. It isn't as cross-cultural (e.g., no 'M' in many cultures that don't use latin alphabet). initTimeFormat should probe system format based on the new source of locale time format, toLocaleFormat("%X"), not based on formatTime. var amProbeString = amProbeTime.toLocaleString("%X"); ... var pmProbeString = pmProbeTime.toLocaleString("%X"); initTimeFormat should set an output format string for formatTime to use, so formatTime doesn't have to recompute format each time. Add something like this at the end of initTimeFormat: var ampmSep = (pmProbeString.indexOf(' ') != -1? " " : ""; this.kTimeFormatString = (this.ampmIndex == PRE_INDEX? "%p" + ampmSep + "%I:%M" : this.ampmIndex == POST_INDEX? "%I:%M" + ampmSep + "%p" : "%H:%M"); The formatTime method can be something like below to replace leading hour zero even if preceded by ampm. // remove leading zero in hours before colon in xx01:23xx return aValue.toLocaleFormat(this.kTimeFormatString).replace(/0(\d):/,"$1:"); CSS: Note: Calendar uses cyan/blue colors, but toolkit widgets default theme is gray-based or O.S. preference based (e.g., selection color), so color might need to be removed or change to gray when moved to toolkit so it matches other widgets.
I'd prefer to see this land ASAP and remaining localization/style issues worked out post-landing (assuming it doesn't break any current code), as this is a pretty big patch already -- I'm guessing due to the renaming of files, but still.
I agree we need to get it landed. I appreciate gekacheka's suggestion enough though that I went ahead and applied his code. The only "issue", and the only reason why I didn't do it that way initially was because it threw off my initialization a bit. But I like what I've got now, because of the slight reorg, a whole lot better than what I had before. Sorry for the delays in getting the suggestions into the patch. Thanks! -Andrew
Comment on attachment 182774 [details] [diff] [review] probe done in init 1. date picker doesn't work for me in calendar, it doesn't popup, and there is an error that this.kMinimonth.update is not defined. Problem seems to be that <minimonth/> is not expanded with its xbl definition. Looks like minimonth is not defined. It works if I add the following to the top of content/datetimepickers/datetimepickers.css: @import url("chrome://calendar/content/datetimepickers/minimonth.css"); 2. This version checks for initialization to the beginning of the parse and format methods. The previous version initialized in the constructor. The reason for this approach should be documented. (Previous version failed because <constructor> does not take a <body>, just put the code directly in <constructor>.) 3. Please add comment explaining need for kTimeFormatString, such as + + // build time display format that mimics "%x" format without seconds var ampmSep = (pmProbeString.indexOf(' ') != -1? " " : ""); 4. Please add comment explaining need to modify custom formatted time string: + // remove leading zero in hours before colon in xx01:23pmxx return aValue.toLocaleFormat(this.kTimeFormatString).replace(/0(\d):/,"$1:");
So close! Thanks!
(patch -l -p 2 -i file.patch) Patch newpickersall4a.patch added comment + Datetimepicker-base internally initializes + date/time formats so that descendents are not required to initialize the + formats they need in their constructor. So the if to call initDateFormat and initTimeFormat from the methods are there because the inits in the datetimepicker-base constructor weren't working? As mentioned earlier, that constructor had a <body> that shouldn't be there. In patch newpickersall3.patch, the formats were initialized in the datetimepickers-base constructor, which is simpler than the current code. If that constructor is fixed (no <body>, put code directly in <constructor>) then it appears there is no reason to call them from the subclass constructors, and no reason sprinkle the "if ... .initDateFormat()" and "if ... .initTimeFormat()" calls anywhere else. Property lastDateParseIncludedTime onget= doesn't need the if either. Suggest simplify as follows: * remove above comment * put initDateFormat and initTimeFormat calls in datetimepicker-base constructor (as in patch newpickersall3.patch, but without <body> tags) * remove "if ... .initDateFormat()" from parseDate * remove "if ... .initTimeFormat()" from parseTime * remove "if ... .initTimeFormat()" from formatTime * remove "if ... = false" from lastDateParseIncludedTime.onget
The reason the init was broken up between date and time was so a control that wanted to handle just one or the other didn't have to waste cycles init'ing and probing when it wasn't necessary. I realize the <body> tag was an issue, but I didn't really want to be calling init there anyway. A couple of if's is a lot less overhead the having to call init unnecessarily. I honestly don't have a strong preference but can we accept it as is?
Comment on attachment 182785 [details] [diff] [review] comments added and issue with css import addressed r=vladimir for checkin (with additional followup patch applied), please file followup bugs for any remaining problems
Attachment #182785 - Flags: first-review?(gekacheka) → first-review+
(In reply to comment #25) > (From update of attachment 182785 [details] [diff] [review] ) > r=vladimir for checkin (with additional followup patch applied), please file > followup bugs for any remaining problems > This should have been closed a long time ago. :-)
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.