Add Firefox broadcaster-based Sidebar API to SeaMonkey Sidebar

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
Sidebar
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Manuel Reimer, Assigned: Manuel Reimer)

Tracking

Trunk
seamonkey2.1b2
Dependency tree / graph

Firefox Tracking Flags

(blocking-seamonkey2.1 -, seamonkey2.1 wanted)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

10 years ago
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
(Assignee)

Updated

10 years ago
Depends on: 396374
(Assignee)

Comment 1

10 years ago
Created attachment 284296 [details] [diff] [review]
First Patch

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

Updated

10 years ago
Assignee: sidebar → Manuel.Spam
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: sidebar
Version: unspecified → Trunk
Blocks: 467530

Comment 2

7 years ago
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.

Updated

7 years ago
blocking-seamonkey2.1: --- → ?
status-seamonkey2.1: --- → ?
(Assignee)

Comment 3

7 years ago
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

7 years ago
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.
(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.
blocking-seamonkey2.1: ? → -
status-seamonkey2.1: ? → wanted
(Assignee)

Comment 6

7 years ago
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.
Attachment #284296 - Attachment is obsolete: true
Attachment #481007 - Flags: review?(mnyromyr)

Comment 7

7 years ago
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. ;-)
Attachment #481007 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 8

7 years ago
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.
Attachment #481007 - Attachment is obsolete: true
Attachment #485697 - Flags: review?(mnyromyr)
(Assignee)

Comment 9

7 years ago
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.
Attachment #485700 - Flags: review?(mnyromyr)
(Assignee)

Updated

7 years ago
No longer depends on: 396374
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED

Comment 10

7 years ago
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.
Attachment #485700 - Flags: review?(mnyromyr) → review-

Comment 11

7 years ago
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.
Attachment #485697 - Flags: review?(mnyromyr) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 489510 [details] [diff] [review]
Third patch

I hope I didn't miss something. This patch should contain all the last suggestions.
Attachment #485697 - Attachment is obsolete: true
Attachment #485700 - Attachment is obsolete: true
Attachment #489510 - Flags: superreview?(mnyromyr)
Attachment #489510 - Flags: review?(mnyromyr)
(Assignee)

Comment 13

7 years ago
Created attachment 489511 [details] [diff] [review]
Fourth patch [Checkin: comment 15]

Sorry, but I had a typo in my last patch... Fixed...
Attachment #489510 - Attachment is obsolete: true
Attachment #489511 - Flags: superreview?(mnyromyr)
Attachment #489511 - Flags: review?(mnyromyr)
Attachment #489510 - Flags: superreview?(mnyromyr)
Attachment #489510 - Flags: review?(mnyromyr)

Comment 14

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

r/moa=me
Thanks for your patience. :)
Attachment #489511 - Flags: superreview?(mnyromyr)
Attachment #489511 - Flags: superreview+
Attachment #489511 - Flags: review?(mnyromyr)
Attachment #489511 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Comment on attachment 489511 [details] [diff] [review]
Fourth patch [Checkin: comment 15]

http://hg.mozilla.org/comm-central/rev/908551829690
Attachment #489511 - Attachment description: Fourth patch → Fourth patch [Checkin: comment 15]
Leaving the status change decision to you.
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.1b2
(Assignee)

Comment 17

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

7 years ago
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.
(Assignee)

Updated

7 years ago
Summary: Add Firefox Sidebar API to SeaMonkey Sidebar → Add Firefox broadcaster-based Sidebar API to SeaMonkey Sidebar
No longer blocks: 467530
Depends on: 616110

Updated

6 years ago
Depends on: 639212
You need to log in before you can comment on or make changes to this bug.