Timezone preference changes require restart to take effect

RESOLVED FIXED in 0.9

Status

Calendar
Internal Components
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Omar Bajraszewski, Assigned: Fallen)

Tracking

(Depends on: 1 bug, {regression})

unspecified
regression
Dependency tree / graph
Bug Flags:
wanted-calendar0.9 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
Flags: blocking-calendar0.8?
(Reporter)

Comment 1

10 years ago
After restart correct timezone is displayed in New event dialog -->removing request for blocking
Flags: blocking-calendar0.8?

Comment 2

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

10 years ago
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.
Flags: blocking-calendar0.8?
Keywords: regression
Version: Mozilla 1.8 Branch → unspecified
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

Updated

10 years ago
Summary: Timezone guess doesn't work for 'New Event' dialog → Timezone preference changes require restart to take effect

Comment 5

10 years ago
This does not block the 0.8 release.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
(Assignee)

Comment 6

10 years ago
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #309886 - Flags: review?(daniel.boelzle)

Comment 7

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

Comment 8

10 years ago
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.

Updated

10 years ago
Flags: blocking-calendar0.8- → wanted-calendar0.9+

Comment 9

10 years ago
I was more worrying about getting the pref value over and over again. Could you please make sure there's no performance regression?
(Assignee)

Comment 10

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

Comment 11

10 years ago
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?
Attachment #316232 - Flags: review?(daniel.boelzle)
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

10 years ago
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?
OS: Windows XP → All
Hardware: PC → All
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.
Attachment #309886 - Flags: review?(daniel.boelzle) → review-
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.
Attachment #316232 - Flags: review?(daniel.boelzle) → review-
(Assignee)

Comment 16

9 years ago
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.
Assignee: philipp → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 17

9 years ago
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?
Attachment #309886 - Attachment is obsolete: true
Attachment #316232 - Attachment is obsolete: true
Attachment #325166 - Flags: review?(daniel.boelzle)

Updated

9 years ago
Depends on: 409094
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
Attachment #325166 - Flags: review?(daniel.boelzle) → review+
(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.
(Assignee)

Comment 20

9 years ago
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
Attachment #325166 - Attachment description: Use timezone service - v1 → [checked in] Use timezone service - v1
(Assignee)

Comment 21

9 years ago
> >  * 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!
Target Milestone: --- → 0.9

Updated

9 years ago
Depends on: 440126
(Assignee)

Updated

9 years ago
Blocks: 454181
(Assignee)

Comment 22

9 years ago
Work should be continued in bug 454181.

-> FIXED
Assignee: nobody → philipp
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 23

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

Comment 24

8 years ago
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.
Status: RESOLVED → VERIFIED

Comment 25

8 years ago
reopened because reproduced with Lt build 20091105

timezone is changed for new events after restart, not before
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(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

8 years ago
You're right, Martin.

Restored previous status: fixed
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.