514858 - Cannot disable Shredder start page, regardless of setting

Status: VERIFIED FIXED

(Thunderbird :: Preferences, defect)

Description

[IU]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090905 Lightning/1.0pre

No longer able to disable the Shredder start page, regardless of the setting in Options

Regression:
Works: Gecko/20090904
Broken: Gecko/20090905


Reproducible: Always

Steps to Reproduce:
1. Go to Tools --> Options... --> General
2. Uncheck "Shredder Start Page"
3. Restart Shredder
4. Notice that the start page remains.  The region should be blank.

Actual Results:  
Start page persists

Expected Results:  
Region should be blank

Comment 1

[rsx11m]

Confirmed, the start page is shown regardless of mailnews.start_page.enabled being false [Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090905 Shredder/3.0b4pre].

Comment 2

[Mark Banner (:standard8)]

Attachment: Proposed fix

I've done some digging and we are missing the option to check the preference. What we used to do is:

- Not show the start page if the enable preference is set to false.
- Show the start page if the user manually goes to Go -> Mail Start Page (whatever the state of the preference).
- If showing the start page, then we would show it once for each nsIMsgIncomingServer (so each mail server).

This patch does the following:

- Not show the start page if the enable preference is false.
- Disable Go -> Mail Start page if the preference is false.
- Disable the options to set the start page if the preference is false.
- Disables the Go -> Mail Start page option for non-folder pane tabs (e.g. message & gloda search).
- If the preference is true, we'll show the start page every time you click on a folder and nothing is automatically selected.

The last of these is possibly the most significant difference overall once we switch between the two, however when bug 505044 lands this will reduce the amount we actually show it, so I don't think that would be an issue.

The disabling of the mail start page follows what SeaMonkey does. We can in theory make Go -> Mail Start Page work whilst not enabling the preference but it does seem a little strange.

Anyway, asking for ui-review to see what Bryan thinks.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 398868 [details] [diff] [review]
Proposed fix

(In reply to comment #2)
> The disabling of the mail start page follows what SeaMonkey does. We can in
> theory make Go -> Mail Start Page work whilst not enabling the preference but
> it does seem a little strange.

This is the only part that I'm unsure about.  I guess it also brings in uncertainty with the idea of disabling the start page entry when the pref is disabled.

That system strikes me as a bit strict for the sake of being strict and not directly for the users benefit.

ui-r+ because I think we want this fix in and can work out this last part over time.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 398868 [details] [diff] [review]
Proposed fix

Yeah i agree it should be possible to load the start page by Go | Mail Start Page even if you don't have it enabled on startup

>+    document.getElementById("restoreDefaultStartPageButton").disabled = disabled;    

Trailing spaces.

More importantly though, this broke the Restore Default button

Error: startPage is not defined
Source File: chrome://messenger/content/preferences/general.js
Line: 35

Comment 5

[Mark Banner (:standard8)]

Attachment: Simpler Fix

Simpler fix, I think this gets us back to where we where before - even if the enable option is disabled in prefs, then we'll load up the url if the user goes to Go -> Start Mail Page.

I've also kept the change so that we only enable the Go -> Mail Start Page menu option on folder panes (e.g. message tabs etc don't want it).

Switching review to bienvenu as I think mkmelin said he was busy until the weekend and I'd like to fix this regression for b4.

Comment 6

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/6033678956c1

Comment 7

[IU]

Verified:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090912 Lightning/1.0pre ID:20090912032112

Thanks