325468 - Creating a new toolbar does not appear to do anything. [no empty toolbar / no Toolbar-menu entry]

Status: VERIFIED FIXED

(Calendar :: Sunbird Only, defect)

Description

[zobrists]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; FunWebProducts-MyWay; .NET CLR 1.1.4322; InfoPath.1; .NET CLR 2.0.50727)
Build Identifier: v0.3a1

Under View-Toolbars-Customize Toolbar, "add new toolbar" does not appear to have any effect.

Reproducible: Always

Steps to Reproduce:
1.Go to View-Toolbars-Customize Toolbar
2.Click "add new toolbar"
3.Enter in any text for the name
4.Click OK

Actual Results:  
Nothing happens.

Expected Results:  
That it would create a new empty toolbar on the menu.

Comment 1

[Stefan Sitter [:ssitter]]

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060201 Mozilla Sunbird/0.3a1+:

"Add New Toolbar" creates a new toolbar that is about 1 pixel high. I can drag icons to that toolbar and the toolbar high expandes to show the icon. New toolbar is still available after restart.

But: No entry is created in the View-Toolbar menu for the new toolbar. So it is not possible to hide, show or delete the new toolbar.

Comment 2

[Robin Edrenius]

Attachment: comparation between Firefox and Sunbird

In Firefox the new toolbar is a little higher, which makes it easier to spot.
Attaching screenshot.

Comment 3

[Martin Schröder [:mschroeder]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060512 Mozilla Sunbird/0.3a2

The empty toolbar has the same size as in Firefox. (whatever fixed this) Can somebody confirm this?

But the "no entry is created in the View-Toolbar menu for the new toolbar" problem  still exists.

Comment 4

[Joey Minta]

Nominating for blocking 0.3 based on 'Modern toolkit app'.

Comment 5

[Matthew (lilmatt) Willis]

The new toolbar is much more noticeable in Mac OS X, so perhaps that's a pin vs. winstripe issue.

The lack of entry in View > Toolbars is a known issue based on the comment in calendar-menubar.inc but I don't see a bug for it after a quick search.

Comment 6

[Stefan Sitter [:ssitter]]

Using Calendar/0.3a2+ (20060722) on Windows 2000 I get a new empty toolbar that is almost 20px height. So this issue is fixed somehow and only the 'no Toolbar-menu entry' issue is left (as written in Comment #5).

Comment 7

[Matthew (lilmatt) Willis]

Taking this

Comment 8

[Matthew (lilmatt) Willis]

Attachment: rev0 - uses functions from firefox

This patch uses the functions from Firefox to update the View > Toolbars menu with the list of available toolbars. 

Since this is a Sunbird-only thing, I'm taking the step of making a Sunbird-only .js file beneath /m/cal/sunbird. Other Sunbirdy functions can make it into this file eventually, but not in this bug. It is not named sunbird.js so as to not get confused with the preferences file of that name. Eventually (bug 290747) this can all be in its own sunbird.jar

Comment 9

[Matthew (lilmatt) Willis]

Attachment: rev1 - uses applicationUtil.js rather than creating sunbird-only.js

jminta suggested using applicationUtil.js for the Sunbird-only functions since that's what it was originally meant for.  This patch does that.

Comment 10

[Joey Minta]

Comment on attachment 230440 [details] [diff] [review]
rev1 - uses applicationUtil.js rather than creating sunbird-only.js

eep.  copy and paste can be the downfall of code development.

1.) This code needs a giant whack from the Sunbird-style stick. 4-space indents, braces around ifs, etc.

+
+      menuItem.addEventListener("command", onViewToolbarCommand, false);
+    }
Whenever I see addEventListener without removeEventListener I get really scared about leaks.

+    toolbar = toolbar.nextSibling;
+  }
This line looks like we're in a while loop, but we're actually in a for loop, so I don't think this is necessary.

+var gHaveUpdatedToolbarState = false;
+
+function updateToolbarStates(toolbarMenuElt)
+{
+  if (!gHaveUpdatedToolbarState) {
woah...
At the very least, let's do if (gTooLongOfAVariableName) {
  return;
}
to save us some indenting.  Really, however, let's set ourselves up so we don't call this twice.  (and I hate global variables)

Basically, it looks like gTooLongOfAVariableName is only set to true when we're in fullscreen mode, so we should probably just call this function when we enter that mode, rather than checking it every time we popup this menu.

+      var i;
+      for (i = 0; i < toolbarMenuElt.childNodes.length; ++i)
+        document.getElementById(toolbarMenuElt.childNodes[i].getAttribute("observes")).removeAttribute("checked");
Why? We're never setting an "observes" attribute, so why will it ever be there to remove?

+      // Start i at 1, since we skip the menubar.
+      for (i = 1; i < toolbars.length; ++i) {
+        if (toolbars[i].getAttribute("class").indexOf("chromeclass") != -1)
+          toolbars[i].setAttribute("collapsed", "true");
+      }
+      var statusbars = document.getElementsByTagName("statusbar");
+      for (i = 1; i < statusbars.length; ++i) {
+        if (statusbars[i].getAttribute("class").indexOf("chromeclass") != -1)
+          statusbars[i].setAttribute("collapsed", "true");
+      }
+      
I can understand why we start at 1 for toolbars, but not at all why we start there for statusbars.

I'll also include a general request for more code comments here, since otherwise this is going to be nearly impossible to maintain.

Comment 11

[Matthew (lilmatt) Willis]

(In reply to comment #10)
> (From update of attachment 230440 [details] [diff] [review] [edit])
> eep.  copy and paste can be the downfall of code development.

You're right. Somehow, this should really be part of toolkit. viewMenuOverlay or some such thing. I'm getting tired of copy/pasting code from Ff/Tb that is shared among us all.

Comment 12

[Matthew (lilmatt) Willis]

Attachment: rev2 - fixes jminta's nits

I fixed the style issues, added a removeListener before removing each menuitem, and looked at why we would need that last function.
Since we're not doing full-screen at the moment, and that's where most of the concerns and questions were, I decided to omit it.

The result is a lot cleaner and more maintainable.

Comment 13

[Joey Minta]

Comment on attachment 231426 [details] [diff] [review]
rev2 - fixes jminta's nits

Have you tested this in lightning?  I'm pretty sure we'll break their toolbar-customization here.

Comment 14

[Matthew (lilmatt) Willis]

(In reply to comment #13)
> (From update of attachment 231426 [details] [diff] [review] [edit])
> Have you tested this in lightning? 

No, as this was a Sunbird-only bug, and in comment #9 I moved the functions to applicationUtil.js as we discussed via IRC. 

Where do you want me to move the functions to then? I'm thinking I _should_ create a sunbird-only.js with these functions and Sunbird-only stuff can get moved to it in another bug.

Comment 15

[Joey Minta]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 231426 [details] [diff] [review] [edit] [edit])
> > Have you tested this in lightning? 
> 
> No, as this was a Sunbird-only bug, and in comment #9 I moved the functions to
> applicationUtil.js as we discussed via IRC. 
> 
> Where do you want me to move the functions to then? I'm thinking I _should_
> create a sunbird-only.js with these functions and Sunbird-only stuff can get
> moved to it in another bug.
> 
The reason we break Thunderbird customization is because we'll end up replacing their function which is also called onViewToolbarCommand.  There are a couple problems here:
1.) I should be shot for including applicationUtil.js in Lightning.
2.) We need to namespace functions

Solving 2 is a temporary solution until we can fix 1.  Therefore, I'd say just rename the function to something clearly unique, and file a followup+add a comment to remove lightning's dependency on applicationUtil.js

Comment 16

[Joey Minta]

Comment on attachment 231426 [details] [diff] [review]
rev2 - fixes jminta's nits

+
+    toolbar.collapsed = aEvent.originalTarget.getAttribute("checked") != "true";

Some () around the assigned value would make this much easier to read.

+    document.persist(toolbar.id, "collapsed");
I'm 98% sure this isn't necessary on trunk.  I'm abut 60% sure it's not necessary on branch.  I know we need it on 1.5, but since this is sunbird-only, we don't care about that case.  Can you check and see whether it's necessary on branch and drop it if not?

r=jminta with those.

Comment 17

[Matthew (lilmatt) Willis]

(In reply to comment #16)
> Some () around the assigned value would make this much easier to read.
fixed.

> +    document.persist(toolbar.id, "collapsed");
> I'm 98% sure this isn't necessary on trunk.  I'm abut 60% sure it's not
> necessary on branch.  I know we need it on 1.5, but since this is sunbird-only,
> we don't care about that case.  Can you check and see whether it's necessary on
> branch and drop it if not?
It's necessary.

Spinoff for Lightning to stop using applicationUtil.js is bug 346762.

patch checked in (with parens) on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 18

[Stefan Sitter [:ssitter]]

Verified with Sunbird/0.3a2+ (2006080207) on Linux and Windows 2000.