Add prefs for hiding or showing of mail toolbars. Jennifer will be adding the new screenshots.

Note: Plan is to someday have customizable toolbars. Can't do that yet.

added new prefs for showing and hiding toolbar buttons

In the fix I didnt go with using persistence and storing hidden attribute in the localstore.rdf because it wouldnt be reflected in the standalone mailwindow as well as alternate 3pane. If we use prefs instead it gets checked each time and therefore we maintain the same toolbar all the time as per the user's preferences.

looks good. some questions / comments: 1) is "mailnews.remember_selected_message" the same pref as we had in 4.x? 2) you'll want to add (but comment out) and element for "junkButton" to _elementIDs 3) I think jglick wants file and next off by default 4) a lot of this code looks heavily copied and pasted from navigator.js (is that code from callion or bz?) Is it similar enough to make it worth sharing? Or is there not enough to justify that? some comments on the code, so worth fixing in both mailWindow.js and navigator.js + var show = pref.getBoolPref(prefName); + hideButton(buttonName,show) just do: + hideButton(buttonName, pref.getBoolPref(prefName)); and instead of: + var buttonId = "button-" + name; + + if (show) + document.getElementById(buttonId).setAttribute("hidden","false"); + else + document.getElementById(buttonId).setAttribute("hidden", "true"); just do: document.getElementById("button-" + name).setAttribute("hidden", show ? "true" : "false"); 5) "In the fix I didnt go with using persistence and storing hidden attribute in the localstore.rdf because it wouldnt be reflected in the standalone mailwindow as well as alternate 3pane." I think you want to add persist="hidden" to all these buttons. Otherwise, if I'm a user that hides them all, the first time I open the 3 pane or the stand alone msg window after a restart my toolbar is going to jump. that's going to look ugly. Since jglick wants to hide next and file by default, you should add those default values to localstore.rdf for messageWindow.xul, messenger.xul and the *vert*.xul.

I wrote: "4) a lot of this code looks heavily copied and pasted from navigator.js (is that code from callion or bz?) Is it similar enough to make it worth sharing? Or is there not enough to justify that?" given that mail & browser are splitting apart, and that you didn't add that much code, let's not put this code in a common place. (unless it turns out that all windows, compose, editor, etc, etc, start doing the exact same thing.) but you should see if my code clean up suggestion work, and if so, do the same for navigator.js

>>1) is "mailnews.remember_selected_message" the same pref as we had in 4.x? I checked it out with lxr classic. http://lxr.mozilla.org/classic/search?string=remember_selected_message >>2) you'll want to add (but comment out) and element for "junkButton" to >>_elementIDs Added and commented out the _elementID for junkButton. >>3) I think jglick wants file and next off by default Changed default settings in mailnews.js -will be attaching in next patch. >>just do: >>document.getElementById("button-" + name).setAttribute("hidden", show ? "true" >>:"false"); the logic is the other way round but yes it worked and saves a lot of lines and I will file a bug to make the change in navigator.js as well.

Comment on attachment 100853 [details] [diff] [review] Patch V 1.1 sr=sspitzer, but where's the change to localstore.rdf?

Comment on attachment 100854 [details] [diff] [review] Patch for mailnews.js V 1.1 sr=sspitzer

so before you check in, we need a patch for localstore.rdf after this lands, you can go log a bug and attach a patch for navigator.js

Changes for localstore.rdf. This will stop any possible jump in the toolbar when we startup- thanks to seth for the info.

Comment on attachment 101020 [details] [diff] [review] Patch V1.0 needs work. why are you adding the description lines for button that you aren't hiding by default? also, please spin up a bugscape bug, as the ns tree will need similar changes (and might was well review and check them in at the same time.)

Removed descriptions for elements that are not to be hidden by default.

Comment on attachment 101204 [details] [diff] [review] Patch V1.0 changes to localstore.rdf looks good. assuming you tested for a new profile, sr=sspitzer

Marking Fixed.

Hey guys. This shouldn't default to all the buttons being turnd off. This caused me to file a bug on a missing "Next" button. Shouldn't they default to leaving your toolbar the way it was, and then letting the user turn them off if they want?

I recreated my entire profile because of this bug. Changing something as visible as this should have at least been announced somewhere (news://news.mozilla.org:119/netscape.public.mozilla.announce). :-\

I don't understand why we can't do this for a home button on the navigation toolbar. Surely that has much greater demand than this. Yet that was wontfixed for some bizarre ideological reason...

<screenshot> A thought: perhaps you could use a "checklistbox" (c.f. Windows System prefs) for the prefs, in case intl needs more room for translations, or you need to add more prefs. > I think you want to add persist="hidden" to all these buttons. Otherwise, if > I'm a user that hides them all, the first time I open the 3 pane or the stand > alone msg window after a restart my toolbar is going to jump. that's going to > look ugly. Since you're calling ShowHideToolbarButtons() before the window becomes visible you shouldn't need persist at all... > document.getElementById("button-" + name).setAttribute("hidden", show ? "true" : "false"); In which case, you can use document.getElementById("button-" + name).hidden = show; which you might as well inline instead of defining hideButton(). > next off by default The button I use the most :-(

varada forgot to check in the change to mailnews.js I've checked this in: pref("mail.toolbars.showbutton.file", true); pref("mail.toolbars.showbutton.next", true); /* not ready yet: pref("mail.toolbars.showbutton.junk", false); */ pref("mail.toolbars.showbutton.print", true); pref("mail.toolbars.showbutton.stop", true); I think we still want file and next off by default, but maybe not yet. (let's wait until we have a junk mail button to hide the current buttons by default) as far as neil's comments about localstore.rdf, and .hidden, I'll let varada comment. I suggested the localstore.rdf changes because I assumed we were going to hide/show on startup, but if it all happens before the window shows, we can remove the localstore.rdf changes. I'll leave it to varada to investigate.

Oh, and of course localstore is no use in the following case anyway: 1. Opens mail window. Discovers Next button is hidden, so changes pref 2. Opens message in new window. Localstore says hidden, pref says not!

> I think we still want file and next off by default, but maybe not yet. Is there any particular reason? I imagine users opening Mozilla news and wondering how to get to the next news message other than clicking to each one with their mouse.

we've also got a mismatch between our pref UI and browser's pref UI. -- Select the buttons you want to see in the toolbars -- [ ] Bookmarks [ ] Search [ ] Go [ ] Print [ ] Home We should match up the groupbox labels, and probably use a grid like browser (if it will fit). Once we add junk mail, we'll also have five buttons. varada, can you work with jglick and robin (to decide if they want to fix our entity or browsers), and if they agree about the layout.

I think it is good that we should have an uniform layout for similar prefs. Once a screenshot of the layout is given I can implement that asap - do you think I should open another bug because this one is about adding the prefs and it is already done? Will also look into the localstore.rdf and make sure we are not making redundant checks for persistance, as well as trying to get rid of the extra hideButton() function.

no need for a new bug. just do it all here.

don't forget to back out your fix for bugscape #20289, too.

Changing our layout so the spacing matches browser is fine w/me. Buttons should appear in the order they would appear on the toolbar.

Got rid of the hideButton function in mailWindow.js and persist attributes in mailWindowOverlay.xul. Redesigned the groupbox in pref-mailnews.xul to match the browser(jglick is in agreement with that). Have updated localstore.rdf in both trees with backed out version - will check that in with these changes once they are reviewed.

1) see how browser did it in pref-navigator.xul 2) what about the label? shouldn't browser and mail prefs use the same wording? 3) why did you add this id? id="menu_showSearch_showMessage_Separator"

Re-aligning checkboxes into a two sets of vboxes inside a horizontal groupbox. Also changed title label of groupbox to conform with the browser toolbar pref's groupbox.

Comment on attachment 102249 [details] [diff] [review] Changes to pref-mailnews.xul and pref-mailnews.dtd sr=sspitzer

Backed out changes to localstore.rdf and checked in changes to pref-UI and optimised the function calls.

Comment on attachment 102239 [details] [diff] [review] Changes to mailWindow.js mailwindowOverlay.xul pref-mailnews.xul can you attach a complete patch? this one needs work (again, why the new id on the separator?)

that extra ID was part of fix for 107447(also checked in now) - talked to seth on aim and got the SR.

Comment on attachment 102239 [details] [diff] [review] Changes to mailWindow.js mailwindowOverlay.xul pref-mailnews.xul >+ document.getElementById("button-" + prefArray[i]).hidden = !(prefBranch.getBoolPref(prefArray[i])); Um, yes, I meant .hidden = !(show), honest :-)

OK using nov20 commercial trunk build: win98, linux rh8.0, mac OS 10.2