639212 - Fix sidebar so it works with editor/messagecompose [bset is null]

Status: RESOLVED FIXED

(SeaMonkey :: Sidebar, defect)

Description

[Ian Neal]

Attachment: Add broadcaster set to files v1.0 [Checked in: Comment 1]

At the moment when you try and open the sidebar in an editor or message compose window it does not work properly and you get a message in the error console:
bset is null
chrome://communicator/content/sidebar/sidebarOverlay.js
Line:  1668

This patch adds the missing broadcasterset to the relevant files to fix the issue.

Comment 1

[Ian Neal]

Comment on attachment 517172 [details] [diff] [review]
Add broadcaster set to files v1.0 [Checked in: Comment 1]

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

Comment 2

[Manuel Reimer]

In my opinion this fix is a bad idea.

Let me explain how the "broadcaster based" API is implemented in SeaMonkey in short points:
- Browser starts
- Sync code reads broadcasters
- Broadcasters, which exist in browser, are added to panels.rdf
- Broadcasters, not longer there but still in panels.rdf are killed

So this is a "one direction" sync from broadcaster to panels.rdf.

What you did is to create an empty broadcasterset to mail/news. So if you now open a new mail window with a sidebar enabled (should not be the default, but Mnenhy implements this), then the sync code runs again and drops *all* sidebars. If you switch back to a browser, all panels should be gone.

Problem here is that everything sits on the one and only "panels.rdf" file. You can't have different configuration between different applications and you can't enable the old sidebar for mail/news in the way it currently is designed!

Could someone please verify, that the possible regression, I noted above, isn't existent? If it exists, another fix is needed, which backs out the sync process anywhere but in the browser scripts. Means: No broadcasters from mail/news should be imported. This results in chaos!

Again: This whole sync stuff is *crap*. The whole sidebar has to be replaced as soon as possible!

Reopening to get this back on the radar of all affected people.

Comment 3

[Manuel Reimer]

As I'm sure that this will cause problems, I'll come up with a fix (quitting my sync code if not executed in navigator.xul) later this day.

Comment 4

[Manuel Reimer]

Attachment: Patch to limit broadcaster sync to the browser component [Checkin: comment 19]

This patch modifies the sync function to exit if not called in browser context.

Currently anything works well, but this *will* change if the first addon adds a broadcaster only to the browser and not to mail and composer. In this case we have the situation, I described in my last comment.

Comment 5

[Robert Kaiser]

Comment on attachment 518746 [details] [diff] [review]
Patch to limit broadcaster sync to the browser component [Checkin: comment 19]

2.1b2 has shipped, and no approval needed for b3 at this time.

Comment 6

[Ian Neal]

(In reply to comment #4)
> Created attachment 518746 [details] [diff] [review]
> Patch to limit broadcaster sync to the browser component
> 
> This patch modifies the sync function to exit if not called in browser context.
> 
> Currently anything works well, but this *will* change if the first addon adds a
> broadcaster only to the browser and not to mail and composer. In this case we
> have the situation, I described in my last comment.

What happens if an addon adds a broadcaster for mail and not browser?

Comment 7

[Manuel Reimer]

> What happens if an addon adds a broadcaster for mail and not browser?

Without my patch, opening mail/news will cause this broadcaster to be added to the sidebar, but it will be displayed in *all* applications! Starting SeaMonkey again with the browser as first application will kill the sidebar again until it reappears in all applications with the first open of mail/news.

With my patch, broadcasters in mail are just ignored, which fixes this ugly behaviour. In my opinion noone will create mailer sidebars, as this would mean he will create a sidebar addon just for SeaMonkey, as Thunderbird has no sidebar.

Making it possible to have different sidebar settings in browser, mail and composer would mean a much more complex sync code. At least my vision for SeaMonkey is to make it a real application suite, meaning that in far future it should be possible to have mail and browser tabs in the one and only "suite" window.

For me the whole old sidebar code is subject for replacement. At least it is nothing where it's worth to invest more hours of time to make it even more complex. Feel free to create a new bug with subject "broadcasters in mail/news shouldn't be ignored" and depend it on Bug 613971

Comment 8

[Ian Neal]

Comment on attachment 518746 [details] [diff] [review]
Patch to limit broadcaster sync to the browser component [Checkin: comment 19]

I think Karsten is a better person to confirm this change as he is the owner

Comment 9

[[e-mail]]

Wouldn't it be easier to
a) back out attachment 517172 [details] [diff] [review]
b) rename the broadcasterset that sync uses (bug 576970)
   (to stop Mnenhy from breaking the sidebar)
c) null-check bset?

Comment 10

[Manuel Reimer]

It wouldn't stop addons to create that broadcaster. For example if Thunderbird uses a "mainBroadcasterSet" and someone ports over his addon to SeaMonkey, this addon will create the broadcasterset and so cause the bad effect, I described in Comment 7 to reappear.

What you described would be a good workaround for our code, completely ignoring addons. My patch implements a fix as it should be: Just ignore a "mainBroadcasterSet" anywhere but in browser.

Comment 11

[Karsten Düsterloh]

Do you have a specific testcase?

Comment 12

[Manuel Reimer]

No, and currently the problem isn't triggered, as browser, mail and composer all have the same set of sidebars.

To reproduce the problem, I'm sure anything you need is a addon which installs a sidebar into SeaMonkey via broadcaster. As there are not much of them, so far, this may be tricky ;-)

IMHO it is obvious, that a "one direction sync" to the one and only "panels.rdf" can't work if there are three sources with three possibly different sets of sidebars.

Comment 13

[Karsten Düsterloh]

So, I downloaded addon 5637 from AMO (digg sidebar) and hacked it to run in SM trunk and bind itself (only) to messenger.xul:

+++ install.rdf 2011-03-18 22:13:53.000000000 +0100
-                   em:id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}"
+                   em:id="{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}"
+++ chrome.manifest     2011-03-18 22:11:14.000000000 +0100
@@ -4 +4 @@ skin diggsidebar classic/1.0 jar:chrome/
-overlay        chrome://browser/content/browser.xul    chrome://diggsidebar/content/firefoxOverlay.xul
+overlay        chrome://messenger/content/messenger.xul        chrome://diggsidebar/content/firefoxOverlay.xul

But I cannot (re)produce the problem...

Comment 14

[[e-mail]]

Comment on attachment 518746 [details] [diff] [review]
Patch to limit broadcaster sync to the browser component [Checkin: comment 19]

I have a slight preference for tweaking the caller to check the component.

Comment 15

[[e-mail]]

Comment on attachment 517172 [details] [diff] [review]
Add broadcaster set to files v1.0 [Checked in: Comment 1]

And whichever way we check, should we then back this patch out anyway?

Comment 16

[Karsten Düsterloh]

Comment on attachment 518746 [details] [diff] [review]
Patch to limit broadcaster sync to the browser component [Checkin: comment 19]

Okay, using the hacked digg extension overlaying messenger.xul plus Mnenhy (but it should work as well with hacking digg to overlay the mail editor), I do see the problem:
1. Start SM with browser. Notice digg is not in in the browser's sidebar's list of available sidebars.
2. Open messenger. Notice digg appears in its sidebar list _and_ (now!) in the open browser's sidebar's list.
3. Open mail editor. Notice digg is not in its sidebar's list.
4. etc. pp.

Manuel's patch does alleviate this.

(In reply to comment #15)
> Comment on attachment 517172 [details] [diff] [review]
> Add broadcaster set to files v1.0 [Checked in: Comment 1]
> 
> And whichever way we check, should we then back this patch out anyway?

It's the sidebar (sync) code that is the problem, not the broadcaster.

Comment 17

[Justin Wood (:Callek)]

blocking- but feel free to go ahead and land.

Comment 18

[Manuel Reimer]

We definitively need this fix before we release or the new possibly useful feature (simpler adding of sidebars) actually breaks more than it fixes... Trying to find someone to check this in. Please mark as FIXED again after checking in. Thanks!

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 518746 [details] [diff] [review]
Patch to limit broadcaster sync to the browser component [Checkin: comment 19]

http://hg.mozilla.org/comm-central/rev/673f0abdd82a