399310 - Add Firefox broadcaster-based Sidebar API to SeaMonkey Sidebar

Status: RESOLVED FIXED

(SeaMonkey :: Sidebar, defect)

Description

[Manuel Reimer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.8.1.6) Gecko/20070809 SeaMonkey/1.1.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.8.1.6) Gecko/20070809 SeaMonkey/1.1.4

Currently it's difficult to add sidebar panels to SeaMonkey, as we don't have a real API for doing that. To make this easier, the Firefox Sidebar API should be added into SeaMonkey.

Reproducible: Always

Comment 1

[Manuel Reimer]

Attachment: First Patch

Moving the sidebar API code to here, to simplify the "code cleanup" patch

Comment 2

[Robert Kaiser]

Manuel, can you get something going here in the next few weeks?

If not, Philip, would you be up to getting at least some steps of this done in such a timeframe?

The places "open bookmark in sidebar" functionality is currently visible in our UI but needs some of this to work, so we either need to implement those parts or remove that feature from 2.1 if we can't get it done.

Comment 3

[Manuel Reimer]

I could try to get something done in the next few weeks, but IMHO it would be good to have a conversation with Philip, first.

I *was* a regular user of the sidebar and used it to follow newstickers, but the new RSS feature in Mail/News is much better for this. Now I only open sidebar from time to time to sort Bookmarks.

I think having something like a "sidebar switcher" is a nice thing, but it hasn't have to be implemented the way it currently is. Maybe there is a better way to do this.

I also think, that some stuff should be developed towards the "Xsidebar" extension, written by Philip, so he has to ship less code with his extension. Maybe it would even be possible to make it obsolete for SeaMonkey.

... and I have one condition that has to be fulfilled: It *is* impossible to do this completely as a patch! Maybe some things can be done as patch, but there are some files, I would prefer to rewrite from scratch!

About the conversation with Philip: Maybe we can meet on IRC to talk about our ideas about the sidebar? But we have to make an appointment specially for this topic, as I *can't* join the regular SeaMonkey meetings on IRC, as they usually are in my working time.

I would prefer a meeting after 8:00 PM MESZ.

Comment 4

[Philip Chee]

I'm in Australia at the moment but returning tomorrow (Wednesday). Give me a few days to recuperate and then ping me on IRC. I'm usually not online until around 1200 UTC. You could try to catch me around lunch time around 0500 UTC.

Comment 5

[Justin Wood (:Callek)]

(In reply to comment #2)
> The places "open bookmark in sidebar" functionality is currently visible in our
> UI but needs some of this to work, so we either need to implement those parts
> or remove that feature from 2.1 if we can't get it done.

If this is not done in time we should certainly remove the feature from 2.1 (in a followup, blocking+ bug)

I don't think the API match is something we should block on, but certainly should devote resources if we have them to spare.

Comment 6

[Manuel Reimer]

Attachment: "quick and dirty" fix

This patch "just adds the new API". It syncs broadcasters to the current RDF system to make the old code do its job.

This obsoletes local-panels.rdf.

Comment 7

[Karsten Düsterloh]

Comment on attachment 481007 [details] [diff] [review]
"quick and dirty" fix

Basically, it works.

>diff --git a/suite/common/sidebar/customize.js b/suite/common/sidebar/customize.js
>+    // Check, if the item is in the master sequence. If so, we don't remove it

What does this mean? What's the concept of "master sequence"? (Plus, sentences should end with a punctuation mark. Please add those everywhere where you are using full sentences eg. starting with uppercase words, even I don't comment on it again.)
Actually, I think you just want to say something like "Remove already used panels from the list of available panels."

>+    var ds = sidebarObj.datasource;

ds is rather useless for just one use and doesn't add much info.

>+    var master_list = ds.GetTarget(RDF.GetResource(allPanelsObj.resource),
>+                                   RDF.GetResource(NC + "panel-list"),
>+                                   true);
>+    var masterSeq = Components.classes["@mozilla.org/rdf/container;1"]
>+                              .createInstance(Components.interfaces.nsIRDFContainer);
>+    masterSeq.Init(sidebarObj.datasource, master_list);
>+    var inmaster = (masterSeq.IndexOf(panel) != -1);
>+    if (panel.Value in panels || inmaster) {

inmaster, otoh, is also rather useless, but at least helps understanding the code. ;-)

>diff --git a/suite/common/sidebar/sidebarOverlay.js b/suite/common/sidebar/sidebarOverlay.js
>+function SidebarBroadcastersToRDF()
>+{
>+  // Translation rules to translate between new broadcaster id and old RDF id
>+  const translate = {"viewBookmarksSidebar" :   "bookmarks",
>+                     "viewHistorySidebar" :     "history",
>+                     "viewSearchSidebar" :      "search",
>+                     "viewAddressbookSidebar" : "addressbook"};
>+  const urnprefix = "urn:sidebar:panel:";

Constants should either be all uppercase (URN_PREFIX) or have a k prefix (kURNPrefix).
The object field names don't need to be quoted (here). And no space before the ":", please.  

>+  var RDFCU = Components.classes['@mozilla.org/rdf/container-utils;1'].getService();
>+  RDFCU = RDFCU.QueryInterface(Components.interfaces.nsIRDFContainerUtils);

var RDFCU = Components.classes['@mozilla.org/rdf/container-utils;1']
                      .getService(Components.interfaces.nsIRDFContainerUtils);

>+  var bcarray = new Array();

First, you can just say "var bcarray = [];".
Second, it's wrong either way - you actually use it as a pure object later, so the correct form is ""var bcarray = {};" anyway.

>+  for (var bId = 0; bId < bset.childNodes.length; bId++) {
>+    var curBC = bset.childNodes[bId];

You should use let instead of var on subscope level.
Also, the variable names are incomprehensible enough to warrant some commentary about what's happening.
Third, the broadcasterset may contain non-broadcasters, it'd be better to use elem.getElementsByTagName.

>+    var title = curBC.getAttribute("sidebartitle");
>+    if (!title) title = curBC.getAttribute("label");

let title = curBC.getAttribute("sidebartitle") || curBC.getAttribute("label");

No one-line if clauses, please!

>+    if (!url || !title || !bcid) continue;

No one-line if clauses, please!

>+    // Item already exists, but perhaps we need update...

... need to ...

>+    if (curId.substr(0, urnprefix.length) != urnprefix) continue;

No one-line if clauses, please!

>+    curId = curId.substr(urnprefix);
>+    if (!curId in bcarray) {

This doesn't work as you intended, I suppose: you are computing a boolean expression whether curId is empty and try to find the result (true/false) as a field name in bcarray...

>+  <!-- Overlay of broadcasterset to get our panels in -->
>+  <broadcasterset id="mainBroadcasterSet">

This is a surprisingly stupid name...
Not your fault and no way to fix it - I just wanted to remark this. ;-)

Comment 8

[Manuel Reimer]

Attachment: Second patch

Second patch fixed according to your suggestions.

This also contains a fix, which caused sidebar to be broken if someone deleted a sidebar tab from RDF with an old SeaMonkey release.

Comment 9

[Manuel Reimer]

Attachment: Patch to drop local-panels.rdf

This patch should be checked in together with the final patch, fixing this issue, to drop local-panels.rdf. It is not longer required, as it gets replaced by the new broadcasters.

Comment 10

[Karsten Düsterloh]

Comment on attachment 485700 [details] [diff] [review]
Patch to drop local-panels.rdf

You also need to 'hg remove' local-panels.rdf, which will show up in the patch then.

And since the entities are now part of the sidebarOverlay.xul directly, you should move the content of local-panels.dtd into sidebarOverlay.dtd and  completely remove local-panels.dtd as well.

Comment 11

[Karsten Düsterloh]

Comment on attachment 485697 [details] [diff] [review]
Second patch

>+++ b/suite/common/sidebar/customize.js
>+    // Check, if the item is in the available list (RDF copy of broadcasters).

How about: "Check if the item is one of the new broadcaster panels in the mainBroadcasterSet."?

>+++ b/suite/common/sidebar/sidebarOverlay.js
>+  const translate = {viewBookmarksSidebar:   "bookmarks",
>+                     viewHistorySidebar:     "history",
>+                     viewSearchSidebar:      "search",
>+                     viewAddressbookSidebar: "addressbook"};
>+  const URN_PREFIX = "urn:sidebar:panel:";

translate should be named TRANSLATE, then.

>+    // Item already exists, but perhaps we need update...

... need to ...

>+    if (curId.substr(0, URN_PREFIX.length) != URN_PREFIX) continue;

No one-line ifs, please.

> <!ENTITY % sidebarOverlayDTD SYSTEM "chrome://communicator/locale/sidebar/sidebarOverlay.dtd" >
> %sidebarOverlayDTD;
>+<!ENTITY % localPanelsDTD SYSTEM "chrome://communicator/locale/sidebar/local-panels.dtd" >
>+%localPanelsDTD;
> ]>

As noted in comment #10, you should roll local-panels.dtd into sidebarOverlay.dtd and remove it as well.
And please fold the file removal into this patch.

Comment 12

[Manuel Reimer]

Attachment: Third patch

I hope I didn't miss something. This patch should contain all the last suggestions.

Comment 13

[Manuel Reimer]

Attachment: Fourth patch [Checkin: comment 15]

Sorry, but I had a typo in my last patch... Fixed...

Comment 14

[Karsten Düsterloh]

Comment on attachment 489511 [details] [diff] [review]
Fourth patch [Checkin: comment 15]

r/moa=me
Thanks for your patience. :)

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 489511 [details] [diff] [review]
Fourth patch [Checkin: comment 15]

http://hg.mozilla.org/comm-central/rev/908551829690

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Leaving the status change decision to you.

Comment 17

[Manuel Reimer]

I'll mark this one "FIXED" as, what I call "the API" is added now. I'll create a separate bug for the places bookmarks stuff, which IMHO isn't part of this bug.

I already created a bug for the "Sidebar code replacement": Bug 613971

Just to say it again: The fix, I created for this bug, is just to finally have something for the addon developers in SeaMonkey 2.1. It is a *workaround* or *hack*, which makes it easier for addon developers to get their sidebars into SeaMonkey, but doesn't make the messy sidebar code of SeaMonkey better in any way.

Comment 18

[Manuel Reimer]

Bug for the "places bookmark stuff" is this one: Bug 613974
Feel free to add you as CC to one of those bugs if you are interested in them.