Last Comment Bug 399310 - Add Firefox broadcaster-based Sidebar API to SeaMonkey Sidebar
: Add Firefox broadcaster-based Sidebar API to SeaMonkey Sidebar
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Sidebar (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.1b2
Assigned To: Manuel Reimer
:
Mentors:
Depends on: 616110 639212
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-10 05:53 PDT by Manuel Reimer
Modified: 2011-03-06 12:57 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
wanted


Attachments
First Patch (8.72 KB, patch)
2007-10-10 05:56 PDT, Manuel Reimer
no flags Details | Diff | Splinter Review
"quick and dirty" fix (15.74 KB, patch)
2010-10-05 12:51 PDT, Manuel Reimer
mnyromyr: review-
Details | Diff | Splinter Review
Second patch (15.80 KB, patch)
2010-10-25 04:35 PDT, Manuel Reimer
mnyromyr: review-
Details | Diff | Splinter Review
Patch to drop local-panels.rdf (1.62 KB, patch)
2010-10-25 04:42 PDT, Manuel Reimer
mnyromyr: review-
Details | Diff | Splinter Review
Third patch (26.19 KB, patch)
2010-11-10 08:38 PST, Manuel Reimer
no flags Details | Diff | Splinter Review
Fourth patch [Checkin: comment 15] (26.19 KB, patch)
2010-11-10 08:42 PST, Manuel Reimer
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Splinter Review

Description Manuel Reimer 2007-10-10 05:53:34 PDT
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 2007-10-10 05:56:16 PDT
Created attachment 284296 [details] [diff] [review]
First Patch

Moving the sidebar API code to here, to simplify the "code cleanup" patch
Comment 2 Robert Kaiser 2010-09-04 07:22:50 PDT
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 2010-09-07 08:26:30 PDT
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 2010-09-07 08:41:47 PDT
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) (Away until Aug 29) 2010-09-07 19:58:13 PDT
(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 2010-10-05 12:51:43 PDT
Created attachment 481007 [details] [diff] [review]
"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 2010-10-12 15:12:40 PDT
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 2010-10-25 04:35:15 PDT
Created attachment 485697 [details] [diff] [review]
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 2010-10-25 04:42:02 PDT
Created attachment 485700 [details] [diff] [review]
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 2010-11-06 09:11:33 PDT
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 2010-11-06 09:45:52 PDT
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 2010-11-10 08:38:28 PST
Created attachment 489510 [details] [diff] [review]
Third patch

I hope I didn't miss something. This patch should contain all the last suggestions.
Comment 13 Manuel Reimer 2010-11-10 08:42:03 PST
Created attachment 489511 [details] [diff] [review]
Fourth patch [Checkin: comment 15]

Sorry, but I had a typo in my last patch... Fixed...
Comment 14 Karsten Düsterloh 2010-11-10 12:08:24 PST
Comment on attachment 489511 [details] [diff] [review]
Fourth patch [Checkin: comment 15]

r/moa=me
Thanks for your patience. :)
Comment 15 Jens Hatlak (:InvisibleSmiley) 2010-11-17 10:03:33 PST
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) 2010-11-17 10:04:15 PST
Leaving the status change decision to you.
Comment 17 Manuel Reimer 2010-11-22 08:26:32 PST
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 2010-11-22 08:35:54 PST
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.

Note You need to log in before you can comment on or make changes to this bug.