changing application timezone does not update times in active view.

RESOLVED FIXED in 6.5

Status

defect
P1
major
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: Taraman, Assigned: darktrojan)

Tracking

(Blocks 1 bug, {regression})

Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

Steps to reproduce:
1. set your Lightning timezone to Europe/Berlin via the preferences dialog
2. create an event at 12 o'clock noon
3. select "show timezones" in the event dialog.
4. change the event timezone to "America/Los Angeles"
5. Start Time changes to 3 AM
6. save the event, display stays at 12 o'clock, because App-Timezone is still "Europe/Berlin"
7. open preferences and change App-Timezone to "America/Los Angeles"
8. displayed time stays at 12.

Expected result:
Displayed time should read 3 AM.
The other views are correctly updated most of the time, but - to make things worse - not always.

Restarting the Program gives correct display again.

Possible regression of bug 396279
Looks like the Problem is that the defaultTimezone() function does not update the tzid if it is already initialized. [1]
This function is called if the calendar.timezone.local pref is changed [2]

[1]: http://mxr.mozilla.org/comm-central/source/calendar/base/src/calTimezoneService.js#299
[2]: http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-base-view.xml#640
Assignee: nobody → Mozilla
Status: NEW → ASSIGNED
Hmm, looking at it further, I found the observer for the pref-change in the TimezoneService.

Unfortunately this observer may be called AFTER the view-observers causing at least the active view to stay on the old timezone and most of the tines the other views too.

Is there a way to set precedence on observers?
I see two possible solutions:

1) A second pref.
The change of the first pref is only observed by the timezone-service, which then changes the second one whose change the other components observe. This should be the most processing economic way.
Downside of this is that the two prefs could confuse people who directly edit the preferences.

2) Directly access the preference from other observers.
Since the pref is not changed very often, we could change the observers of the views, so that they directly read the pref and not call defaultTimezone()

Philipp, what do you think?
Flags: needinfo?(philipp)
And just as I sent it, a third solution came to my mind:
The timezone observer could dispatch a custom event, the other observers listen to.
See also bug 454181, which is probably a duplicate now. There the original bug is linked where I implemented the pref observer, this is a known issue. I'd suggest going with your third option. An nsIObserverService event sounds logical because you can listen to it from everywhere. 

You could consider extending this by a DOM event that is fired on the document of every one of our windows, this way you could just add a <handler> element to the xbl bindings that need it when the timezone changes. I think we already to something similar for switching to accessible system colors.
Flags: needinfo?(philipp)
Duplicate of this bug: 454181
For the system colors, the observer in the startup code does not fire an event as far as I see, but sets an attribute on all open windows. [1]

Since the view-Timezone is an integral part of the views and we also have to think about the todayPane, I would prefer to have an event fired from the TimezoneService which is observed in the views.

What do you think?

[1]: http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-chrome-startup.js#125
(In reply to Markus Adrario [:Taraman] from comment #7)
> Since the view-Timezone is an integral part of the views and we also have to
> think about the todayPane, I would prefer to have an event fired from the
> TimezoneService which is observed in the views.
If you mean an nsIObserverService event, fine with me!
Yes I do.
If I read it correctly we already shipped many releases with this behavior. 
Why is it suddenly considered a release blocker?
When I set this as blocker, I wasn't aware, this problem was already known. So from my side, we could change that.

I have the observer working, but whatever I do, the active view is not updated.
What I tried:
this.refresh(), this.forceRefresh(), this.relayout(); this.goToDay()
I also tried to update the currentView directly from the timezone Observer.

Is there any special magic to update the current view?
Blocker is indeed a little too much. Resetting to major, feel free to lower if needed.
Severity: blocker → major
Posted patch WIP Patch 0.8 (obsolete) β€” β€” Splinter Review
I have a working notification / observer infrastructure now, which is basically working.

There are still some flaws:
1. When the timezone change changes the date, the event-box is not moved to the right day but stays in the same day in month & multiday view as well as in the today-pane.

2. in Month view, the outcome is somewhat strange. To reproduce try this:
- set timezone pref to America/Adak
- create an event at 06:00 in this TZ, duration 1H
- restart TB
- change timezone pref to Europe/Amsterdam
Outcome: The event jumps to the previous day and the arrow for an event starting on the previous day is shown.
- now jump one month forward and back again
Outcome: The event is shown correctly.
- further changes in TZ produce the correct result.

I think I'm just using the wrong method to refresh the view or forget to set some variable.
I hope you can give me a hint.
Attachment #8470987 - Flags: feedback?(philipp)
Comment on attachment 8470987 [details] [diff] [review]
WIP Patch 0.8

Review of attachment 8470987 [details] [diff] [review]:
-----------------------------------------------------------------

I think this may have to do with the day boxes all having their own date object and therefore also timezone reference. When the view is moved to a different month, all day boxes are recalculated. This happens through the moveView methods, which calls goToDay, which checks the view range:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-multiday-view.xml#2881

I suspect calling refresh() instead of refreshView() will fix this problem.
Attachment #8470987 - Flags: feedback?(philipp) → feedback+
The only working solution I found is:
    moveView(-1);
    moveView(1);

refresh() and forceRefresh() do nothing
relayout() makes all boxes disappear.

Another solution could be to have a function "updateItems()" which updates every occurrence of every box. I haven't tried if this works however.

Same Problem arises on the TodayPane.

The Unifinder is fine so far.

Both solutions can be slow if you have a lot of items displayed, but since I don't believe this is used too regularly, we can live with that.
Then I'd suggest to run the profiler to find out what moveView is calling and do the same. I bet its some combination of refresh/relayout etc. I wouldn't like to do the double moveView thing as its just a workaround.
Blocks: 984045
Duplicate of this bug: 1102049
Duplicate of this bug: 1213238
Duplicate of this bug: 1469255
Posted patch 1008735-timezone-changed-1.diff (obsolete) β€” β€” Splinter Review
I almost accidentally created the same patch. Must've been a good idea.

I've also fixed up the arrows which (wrongly) showed an event spanned multiple days, and fixed the today pane and calendar tab minimonths.
Assignee: Mozilla → geoff
Attachment #8470987 - Attachment is obsolete: true
Attachment #9002658 - Flags: review?(philipp)
Posted patch 1008735-timezone-changed-2.diff (obsolete) β€” β€” Splinter Review
I always forget something. This time it was running eslint. :(
Attachment #9002658 - Attachment is obsolete: true
Attachment #9002658 - Flags: review?(philipp)
Attachment #9002661 - Flags: review?(philipp)
Comment on attachment 9002661 [details] [diff] [review]
1008735-timezone-changed-2.diff

Review of attachment 9002661 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/calendar-views.js
@@ +741,5 @@
> +        minimonth.update(minimonth.value);
> +    }
> +};
> +Services.obs.addObserver(timezoneObserver, "defaultTimezoneChanged");
> +addEventListener("unload", () => {

Prefer to be more explicit here, i.e. window.addEventListener().
Attachment #9002661 - Flags: review?(philipp) → review+
With requested change.
Attachment #9002661 - Attachment is obsolete: true
Attachment #9003063 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b94f1e4b62e6
When changing timezones, wait until timezone service is ready before updating views. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.5
You need to log in before you can comment on or make changes to this bug.