354691 - Dual sidebar system for Composer

Status: RESOLVED FIXED

(Composer Graveyard :: Sidebars, enhancement)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I want a dual sidebar system for composer, one sidebar at the left of the content area, one at the right. Each sidebar can contain multiple sidebar items.
Each sidebar item can be moved up and down in its sidebar, moved to the other side bar or detached from the main window in its own standalone window.
Of course, sidebar items can be closed and removed from their owner sidebar.

The set of current sidebar items should be persistent, and the size of each
sidebar item should be persistent too. By default, sidebar items are flexing in
their sidebar owner.

Sidebars can be closed.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Updating component, now that we have a complete list !

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #1

patch successfully tested.
even the position and size of detached sidebar items is persistent :-)

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 240915 [details] [diff] [review]
fix #1

Neil, can you comment and, if possible, review ? Thanks !

Comment 4

[[e-mail]]

Comment on attachment 240915 [details] [diff] [review]
fix #1

>+            var label      = this._getString(name + "Title", properties);
I wonder why you didn't use a title attribute?

>+            window.openDialog("chrome://composer/content/dialogs/standaloneSidebar.xul","_blank",
>+                              "chrome,modal=no,titlebar,scrollbars=yes,resizable", name,
>+                              label ? label : name, src, item);
Note: This will still have the dialog style, which may or may not be appropriate for this window. Maybe all,dialog=no would be better?


>+      <property name="mItems"
>+                onget="return this.getElementsByTagName('sidebaritem');" />
Nit: readonly="true"

>+            var strBundleService =
>+                Components.classes["@mozilla.org/intl/stringbundle;1"].getService(); 
>+            strBundleService = 
>+                strBundleService.QueryInterface(Components.interfaces.nsIStringBundleService);
Nit: use getService(Components.interfaces.nsIStringBundleService); instead

>+           stringBundle = strBundleService.createBundle(aURL); 
Nit: extra space at end, should be at beginning ;-)

>+          } catch (ex) {}
>+
>+          if (stringBundle)
>+          {
>+            try {
>+              return stringBundle.GetStringFromName(aStringName);
>+            } catch (e) {}
>+          }
Don't need double try/catch here, createBundle never returns null.

>+            if (itemsArray[i].length)
Nit: if (itemsArray[i]) suffices

>+              var item = aSidebarItems._getItemsFromName(itemsArray[i]);
>+              if (item && item.length)
>+              {
>+                item = item[0];
Nit: _getItemsFromName never returns null, so use
var item = aSidebarItems._getItemsFromName(itemsArray[i]).item(0);
if (item)

>+                item.setAttribute("height", child.getAttribute("height"));
>+                // make it persist so we get it back next session
>+                document.persist(item.getAttribute("id"), "height");
Nit: many of these attributes (e.g. id, height) have equivalent properties.

>+          // sort it by ordinal
>+          itemsArray.sort();
Actually it sorts by textual representation so 11 will sort before 3.

>+          var ordinalIndex = this.childNodes.length + 1;
>+          if (ordinalIndex != 1)
>+          {
>+            var splitter = document.createElementNS(this.mXUL_NS, "splitter");
>+            // we need to know when the splitter is moved.
>+            splitter.setAttribute("oncommand",
>+                                  "this.parentNode._setPersistentHeights()");
>+            // append the splitter to the sidebar
>+            splitter.setAttribute("ordinal", ordinalIndex++);
>+            this.appendChild(splitter);
>+          }
Might want to use ++ordinalIndex to save adding 1 earlier.

>+          // popup cleanup
>+          var child = aPopup.firstChild;
>+          while (child)
>+          {
>+            var tmp = child.nextSibling;
>+            aPopup.removeChild(child);
>+            child = tmp;
>+          }
IIRC bz prefers
while (aPopup.hasChildNodes())
  aPopup.removeChild(aPopup.lastChild);

>+          var iframes = this.getElementsByTagName("iframe");
>+          if (iframes && iframes.length)
>+            return iframes[0]
>+          return null;
iframes is never null so just return iframes.item(0); (with a semicolon!)

>+          if (currentOrdinal == 1)
>+          {
>+            // we have a splitter before the current item and we have
>+            // to remove it
You mean after, right?

>+      <method name="_movetToOtherSidebar">
moveTo rather than movetTo?

>+function GetUIElements()
>+{
>+  var elts = document.getElementsByAttribute("id", "*");
>+  for (var i = 0; i < elts.length; i++)
>+  {
>+    dgid( elts.item(i).getAttribute("id") );
>+  }
>+}
>+
>+// that one is really very useful
> function dgid(aStr)
> {
>-  return document.getElementById(aStr);
>+  gDialog[aStr] = document.getElementById(aStr);
> }
You get the id from the element, then call getElementById to get the element!

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix#2, in answer to Neil's comments

Comment 6

[[e-mail]]

Comment on attachment 241185 [details] [diff] [review]
fix#2, in answer to Neil's comments

>+                              label ? label : name, src, item);
label || name also works.

>+      <property name="mContents"
>+                onget="return this.getElementsByTagName('sidebarcontent');" />
>+      <property name="mSplitters"
>+                onget="return this.getElementsByTagName('splitter');" />
Some readonlys I missed last time.

>+          // sort it by ordinal
>+          itemsArray.sort();
You forgot to use your compare function...

>+            newList += itemsArray[i][1];
>+            if (i != itemsArray.length - 1)
>+              newList += ",";
Might be neater to write
if (i)
  newList += ",";
newList += itemsArray[i][1];

>+          var ordinalIndex = this.childNodes.length;
>+          if (ordinalIndex != 1)
This is now just if (ordinalIndex)

Comment 7

[[e-mail]]

Comment on attachment 241185 [details] [diff] [review]
fix#2, in answer to Neil's comments

>+            itemsArray.push(  [ parseInt(item.getAttribute("ordinal")),
>+                                item.getAttribute("name"),
>+                                item] );
You could just push the item itself, that would avoid the confusing array subscripts e.g.
function compare(a, b) {
  return a.ordinal - b.ordinal;
}
or
newList += itemsArray[i].getAttribute("name");

Apart from my other comments everything else looks ok.

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Thanks Neil !

Fixed and checked in (trunk).