1011857 - Implement CustomizableUI for SeaMonkey

Status: NEW

(SeaMonkey :: UI Design, enhancement)

Description

[Jez]

Now that Firefox has CustomizableUI.jsm available for marshaling the interaction of extensions with the user interface, it would be nice for this to be available for SeaMonkey too.  In principle, it should be able to be implemented in SeaMonkey even though it doesn't have the Australis interface (maybe with one or two of the module's functions as NOPs in SeaMonkey's implementation).  Perhaps a good strategy would be to start with a few basic, core functions, and build up over time to a more complete implementation.  This bug could be a meta-bug for the eventual complete implementation of CustomizableUI in SeaMonkey?

Comment 1

[Jez]

CCing self.

Comment 2

[Robert Kaiser]

It surely would ease add-on interoperability to have this available.

Comment 3

[Matt A. Tobin]

Perhaps the CustomizableUI widgets should be wrapped in a normal toolbar item on a per widget basis so they can live in the Customize Toolbar window and be placed arbitrarily like any classical item.

Comment 4

[Michael Buckley]

I have made a started on a module that impliments an interface that is almost the same as that of CustomizableUI, which really just serves as a wrapper around how the toolbar currently work.  Anyone trying to do a restartless addon for SeaMonkey/Thunerbird would have write something like this, the only thing I am doing is making it almost the same interface.  

https://codefisher.org/catch/blog/2015/01/16/customizableui-in-thunderbird-and-seamonkey/

It is working really well in Thunderbird right now, but it is not restoring buttons correctly in SeaMonkey.  I hope to get that fixed soonish though.

Comment 5

[Philip Chee]

(In reply to Michael Buckley from comment #4)

> https://codefisher.org/catch/blog/2015/01/16/customizableui-in-thunderbird-
> and-seamonkey/

> It is working really well in Thunderbird right now, but it is not restoring
> buttons correctly in SeaMonkey.  I hope to get that fixed soonish though.

Looks interesting. Can we incorporate your code in SeaMonkey? A MPLv2 licence would be much appreciated.

Comment 6

[Michael Buckley]

Currently the project it is part of it MIT licensed, but you can have it as MPLv2 as well if you want.  It is all the same to me. Any official way I have to say that?

I have updated this file quite a bit since I made that post (I should go back and update it).  But the newer file is at https://github.com/codefisher/mozbutton_sdk/blob/master/templates/customizable.jsm

It is still a work in progress, I have done about 7 of the 30+ functions, though I think they are the more important/useful ones.  But for what it has, it is now a drop in replacement.

The other thing you could do to make extensions written for Australis work better, is make the panelview xul element an alias for a panel.  Everywhere you can put a panelview in SeaMonkey it should work like a panel.

Comment 7

[Philip Chee]

(In reply to Michael Buckley from comment #6)
> Currently the project it is part of it MIT licensed, but you can have it as
> MPLv2 as well if you want.  It is all the same to me. Any official way I
> have to say that?

Just add the MPL header to the top of the file and then attach the file to this bug.
See https://www.mozilla.org/MPL/headers/ for the format.

> I have updated this file quite a bit since I made that post (I should go
> back and update it).  But the newer file is at
> https://github.com/codefisher/mozbutton_sdk/blob/master/templates/
> customizable.jsm

Great!

> It is still a work in progress, I have done about 7 of the 30+ functions,
> though I think they are the more important/useful ones.  But for what it
> has, it is now a drop in replacement.

> The other thing you could do to make extensions written for Australis work
> better, is make the panelview xul element an alias for a panel.  Everywhere
> you can put a panelview in SeaMonkey it should work like a panel.
I think this should be done in a separate bug.

Comment 8

[Michael Buckley]

Attachment: customizable.jsm

Comment 9

[Philip Chee]

(In reply to Michael Buckley from comment #8)
> Created attachment 8597621 [details]
> customizable.jsm

Great! Thanks!

Comment 10

[Michael Buckley]

Attachment: Updated to fix problem with deleteWidget

Comment 11

[Philip Chee]

Attachment: Patch v1.0 Reformat and add to build.

CustomizableUI.jsm checkpoint 1
1. Reformatted to match Mozilla style guide.
2. Removed Cc, Ci, Cu shortcuts.
3. Import and use Services.jsm.

Comment 12

[Philip Chee]

Attachment: Patch v1.1 Reformat and add to build, fix spelling error

CustomizableUI.jsm checkpoint 1.1
1. Reformatted to match Mozilla style guide.
2. Removed Cc, Ci, Cu shortcuts.
3. Import and use Services.jsm.
4. Fix spelling mistake s/Implimented/Implemented/g

Comment 13

[Philip Chee]

Comment on attachment 8625072 [details] [diff] [review]
Patch v1.1 Reformat and add to build, fix spelling error

Just asking for preliminary feedback and bikeshedding. Currently most methods are stubs that throw. *This* bug is for landing the file. Implementing methods should be in follow-up bugs.

Comment 14

[lemon_juice]

I don't think I am familiar enough with the CustomizableUI code to be able to have any opinion on your proposed changed. I could only test some Fx extensions that use it if I had a build of SM with your patch applied.

Comment 15

[Philip Chee]

(In reply to lemon_juice from comment #14)
> I don't think I am familiar enough with the CustomizableUI code to be able
> to have any opinion on your proposed changed.
That's OK. feedback can also mean take it out for a spin and let us know if it works.

> I could only test some Fx
> extensions that use it if I had a build of SM with your patch applied.

Uploaded to http://seamonkey.callek.net/contrib/
seamonkey-2.38a1.en-US.win32-20150630.zip

Comment 16

[lemon_juice]

I've tested it with a most basic extension https://github.com/jvillalobos/Australis-Hello-World, which only puts a button with a graphic icon in the toolbar. In SM it was partially successful - the button became available in the customize window (it wasn't placed automatically on the toolbar like in Firefox - but that's fine) and I could move it to toolbars and the click action worked. The problem was the icon did not appear at all so the button was recognizable only on toolbars that had text labels enabled.

Comment 17

[Ian Neal]

Comment on attachment 8625072 [details] [diff] [review]
Patch v1.1 Reformat and add to build, fix spelling error


>+  addWidgetToArea: function(aWidgetId, aArea, aPosition) {
>+    // we are not yet handeling aPosition
Nit: handling, also should this be a TODO?

>+  registerArea: function(aName, aProperties) {
>+    // TODO: we need to do something about tracking the currentset for
>+    // registered toolbars
Nit: missing .

>+  unregisterArea: function(aName, aDestroyPlacements) {
>+    try {
>+      gAreas.delete(aName);
>+    } catch (e) {}
>+    callWithEachWindow(function(win) {
>+      var doc = win.document;
For consistency shouldn't this be document rather than doc?

>+  endBatchUpdate: function(aForceDirty) {
>+    // do we care?
Nit: Do rather than do.

>+  registerToolbarNode: function(aToolbar, aExistingChildren) {
>+    // do we need this?  It should not really be used by anyone.
Nit: Do rather than do.

>+  removeWidgetFromArea: function(aWidgetId) {
>+    callWithEachWindow(function(win) {
>+      var doc = win.document;
For consistency shouldn't this be document rather than doc?

>+function getButton(aButtonId, toolbar) {
Shouldn't this be aToolbar?

>+function restoreToToolbar(toolbox, aWidgetId) {
Shouldn't this be aToolbox?

>+function addWidgetToWindow(window, aProperties) {
Should this be aWindow?

>+function addWidgetToArea(window, aWidgetId, aArea, aPosition) {
Should this be aWindow?

>+  // we are not yet handeling aPosition
Nit: handling not handeling

>+function registerAreaForWindow(win, aName, aProperties) {
Should this be aWindow instead of win?

>+function handleWindowLoad(window) {
Should this be aWindow?

>+function callWithEachWindow(func) {
aFunction instead

>+function log(e) {
maybe aError or aMessage?

>+var windowListener = {
>+  hwl: handleWindowLoad,
>+  onOpenWindow: function(aWindow) {
>+    // Wait for the window to finish loading
Nit: missing .

Comment 18

[[e-mail]]

Comment on attachment 8625072 [details] [diff] [review]
Patch v1.1 Reformat and add to build, fix spelling error

>+        button.parentNode.removeChild(button);
button.remove();

>+        var toolboxes = document.getElementsByTagName("toolbox");
>+        for (var t = 0; t < toolboxes.length; t++) {
>+          try {
>+            var toolbox = toolboxes[t];
>+            if (!toolbox.palette) {
>+              continue;
>+            }
>+            var buttons = toolbox.palette.getElementsByAttribute("id", aWidgetId);
>+            for (var i = 0; i < buttons.length; i++) {
>+              toolbox.palette.removeChild(buttons[i]);
>+            }
>+          } catch (e) {
>+            log(e);
>+          }
>+        }
(I guess this code was written to work in both SeaMonkey and Thunderbird; of course this bit will never happen because our toolboxpalettes remain in the DOM.)

>+    var old = [];
>+    for (let aProperties of gWidgets) {
>+      if (aProperties.id == aWidgetId) {
>+        old.push(aProperties);
>+      }
>+    }
>+    for (var i = 0; i < old.length; i++) {
>+      try {
>+        gWidgets.delete(old[i]);
>+      } catch (e) {}
>+    }
I'm pretty sure you can delete set elements as you enumerate them.

>+          toolbar.setAttribute("currentset", "");
"__empty"?

>+  removeListener: function(aListener) {
>+    if (aListener == this) {
>+      return;
>+    }
[?]

>+          var currentSet = toolbar.getAttribute("currentset");
>+          toolbar.setAttribute("currentset", currentSet.replace(aWidgetId, ""));
Rather than attempting to recalculate the current set yourself, just ask the toolbar.currentSet and it will recalculate it for you.

>+          toolbar.persist(toolbar, "currentset");
[This only works for built-in toolbars.]

>+    var buttonSet = toolbar.getAttribute("currentset");
...
>+        toolbar.setAttribute("currentset", buttonSet);
>+        document.persist(toolbar.id, "currentset");
Value of buttonSet doesn't actually change.

>+  if (aProperties.toolbox) {
>+    var toolboxId = aProperties.toolbox;
>+  } else {
>+    var toolboxId = "navigator-toolbox";
>+  }
>+  var toolbox = document.getElementById(toolboxId);
(aProperties.toolbox || "navigator-toolbox")

>+          toolbox.setAttribute("_addeddefaultset", buttonSet);
>+          document.persist(toolbox.id, "_addeddefaultset");
???

>+  toolbar.insertItem(aWidgetId, null, null, false);
>+  document.persist(aArea, "currentset");
You didn't actually update the current set.

>+function registerAreaForWindow(win, aName, aProperties) {
>+  let document = win.document;
>+  if (!document) {
>+    return;
>+  }
>+  var toolbar = document.getElementById(aName);
>+  if (!toolbar) {
>+    return;
>+  }
>+  if (toolbar.hasAttribute("currentset")) {
>+    var buttonSet = toolbar.getAttribute("currentset");
>+  } else {
>+    var buttonSet = toolbar.getAttribute("defaultset");
>+  }
>+  var buttons = buttonSet.split(",");
>+  for (var i in buttons) {
>+    toolbar.insertItem(buttons[i], null, null, false);
>+  }
>+  // we do this to make sure we have not changed or messed up anything by calling insertItem
>+  toolbar.setAttribute("currentset", buttonSet);
>+  document.persist(aName, "currentset");
>+}
What is this supposed to achieve?

>+function callWithEachWindow(func) {
>+  let windows = Services.wm.getEnumerator(null);
I guess this function works for any window with a customisable toolbar as long as its id is unique?

>+  while (windows.hasMoreElements()) {
>+    let domWindow = windows.getNext().QueryInterface(Components.interfaces.nsIDOMWindow);
Nit: You don't actually need to QI for nsIDOMWindow.

>+    domWindow.addEventListener("load", function onLoad() {
>+      domWindow.removeEventListener("load", onLoad, false);
>+      self.hwl(domWindow);
>+    }, false);
Ideally should just add handleWindowLoad as the event listener (and make handleWindowLoad remove itself), but at least just call handleWindowLoad directly.

Comment 19

[Woi]

This bug blocks issue #158 "Add SeaMonkey support" of the passff plugin. See: https://github.com/jvenant/passff/issues/158

Comment 20

[WaltS48 [:walts48]]

I don't think Phillip is working on this anymore.

Comment 21

[Matt A. Tobin]

No kidding, Ratty hasn't been heard from for years now.

Comment 22

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8625072 [details] [diff] [review]
Patch v1.1 Reformat and add to build, fix spelling error

No Ratty left the project ( or at least we must assume it). I looked at it CustomizableUI a while beack but when we implement it current NoScript 5.1.x will break. Seems it is used there to check for Firefox. Maybe for 2.57.