462423 - allow changing and choosing mail start page even when it is not set to show at the start

Status: VERIFIED FIXED

(Thunderbird :: Preferences, enhancement)

Description

[Przemyslaw Bialik]

Why ?
Because this way it is way more useful and it could be used in a similar way to the Firefox homepage / bookmark.

Comment 1

[Przemyslaw Bialik]

Attachment: Patch v1

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 345559 [details] [diff] [review]
Patch v1

>+    

Spurious whitespace ^^^

>+    var startpageenabled = pref.getBoolPref("mailnews.start_page.enabled");
>+    if (gDisplayStartupPage && startpageenabled)
>     {
>       loadStartPage();
>       gDisplayStartupPage = false;
>-      UpdateMailToolbar("gDisplayStartupPage");
>-    }  
>+    } 

Spurious whitespace ^^^

... but more importantly, there is no "pref" defined in the above code context.
The exception is happily swallowed somewhere though... :/

And there is no need to call UpdateMailToolbar in case the start page isn't getting loaded.

Comment 3

[Przemyslaw Bialik]

(In reply to comment #2)
> ... but more importantly, there is no "pref" defined in the above code context.

>+    var startpageenabled = pref.getBoolPref("mailnews.start_page.enabled");
?

> The exception is happily swallowed somewhere though... :/
you want exception for the above pref ? , loadstartpage() function is catching exception for loading the page
 
> And there is no need to call UpdateMailToolbar in case the start page isn't
> getting loaded.
I had too move it out because with option to check for mail on startup and with disabled startpage,  getmail button was disabled (until I clicked somewhere)
now I've noticed that it happens only with Lightning extension enabled, it's not a big problem in this case but my patch did it and I would like to fix it but unfortunately I don't know why it happens :|

Comment 4

[Przemyslaw Bialik]

> > And there is no need to call UpdateMailToolbar in case the start page isn't
> > getting loaded.
> I had too move it out because with option to check for mail on startup and with
> disabled startpage,  getmail button was disabled (until I clicked somewhere)
> now I've noticed that it happens only with Lightning extension enabled, it's
> not a big problem in this case but my patch did it and I would like to fix it
> but unfortunately I don't know why it happens :|
previously loadStartPage() was executed at every startup which calls at the end ClearMessageSelection(), with my patch loadStartPage() called only when there is a need to
so should I leave UpdateMailToolbar where it is, use ClearMessageSelection() instead or you have other idea to solve disabled getmail for Lightning ?

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to comment #3)
What I meant was, all pref.getBoolPref will ever do there is cause an exception, since pref isn't defined. 

And since you'll always get the exception, the i can't see how the UpdateMailToolbar call would ever get reached in the first place...

Comment 6

[Przemyslaw Bialik]

(In reply to comment #5)
> What I meant was, all pref.getBoolPref will ever do there is cause an
> exception, since pref isn't defined.
hmm I think we have some misunderstanding here, I'm not using new pref because there is no need to, I'm using existing pref to check if user wants to show start page on startup or not
as you see from the patch pref.getboolpref was moved from mailWindow.js/loadStartPage() to commandglue.js/FolderPaneSelectionChange()
sorry, maybe I should have commented more but I hope all is clear now ?

I've tested the patch in a various ways and all works well in the practice
 
> And since you'll always get the exception, the i can't see how the
> UpdateMailToolbar call would ever get reached in the first place...

Comment 7

[Magnus Melin [:mkmelin]]

Yes, I know you used the existing pref name, what i'm saying is the "pref" in pref.getBoolPref isn't defined inside the function you are now using it in.

Comment 8

[Przemyslaw Bialik]

OK, sorry...
so adding
var pref = Components.classes["@mozilla.org/preferences-service;1"]
    .getService(Components.interfaces.nsIPrefBranch2);
before var startpageenabled should be enough ?
I would notice it if this would not work but it did :|
Regarding UpdateMailToolbar - should I leave it where it was originally and file bug for Lightning or fix it now as proposed in the current patch ?

Comment 9

[Magnus Melin [:mkmelin]]

Looks like gPrefBranch is defined, so use that.
Re UpdateMailToolbar I doubt it's a problem when the code is actually working ;)
But if it is, no we shouldn't say startpage was loaded if it wasn't. If it's needed just UpdateMailToolbar("FolderPaneSelectionChange"); or something

Comment 10

[Przemyslaw Bialik]

Attachment: Patch v2

Updated to comments, I hope ;)

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 345957 [details] [diff] [review]
Patch v2

Since we're always going to do UpdateMailToolbar("FolderPaneSelectionChange"); at the end, you can remove the two previous UpdateMailToolbar calls... Updating it once should be enough.

Also, please wrap the GetMessagePaneFrame().location.href line at ~80 chars.

BTW, I see now pref was indeed defined (through mailWindow.js i suppose), though using gPrefBranch is still the way to go in this file.

Comment 12

[Przemyslaw Bialik]

> Since we're always going to do UpdateMailToolbar("FolderPaneSelectionChange");
> at the end, you can remove the two previous UpdateMailToolbar calls... Updating
> it once should be enough.
yeah, should be enough
 
> Also, please wrap the GetMessagePaneFrame().location.href line at ~80 chars.
OK, if it would be not the best wrap feel free to fix it during checkin

> BTW, I see now pref was indeed defined (through mailWindow.js i suppose),
> though using gPrefBranch is still the way to go in this file.
so if I understand you properly there is no need to redefine if it's working (as in the last patch)

patch coming

Comment 13

[Przemyslaw Bialik]

Attachment: Patch v3

Updated to comments

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 346089 [details] [diff] [review]
Patch v3

Looks good, though the wrapping of the line is a bit odd. I can fix that before checking in.

Comment 15

[Przemyslaw Bialik]

Thanks, it's odd because my editor fooled me -> it got ditched :)

Comment 16

[Magnus Melin [:mkmelin]]

As a side effect of the UpdateMailToolbar call, this also fixes bug 449679.

changeset:   1006:90b2b7edbdcd
http://hg.mozilla.org/comm-central/rev/90b2b7edbdcd

->FIXED

Comment 17

[Przemyslaw Bialik]

-> VERIFIED

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081105 Lightning/1.0pre Shredder/3.0b1pre