Closed Bug 304541 Opened 14 years ago Closed 14 years ago

Timezone picker does nothing.

Categories

(Calendar :: Sunbird Only, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: jminta)

Details

Attachments

(2 files, 1 obsolete file)

The timezone picker is brutally honest, telling users that it does nothing. 
Since we actually do look for the calendar.timezone.local pref when guessing the
TZ the user is in, we ought to at least make it possible for them to set it.  A
better way would be the graphical timezone picker in bug 302253.  There are some
concerns about the licensing for the map used there, though.

This bug is simply to make the timezone picker semi-useful for 0.3a1.  Patch to
follow.
This enables the timezone picker, removing the very long menu-list and instead
dynamically creating it.  This makes it l10n-unfriendly, though.  Should we be
concerned with that?

Also fixes the whitespace, which was done with tabs instead of spaces.
Assignee: mostafah → jminta
Status: NEW → ASSIGNED
Attachment #192596 - Flags: first-review?(mvl)
(In reply to comment #1)
> This enables the timezone picker, removing the very long menu-list and instead
> dynamically creating it.  This makes it l10n-unfriendly, though.  Should we be
> concerned with that?

Yes, especially when it is a regression. I think it would be better to just
enable the picker in this bug, and file new bugs on improving it.
A possible solution would be to generate the picker, but keep the translations,
and check if there is a translation for a zone. If not, display english.

> Also fixes the whitespace, which was done with tabs instead of spaces.

That's always a good think to do :)
Comment on attachment 192596 [details] [diff] [review]
enable timezone picker

r-, based on the localization regression.

(Another idea on a better l10n story: Split region from city, so you only need
to translate 'Europe' once)
Attachment #192596 - Flags: first-review?(mvl) → first-review-
Seems like this patch here would also get rid of calendar.timezone.default in timezonePrefs.xul. There is another instance of that in resources/content/pref/rootCalendarPref.js. (I guess that both of them are obsolete.)
(In reply to comment #4)
> Seems like this patch here would also get rid of calendar.timezone.default in
> timezonePrefs.xul. There is another instance of that in
> resources/content/pref/rootCalendarPref.js. (I guess that both of them are
> obsolete.)
> 

Yes,calendar.timezone.default is incorrect. calendar.timezone.local is the proper name.
Attached patch enable picker (obsolete) — Splinter Review
This patch enables the timezone picker. The big part is s/value="/value="/mozilla.org/20050126_1/"/ to give the preference the right value.
A few lines of code were added to set the default to the guessed timezone. It also changes the name of the pref to be set.
I used some shell scripting to verify that the list of timezones in the picker mathces the timezones in zones.tab. I found one error, Europe.Vienna instead of Europe/Vienna. Fixed that.
Attachment #218857 - Flags: first-review?(jminta)
Comment on attachment 218857 [details] [diff] [review]
enable picker

OK, I'll bite for 0.3a2, as long as we get the new, better prefs in ASAP.

My biggest concern here is that simply showing the panel will set the guessed TZ to be the pref.  Ideally, I think I'd like some checking to make sure that the user really set it.  Any thoughts here?  If you're ok with that, then I am.
+	onload="parent.initPanel('chrome://calendar/content/pref/timezonePrefs.xul');setGuessedTimezone();"
Can you stick a space before setGuessedTimezone?

+function setGuessedTimezone() {
+    var elem = document.getElementById("defaultzone");
+    if (!elem.value) {
+        elem.value = calendarDefaultTimezone();
+    }
+}
+
  </script>
Indenting, and CDATA tags.

r=jminta with the guessing->pref question answered and the above nits.
Attachment #218857 - Flags: first-review?(jminta) → first-review+
Attached patch updated patchSplinter Review
I added some tricks with prefstring and onchange, to not save the default value. It will actually get saved, but to an unused pref. Not 100% clean, but the best I could do without spending too much time, given that this code will go away soon.
Attachment #218857 - Attachment is obsolete: true
Attachment #219048 - Flags: first-review?(jminta)
Comment on attachment 219048 [details] [diff] [review]
updated patch

This looks much better. r=jminta
Attachment #219048 - Flags: first-review?(jminta) → first-review+
patch checked in
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.