Last Comment Bug 413847 - Timezone preference changes require restart to take effect
: Timezone preference changes require restart to take effect
Status: RESOLVED FIXED
: regression
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 0.9
Assigned To: Philipp Kewisch [:Fallen]
:
Mentors:
Depends on: 409094 440126
Blocks: 454181
  Show dependency treegraph
 
Reported: 2008-01-24 08:27 PST by Omar Bajraszewski
Modified: 2009-11-11 09:42 PST (History)
4 users (show)
bugzilla: wanted‑calendar0.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix timezone caching (1.32 KB, patch)
2008-03-17 01:27 PDT, Philipp Kewisch [:Fallen]
dbo.moz: review-
Details | Diff | Splinter Review
Perf test (4.27 KB, application/javascript)
2008-04-17 05:19 PDT, Philipp Kewisch [:Fallen]
no flags Details
Use pref observers - v2 (2.93 KB, patch)
2008-04-17 08:11 PDT, Philipp Kewisch [:Fallen]
dbo.moz: review-
Details | Diff | Splinter Review
[checked in] Use timezone service - v1 (59.36 KB, patch)
2008-06-15 07:17 PDT, Philipp Kewisch [:Fallen]
dbo.moz: review+
Details | Diff | Splinter Review

Description Omar Bajraszewski 2008-01-24 08:27:15 PST
Steps to reproduce:
1)Create a new profile and install Lt 2008012403

After this step guessed timezone was Europe/Belgrade and should be Europe/Warsaw

2)Change timezone to Europe/Warsaw
3)Open 'New event' dialog and enable Timezone option

Result:
Europe/Belgrade is displayed
Comment 1 Omar Bajraszewski 2008-01-24 08:29:41 PST
After restart correct timezone is displayed in New event dialog -->removing request for blocking
Comment 2 :Hb 2008-02-02 08:59:00 PST
Same issue with Sunbird  0.8pre 20080128 and Sunbird 0.7. A restart is needed to recognize the changed timezone setting. A message like "restart is needed to make changes work" would be fine.
Comment 3 Stefan Sitter 2008-02-02 09:08:58 PST
Confirmed as regression. Using Sunbird 0.7 the new timezone is immediately used when creating new events. Using Sunbird 0.8pre an application restart is required first.
Comment 4 Lars Wohlfahrt (thetux) 2008-02-07 10:04:23 PST
Confirmed, restart is required

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080206 Calendar/0.8pre 2008020618
Comment 5 Simon Paquet [:sipaq] 2008-02-08 04:58:49 PST
This does not block the 0.8 release.
Comment 6 Philipp Kewisch [:Fallen] 2008-03-17 01:27:36 PDT
Created attachment 309886 [details] [diff] [review]
Fix timezone caching

This fixes by reguessing the timezone if the pref doesn't match the tzid of the cached timezone.
Comment 7 Daniel Boelzle [:dbo] 2008-03-17 02:42:24 PDT
Comment on attachment 309886 [details] [diff] [review]
Fix timezone caching

> function calendarDefaultTimezone() {
>-    if (calendarDefaultTimezone.mTz === undefined) {
>-        var prefTzid = getPrefSafe("calendar.timezone.local", null);
>+    var prefTzid = getPrefSafe("calendar.timezone.local", null);
>+    if (calendarDefaultTimezone.mTz === undefined ||
>+        calendarDefaultTimezone.mTz.tzid != prefTzid) {
Since calendarDefaultTimezone() is called every so often I cannot foresee whether this potentially hurts performance (assuming reading out the pref is costly). Potential alternative: installing a pref observer.
Comment 8 Philipp Kewisch [:Fallen] 2008-03-18 06:36:07 PDT
I think the performance difference for one extra check is neglectable. The .tzid is stored directly in the timezone, so its just a matter of an extra string check (which is admittedly accross XPCOM, but I think its still ok).

I might be able to profile this in a while.
Comment 9 Daniel Boelzle [:dbo] 2008-04-07 06:57:22 PDT
I was more worrying about getting the pref value over and over again. Could you please make sure there's no performance regression?
Comment 10 Philipp Kewisch [:Fallen] 2008-04-17 05:19:48 PDT
Created attachment 316210 [details]
Perf test

I used this file and the error console to test. Performance decrease about by one decimal power.

Average: was 0.004 now 0.04
Average: was 0.005 now 0.042
Comment 11 Philipp Kewisch [:Fallen] 2008-04-17 08:11:06 PDT
Created attachment 316232 [details] [diff] [review]
Use pref observers - v2

This version uses a pref observer. Not quite sure if all that I'm doing here is needed, especially w.r.t to the observers.

Daniel, can you take a look to see if I am calling the correct add/remove observers?
Comment 12 Daniel Boelzle [:dbo] 2008-04-17 08:53:09 PDT
drive by comments:
- I think you should check if the branch observer has already been registered; if it is fired (pref-change), it would get added again.
- Somehow provocative: Is it really necessary to remove the observer on shutdown? I know it's clean to do so, but it bloats the code for little purpose: the process is going down anyway...
Comment 13 :Hb 2008-04-18 02:31:49 PDT
General speed of the application is much more worth than a saved dialog box like "Restart Sunbird to make timezone changes work" after a step most user will never do. 

If "Guess systems timezone" was successful most users won't change the timezone setting. This bug might be a regression, but is it really needed to do this by code?
Comment 14 Daniel Boelzle [:dbo] 2008-04-20 08:18:46 PDT
Comment on attachment 309886 [details] [diff] [review]
Fix timezone caching

I think we should consider the pref observer solution. This won't affect performance in a negative manner IMHO.
Comment 15 Daniel Boelzle [:dbo] 2008-04-21 07:45:45 PDT
Comment on attachment 316232 [details] [diff] [review]
Use pref observers - v2

I think you need to check if the branch observer has already been registered;
if it is fired (pref-change), it would get added again next time.
Comment 16 Philipp Kewisch [:Fallen] 2008-04-27 08:26:17 PDT
This function looks more and more like a component. The different contexts and such, make me think calendarDefaultTimezone should go into a component, like calITimezoneService. Unfortunately, the implementation languages of the component and guessSystemTimezone don't match, and porting guessSystemTimezone to C++ isn't an easy task. Doing so the other way around might work out.

I won't be taking care of that step though, sorry.
Comment 17 Philipp Kewisch [:Fallen] 2008-06-15 07:17:34 PDT
Created attachment 325166 [details] [diff] [review]
[checked in] Use timezone service - v1

This patch fixes half of the issue by moving the timezone guessing stuff into calITimezoneService.

Unfortunately there is another issue with the views: Setting the timezone on a view doesn't let it propagate through all dates in the view. After multiple tries with different approaches, I refrained from fixing that part, I found my changes were way to massive and given the easy workaround its not worth rewriting that much view code just to fix this issue.

Berend, you recently looked into the view code, maybe its easier for you to find a solution?
Comment 18 Daniel Boelzle [:dbo] 2008-06-18 02:20:01 PDT
Comment on attachment 325166 [details] [diff] [review]
[checked in] Use timezone service - v1

> interface calITimezoneService : calITimezoneProvider
> {
>     readonly attribute calITimezone floating;
>     readonly attribute calITimezone UTC;
>+
>+    readonly attribute calITimezone defaultTimezone;
> };

add some documentation, e.g. to which pref the attribute related.

>Index: calendar/base/src/calTimezoneService.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calTimezoneService.js,v
>retrieving revision 1.1.2.1
>diff -u -U 8 -p -r1.1.2.1 calTimezoneService.js
>--- calendar/base/src/calTimezoneService.js	5 Jun 2008 15:40:18 -0000	1.1.2.1
>+++ calendar/base/src/calTimezoneService.js	15 Jun 2008 14:12:51 -0000
>@@ -15,16 +15,17 @@
>  *
>  * The Initial Developer of the Original Code is
>  *   Sun Microsystems, Inc.
>  * Portions created by the Initial Developer are Copyright (C) 2008
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Daniel Boelzle <daniel.boelzle@sun.com>
>+ *   Philipp Kewisch <mozilla@kewis.ch>
add gekachecka

>+    const periodCalRule =
>+        Components.classes["@mozilla.org/calendar/recurrence-rule;1"]
>+                  .createInstance(Components.interfaces.calIRecurrenceRule);
can use createRecurrenceRule() here

r=dbo
Comment 19 Daniel Boelzle [:dbo] 2008-06-18 02:20:55 PDT
(In reply to comment #17)
> Unfortunately there is another issue with the views: Setting the timezone on a
> view doesn't let it propagate through all dates in the view. After multiple
> tries with different approaches, I refrained from fixing that part, I found my
> changes were way to massive and given the easy workaround its not worth
> rewriting that much view code just to fix this issue.
So let's keep this bug open then.
Comment 20 Philipp Kewisch [:Fallen] 2008-06-18 02:40:30 PDT
Comment on attachment 325166 [details] [diff] [review]
[checked in] Use timezone service - v1

Checked in on HEAD and MOZILLA_1_8_BRANCH

bug stays open
Comment 21 Philipp Kewisch [:Fallen] 2008-06-18 02:41:18 PDT
> >  * Contributor(s):
> >  *   Daniel Boelzle <daniel.boelzle@sun.com>
> >+ *   Philipp Kewisch <mozilla@kewis.ch>
> add gekachecka
Of course, my bad, done before checkin. Sorry about that, gekachecka!
Comment 22 Philipp Kewisch [:Fallen] 2008-09-08 04:10:19 PDT
Work should be continued in bug 454181.

-> FIXED
Comment 23 Stefan Sitter 2008-09-13 04:40:51 PDT
Philipp, can you elaborate what exactly has been fixed here?

The issue as reported in comment #0 still exists using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008091218 Sunbird/0.9. I see the follow up bug for the views but nothing about the original issue.
Comment 24 Philipp Kewisch [:Fallen] 2009-08-09 05:58:27 PDT
In this bug, I made the timezone service observe changes to the pref, which caused the timezone to be kept. Users of the timezone service often keep their own copy of the timezone (i.e the view boxes), which doesn't update  until the boxes are thrown away.

You are right, it seems the new event dialog caches things somehow too. I'll add this to the mentioned bug.
Comment 25 Damian Szczepanik 2009-11-05 15:45:22 PST
reopened because reproduced with Lt build 20091105

timezone is changed for new events after restart, not before
Comment 26 Martin Schröder [:mschroeder] 2009-11-08 10:16:47 PST
(In reply to comment #25)
> reopened because reproduced with Lt build 20091105
> 
> timezone is changed for new events after restart, not before

Damian, have you reopened this bug report because of (the still to be fixed) bug 454181? If I'm right, just re-resolve this one. :) Welcome back, Damian!
Comment 27 Damian Szczepanik 2009-11-11 09:42:48 PST
You're right, Martin.

Restored previous status: fixed

Note You need to log in before you can comment on or make changes to this bug.