386830 - Unify default view, if no calendar view was selected before

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Description

[Sven Giermann]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: 

Currently there are two different default views in 'messenger-overlay-sidebar.js': one in 'ltnMinimonthPick' (month) and the other in 'selectedCalendarPane' (week).

It would be best to unify this to get the same behaviour...


Reproducible: Always

Comment 1

[Sven Giermann]

Attachment: Static implementation to be always 'month'

Comment 2

[Sven Giermann]

Attachment: Implementation to read from preferences.

Obviously this one needs more checks, whether the read pref matches any view and of course an UI to select ones default view...

Comment 3

[Michael Büttner (no reviews TFN)]

Comment on attachment 270868 [details] [diff] [review]
Static implementation to be always 'month'

I find this is a good idea, but I would like to hear Christians opinion on this. Furthermore, we need to clarify how this integrates with the upcoming today pane.

Comment 4

[Stefan Sitter [:ssitter]]

I think it makes sense to solve this together with Bug 386480 -> Switching to a different view could update the preference so that the same view is shown after restart.

Comment 5

[Michael Büttner (no reviews TFN)]

Comment on attachment 270868 [details] [diff] [review]
Static implementation to be always 'month'

>     if (document.getElementById("displayDeck").selectedPanel != 
>         document.getElementById("calendar-view-box")) {
>         var view = currentView();
> 
>         // If we've never shown the view before, we need to do some special
>         // things here.
>         if (!view.initialized) {
>-            showCalendarView('month');
>+            showCalendarView();
>             view = currentView();
>             cdt.timezone = view.timezone;
>             view.goToDay(cdt);
>             return;
>         }
Bug 371916 made this whole block of code obsolete, since the minimonth is no longer displayed in mail mode. Thus the initial if-statement is never true, so the whole block of code won't ever execute, therefore can be removed.

> function showCalendarView(type)
> {
>+    if (type == null) {
>+        type = 'month';
>+    }
>+
I simply would return if no type is set. Bug 376086 introduces the checked state to the appropriate commands and therefore is already in charge of defining the default view. I'd simply say that no passing a correct argument to this function is an error, but doesn't need a default here, neither per pref nor in any other way.

Comment 6

[Sven Giermann]

Mickey,

before deleting (currently) unneeded code, please remember the planned "Today pane". I was informed, that another MiniMonth is planned for this - and it could be displayed in mail mode...

Regarding default view: I agree, that my suggestion is not perfect and is more seen as a draft.
But in case a default view is being defined somewhere, it should be ensured, that every function uses this view when switching from mail to calendar mode.
This is why I suggested to just give no argument and let the function decide, which view to choose.

Sven.

Comment 7

[Michael Büttner (no reviews TFN)]

(In reply to comment #6)
> before deleting (currently) unneeded code, please remember the planned "Today
> pane". I was informed, that another MiniMonth is planned for this - and it
> could be displayed in mail mode...
I'm well aware of where the implementation of the "Today Pane" is driving us. Unless Christian did change anything, you can find the specification at [1]. So,  from what I can see now it's obsolete code.

> But in case a default view is being defined somewhere, it should be ensured,
> that every function uses this view when switching from mail to calendar mode.
> This is why I suggested to just give no argument and let the function decide,
> which view to choose.
Exactly, and I just think that the function that handles the view switching is the only that also should take care of the default value. But you're absolutely right, that it should handle wrong arguments. I just said that I think it's more of an error if somebody passes a wrong argument to your function.

[1] http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views

Comment 8

[Sven Giermann]

Using the checked state introduced by Bug 376086, I will attach the appropriate patch rather to Bug 386480, because the main task is to reveal the last selected state.

Comment 9

[Stefan Sitter [:ssitter]]

(In reply to comment #8)
Sven, does the patches in this bug still need review? Otherwise you should remove the review request and mark the patches as obsolete.

Comment 10

[Sven Giermann]

Resolved with bug 386480.