170572 - Add prefs for hiding or showing of mail toolbar buttons

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[varada]

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

Comment 1

[jglick]

Attachment: Mail & Newsgroups Pref Panel

Comment 2

[jglick]

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

Comment 3

[varada]

Attachment: Patch for pref changes for 170573 and 171068 too.

Comment 4

[varada]

Attachment: Patch for mailnews.js

added new prefs for showing and hiding toolbar buttons

Comment 5

[varada]

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.

Comment 6

[(not reading, please use [e-mail] instead)]

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.

Comment 7

[(not reading, please use [e-mail] instead)]

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

Comment 8

[varada]

Attachment: Patch V 1.1

>>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 9

[varada]

Attachment: Patch for mailnews.js V 1.1

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 100853 [details] [diff] [review]
Patch V 1.1

sr=sspitzer, but where's the change to localstore.rdf?

Comment 11

[(not reading, please use [e-mail] instead)]

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

sr=sspitzer

Comment 12

[(not reading, please use [e-mail] instead)]

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

Comment 13

[varada]

Attachment: Patch V1.0

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

Comment 14

[(not reading, please use [e-mail] instead)]

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.)

Comment 15

[varada]

Attachment: Patch V1.0 changes to localstore.rdf

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

Comment 16

[(not reading, please use [e-mail] instead)]

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

Comment 17

[varada]

Marking Fixed.

Comment 18

[Jerry Baker]

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?

Comment 19

[Peter Lairo]

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). :-\

Comment 20

[Bamm Gabriana]

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...

Comment 21

[[e-mail]]

<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 :-(

Comment 22

[(not reading, please use [e-mail] instead)]

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.

Comment 23

[[e-mail]]

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!

Comment 24

[Jerry Baker]

> 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.

Comment 25

[(not reading, please use [e-mail] instead)]

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. 

Comment 26

[varada]

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.

Comment 27

[(not reading, please use [e-mail] instead)]

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

Comment 28

[(not reading, please use [e-mail] instead)]

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

Comment 29

[jglick]

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

Comment 30

[varada]

Attachment: Changes to mailWindow.js mailwindowOverlay.xul pref-mailnews.xul

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.

Comment 31

[(not reading, please use [e-mail] instead)]

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"

Comment 32

[varada]

Attachment: Changes to pref-mailnews.xul and pref-mailnews.dtd

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 33

[(not reading, please use [e-mail] instead)]

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

sr=sspitzer

Comment 34

[varada]

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

Comment 35

[(not reading, please use [e-mail] instead)]

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?)

Comment 36

[varada]

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

Comment 37

[[e-mail]]

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 :-)

Comment 38

[laurel]

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