211088 - Tabs menu does not show the installed sidebars

Status: RESOLVED FIXED

(SeaMonkey :: Sidebar, defect)

Description

[Gilles Durys]

The "tabs" menu on the sidebar does not contain any of the installed sidebar,
only "Customize sidebars" and "Sidebars directory".
The customize sidebar dialog shows all of the installed sidebars.

It was functional on a 2003061922 trunk build but not anymore on a 2003062022
trunk build.

Comment 1

[Oliver Klee]

This is a regression, not smoketest.

Comment 2

[Chase Tingley]

After a bit of poking, this doesn't look like an RDF bug.  The datasource gets
created fine.  There's a problem with templating, but I'm not sure it's in the
templating code either -- my hunch is either a js/xul bug or (more likely) a xul
issue.

The XUL that should be generating this menu is the <toolbarbutton> item at 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/resources/sidebarOverlay.xul#70
and the <menupopup> inside it.

Note that no datasource is initially set to provide data to run through the
template.  The datasource and ref are set by the JS function
SidebarBuildPickerPopup(), which is an onpopupshowing() handler.

For whatever reason, this doesn't seem to be getting called.  I verified this by
moving the code that sets datasource and ref from SidebarBuildPickerPopup() to
sidebar_overlay_init(), and everything worked fine.

I would suspect that there has been some fairly recent patch to XUL that changed
some semantic -- e.g., perhaps toolbarbutons no longer receive onpopupshowing
events.  I could probably track down the exact cause with enough time, but
someone who's been more in tune with xul checkins over the last month might know
better.  

Comment 3

[Chase Tingley]

Attachment: patch

Forcing the menu builder to rebuild solves this problem.  This is a fairly
common hack (actually, I think the semantics of requiring manual rebuild() when
you add a datasource make some sense), although there may still be a bug that
is preventing something from getting notified somewhere in xul.

I'm still not sure why this regressed when it did.

Comment 4

[Robert John Churchill]

Comment on attachment 129037 [details] [diff] [review]
patch

r=[e-mail]

Comment 5

[Chase Tingley]

I finally figured out the source of the regressin, which is bug 209634.
After adding the datasource in SidebarBuildPickerPopup(), the sidebar code
was making an (idempotent) |menu.setAttribute('ref', sidebarObj.resource);| call.
This triggered the code in nsXULTemplateBuilder::AttributeChange to force a rebuild.
The fix for 209634 filtered out this attribute update, so the menu is never rebuilt.
CC'ing bz for the heck of it; this patch should probably be updated to
remove the (now useless) setAttribute line as well.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.. didn't know about the rebuild() thing.  Could you possibly apply the same
patch to the code in pref-applications.js?  I just realized that when I patched
it in bug 213914 I forgot to patch all the callsites...

Comment 7

[Chase Tingley]

Attachment: patch v2

Removes the now-unnecessary setAttribute line.	Makes the same change to
pref-applications.js.

Comment 8

[Chase Tingley]

Taking

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 129691 [details] [diff] [review]
patch v2

What about lines 39-40 in pref-applications.js?  And I assume you tested these
changes, right?

Comment 10

[Robert John Churchill]

Comment on attachment 129691 [details] [diff] [review]
patch v2

r=[e-mail]

Comment 11

[Chase Tingley]

Ah, you're right.  I had skipped 39-40, thinking that without it ref
might never be set, but of course it's in the xul.  I'll make the 
switch there as well, and re-test (code was previously tested as well).

Comment 12

[Chase Tingley]

Attachment: v3

This changes the other instance as well.  I've tested this, and it seems fine.

Comment 13

[Chase Tingley]

Comment on attachment 129826 [details] [diff] [review]
v3

Not sure if I need to get a fresh r from rjc or not.  Can't hurt, though.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 129826 [details] [diff] [review]
v3

Excellent.

Comment 15

[Robert John Churchill]

Comment on attachment 129826 [details] [diff] [review]
v3

r=[e-mail]

Comment 16

[Chase Tingley]

Comment on attachment 129826 [details] [diff] [review]
v3

Floating for approval, just in case.  The tabs menu in the sidebar currently
displays empty. As a result of an optimization to the setAttribute() code, an
old trick to force tree rebuilds no longer works. The patch replaces it with an
explicit rebuild() to fix the tabs menu, and cleans up the same issue in the
helper apps pref panel as well.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 129826 [details] [diff] [review]
v3

a=asa (on behalf of drivers) for checkin to 1.5beta

Comment 18

[Chase Tingley]

Fix checked in.

Comment 19

[Kai de Leeuw]

This still seems partially broken, since there are no checkmarks in the tabs
meny to distinguish those sidebars that are visible from those not visible at a
given moment.

There is just a menu that shows the available sidebars, but not showing the
status of the visibility of individual sidebars.

Asking for a reopening of this bug.

Comment 20

[Kai de Leeuw]

Forgot to mention my build, it is Mozilla Suite 2003082104.

Comment 21

[Chase Tingley]

Yeah, I see that too.  Though I'm not completely convinced it's related --
manipulation of the checked attribute ought to be separate from whether or not
the content gets built.  

I've split it off into bug 217340.