386480 - View mode is not remembered after thunderbird restart

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Omar Bajraszewski]

Using lightning 0.7pre 2007063017 and thunderbird 2.0.0.4
When I switched to calendar mode I saw week view. I changed it to month view and then restarted thunderbird And after I switched to calendar mode I saw week view again

Comment 1

[Michael Büttner (no reviews TFN)]

Christian, do you want to preserve the last visible view across sessions? I personally think it's a valid request.

Comment 2

[Christian Jansen]

Makes sense for me, too.
- After restart the default should always be the mail mode
- In calendar mode the last selected view should be used.

Comment 3

[Sven Giermann]

(In reply to comment #2)
> - After restart the default should always be the mail mode

Maybe this is not, what the user wants - see Bug 359807.
I don't know, if it possible for Lightning to read the command-line options, but maybe someone knows how...

Comment 4

[Sven Giermann]

I agree with Stefan, to solve this one together with bug 386830.

Comment 5

[Worcester12345]

Is this a regression? I'd say so, but I can't totally recall right now. If so, it needs the key word.

Comment 6

[Stefan Sitter [:ssitter]]

(In reply to comment #5)
This is no regression.

Comment 7

[Sven Giermann]

Attachment: Make the last view persist and use it for choosing the initial view

I chose to attach this patch here rather than to Bug 386830, because the main part regards the last visted view.

This makes the checked state (introduced in Bug 376086) persistant and restores the global variable gLastShownCalendarView in ltnOnLoad().

Comment 8

[Worcester12345]

Any chance of getting this reviewed soon? It is hard to "sell" Lightning when this isn't working as expected. 

Comment 9

[Sven Giermann]

Comment on attachment 274774 [details] [diff] [review]
Make the last view persist and use it for choosing the initial view

Moving review to Philipp to get some progress...

Comment 10

[Stefan Sitter [:ssitter]]

Comment on attachment 274774 [details] [diff] [review]
Make the last view persist and use it for choosing the initial view

I'd recommend to 

>+++ mozilla/calendar/lightning/content/messenger-overlay-sidebar.js
>@@ -385,16 +369,29 @@ function ltnOnLoad(event)

>+    var availableViews = getViewDeck();
>+    for (var i = 0; i < availableViews.childNodes.length; i++) {

read availableViews.childNodes.length only once and store it in a variable

>+      var view = availableViews.childNodes[i];
>+      var command = document.getElementById(view.id+"-command");
>+      if( command.hasAttribute("checked") )

check for valid command value too: if ( command && command.hasAttribute(...

>+        gLastShownCalendarView = view.id.substring(0, view.id.indexOf('-'));
>+    }

add curly brackets and leave the for loop via break if we found the view

Comment 11

[Sven Giermann]

Attachment: patch v2

Updated patch with Stefan's recommendations addressed.

Comment 12

[Martin Schröder [:mschroeder]]

Comment on attachment 281638 [details] [diff] [review]
patch v2

Shifting review to mickey.

Comment 13

[Michael Büttner (no reviews TFN)]

Comment on attachment 281638 [details] [diff] [review]
patch v2

>+    // find last shown calendar view (persist="checked")
>+    try {
>+        var availableViews = getViewDeck();
>+        for (var i = availableViews.childNodes.length - 1; i >= 0; i--) {
As Stefan already suggested it is a bit more efficient to read availableViews.childNodes.length only once and store it in a variable.

>+            var view = availableViews.childNodes[i];
>+            var command = document.getElementById(view.id+"-command");
>+            if ( command && command.hasAttribute("checked") ) {
Superfluous blank after around braces, and more important we probably should explicitly check if the attribute equals the value 'true'.

>+    } catch(ex) {}
I also suggest to remove the try/catch block.

>+    if( gLastShownCalendarView == null )
>+        gLastShownCalendarView = 'month';
Curly braces are missing, no blank after 'if', superfluous blanks around braces.

>     // show the last displayed type of calendar view
>-    if(gLastShownCalendarView == null)
>-      gLastShownCalendarView = 'week';
Missing blank after keyword, missing curly braces.

Since all these comments are just minor style nits and the patch is indeed working as advertised, I'm going to fix this before checking it in. r=mickey. Thanks for the patch, Sven. BTW, feel free to remind me from time to time that a patch you're waiting on is stuck in my review queue, I really didn't have this on my radar. Thanks again for being patient.

Comment 14

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 15

[Stefan Sitter [:ssitter]]

(In reply to comment #13)
> (From update of attachment 281638 [details] [diff] [review])
> >+    for (var i = availableViews.childNodes.length - 1; i >= 0; i--) {
> As Stefan already suggested it is a bit more efficient to read
> availableViews.childNodes.length only once and store it in a variable.

This was not the solution I had in mind during writting Comment #10 but the change as shown above reads availableViews.childNodes.length only once ;)

Comment 16

[Omar Bajraszewski]

Verified with lightning 2007092204