359656 - Make it possible to unfork toolbar customization

Status: RESOLVED WORKSFORME

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Phil Ringnalda (:philor)]

Rocinante and I want to bring Thunderbird and Sunbird back to using the copies of customizeToolbar.xul and customizeToolbar.js in toolkit/, rather than having to maintain their own forks, but there are things they've added that they'll need backported, and things (more than I knew) that are Firefox-specific that have crept into toolkit's copy.

What I know so far:

- Thunderbird wants to keep its Cancel button for Windows and Linux, other consumers may or may not want it
- Restoring the default set needs to restore the application's default for icon size and icons+text-or-not, not Firefox's defaults

Comment 1

[Phil Ringnalda (:philor)]

Attachment: Restore default v.1

calendar/'s getting antsy to be unforked, probably because as bug 362742 shows I'm not as good at remembering their half-forked state as I should be, so, in two parts: this makes restoring default set restore a per-app defaultmode (and defaulticonsize, if anyone defaults to small), and sets up Fx and Sb to use it (I've got the mail/ changes in my tree, but I'll save them and a review flag for later) - I'll port Tb's Cancel in a later patch.

While I was in the neighborhood, looking at icon size and restoring, I moved the init()ing of small icons around, so it not only remembers them while in text mode, but also shows that it remembered right away.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 247512 [details] [diff] [review]
Restore default v.1

>Index: toolkit/content/customizeToolbar.js
>===================================================================
finishToolbarCustomization()
> }
> 
> function initDialog()
> {
>   var mode = gToolbox.getAttribute("mode");
>   document.getElementById("modelist").value = mode;
>   gToolboxIconSize = gToolbox.getAttribute("iconsize");
>   var smallIconsCheckbox = document.getElementById("smallicons");
>-  if (mode == "text")
>+  smallIconsCheckbox.checked = gToolboxIconSize == "small";
>+  if (mode == "text")  

nit: trailing space.


>   // Build up the palette of other items.
>   buildPalette();
> 
>   // Wrap all the items on the toolbar in toolbarpaletteitems.
>   wrapToolbarItems();
> }
> 
>@@ -571,21 +570,25 @@ function restoreDefaultSet()
>         var defaultSet = toolbar.getAttribute("defaultset");
>         if (defaultSet)
>           toolbar.currentSet = defaultSet;
>       }
>     }
>     toolbar = toolbar.nextSibling;
>   }
> 
>-  // Restore the default icon size (large) and mode (icons only).
>-  updateIconSize(false);
>-  document.getElementById("smallicons").checked = false;
>-  updateToolbarMode("icons");
>-  document.getElementById("modelist").value = "icons";
>+  // Restore the default icon size and mode.
>+  var defaultIconSize = gToolbox.getAttribute("defaulticonsize");
>+  var defaultMode = gToolbox.getAttribute("defaultmode");
>+  var defaultIconsSmall = defaultIconSize == "small";

maybe change this to |var defaultIconsSmall = gToolbox.getAttribute("defaulticonsize") == "small"| and get rid of defaultIconSize? A boolean attribute might be cleaner here (defaultsmallicons?).

Comment 3

[Phil Ringnalda (:philor)]

Attachment: Restore default v.2

Gah, I can't believe I took out one trailing space, and added four.

I one-lined the defaultIconsSmall check, but I'm leery of making the attribute a boolean, unless you think we can persuade Gavin that it's part of his toolbar binding rewrite to migrate the current horrid "iconsize has two values, 'small' means small, and absolutely anything else, ='big' or ='foopygoats' or no attribute, means large" to a boolean, too. I think it's confusing enough now, without having two separate attributes with similar names, but different behavior and values.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 247776 [details] [diff] [review]
Restore default v.2

OK, r=mano.

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 247776 [details] [diff] [review]
Restore default v.2

lilmatt: see, toldja I would. We'll wire calendar/ up in a Calendar bug, but having gotten your default size-and-mode wrong before, I just wanted your stamp on committing these defaults (cross-, since they won't hurt anything on the branch).

Comment 6

[Matthew (lilmatt) Willis]

Comment on attachment 247776 [details] [diff] [review]
Restore default v.2

r=lilmatt

Comment 7

[Phil Ringnalda (:philor)]

toolkit/content/customizeToolbar.js 1.34
browser/base/content/browser.xul 1.330
calendar/sunbird/base/content/calendar.xul 1.48

Comment 8

[Robert Kaiser]

Hmm, is current toolkit code enough cross-application that (toolkit-based) SeaMonkey can build upon it for resolving bug 15144 some time, or is there still some work needed in this bug?
I just stumbled upon this one and realized it may or may not block our bug, depending on the completeness of the work here...

Comment 9

[Phil Ringnalda (:philor)]

It's usable - calendar/ uses it. What's left here is porting Thunderbird's "Cancel" back so it can use it, and maybe moving the menu code to toolkit/ like I started in bug 220701

Comment 10

[Philip Chee]

(In reply to Comment 0)
> - Restoring the default set needs to restore the application's default for icon
> size and icons+text-or-not, not Firefox's defaults

FYI I managed to get (for SeaMonkey) per toolbar defaults (iconsize and mode).

Comment 11

[Phil Ringnalda (:philor)]

Given how unforked we now are, and how well you're doing on more unforking elsewhere, I don't think this bug is doing any good anymore.