289483 - use <deck> to permit extensions to tbird display model

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mike Shaver (:shaver emeritus)]

I've got a patch that does two main things:

- use a <deck> to wrap the threadPane and accountCentral boxes
- use the onselect notifications from that deck to drive the collapsing or
  showing of other UI elements, like the messagePane, searchBox, etc.

I've tested it lightly, seems to do all the things I expect and none I don't. 
An extension which adds a new deck-child and causes it to be selected in the
same way (showing when they are picked, hiding when they are not) works as I
would expect as well, so I'm going to attach it here for review and feedback!

I'd really, really like to get this into Tbird 1.1a, so that we have the ability
to ship test builds of lightning against a more solid target than the nightly
trunk, if possible.

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: Make a deck (threadPane, accountCentral) the central pivot for UI mode changes

There are some other light cleanups in here, mainly removing the never-possible
FakeAccount call, and renaming HideAccountCentral to ShowThreadPane, since
that's really what the function does (and that it had to hide the
accountCentralBox was just a quirk of the implementation).

Comment 2

[Mike Cowperthwaite]

This isn't Mac-specific, is it?

Have you removed all the calls to HideAccountCentral() and 
HideFakeAccountPage() ?  If so, shouldn't you remove their definitions as well?

I assume you'll remove the dump() statements...?

Any idea why Bonsai Watch is showing a check-in for this bug?

Comment 3

[Mike Shaver (:shaver emeritus)]

No, it's not Mac specific; that's just annoying Bugzilla behaviour that I can't
suppress.

Do you see other calls to HideAccountCentral in Thunderbird?  I don't and I
don't see a lingering definition.  I never found _any_ definition of
HideFakeAccountPage; I'd be happy to be pointed to one, though, if grep and lxr
are both lying to me.

I might remove the dump calls; they're pretty handy for diagnosing state-change
issues, and I suspect there will be a follow-on bug or two -- do you have a
reason to object to them?  They are low frequency, though I could condense them
to a single line without too much difficulty.

Bonsai Watch was probably triggered by the fact that I referenced the bug number
in a checkin in Lightning, but you might do better either a) reading Bonsai
output for the bug number, or b) asking the Bonsai Watch people if you want to
find out.

Comment 4

[Mike Cowperthwaite]

(In reply to comment #3)
> Do you see other calls to HideAccountCentral in Thunderbird?  

I was only looking at the patch, not at the source.  You're certainly more on 
top of it than I am.


> do you have a reason to object to them?

Not necessarily -- I now vaguely recall that dump only generates output if 
you're set up for it (right?); my only reason to object would be for output in 
the JS console for generic installs.

Comment 5

[Scott MacGregor]

I get JS errors in the console when I try to dynamically change the pane layout
and then displaying a message:

displayDeck changed!
JavaScript error: chrome://messenger/content/mailWindow.js, line 547: deck.selec
tedPanel has no properties

also I noticed several dump statements in the patch which should be removed
before checking in.

Comment 6

[Scott MacGregor]

quick search doesn't seem to work for me either with this patch but maybe that's
just because of the JS error reported above.

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 179987 [details] [diff] [review]
Make a deck (threadPane, accountCentral) the central pivot for UI mode changes

I see that selectedPanel error now, and see how to fix it; I'll put a new patch
up tomorrow.  I mismodeled the way the pane switch mutates the box layout,
apparently, because my diagrams don't match what DOMi shows me.  Thanks.

Comment 8

[Mike Shaver (:shaver emeritus)]

Attachment: Fixed view-switch and cleaned up dumps

event.target in the deck-switch observer wasn't always what I expected it to
be, though I'm still not really sure why.  Using document.getElementById() in
there made a lot of things better.  I sometimes get an error (reloading the db
view is returning NS_ERROR_FAILURE) when switching to wide view with a message
selected and quick-search active, but not always, and I'm not entirely
convinced that this patch causes it.

Quick search works a charm now, too.

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 180504 [details] [diff] [review]
Fixed view-switch and cleaned up dumps

This breaks the standalone message window, I just discovered.

Comment 10

[Mike Shaver (:shaver emeritus)]

Attachment: Fixed messageWindow.xul

I dunno why I added a ShowThreadPane call to messageWindow.js's delayedOnLoad,
but it sure wasn't ever going to do anything useful.  All better now, though.

Comment 11

[Scott MacGregor]

Comment on attachment 180796 [details] [diff] [review]
Fixed messageWindow.xul

looks good Mike, although I haven't had a chance to test the new version yet.
Thanks for the good stuff.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 180796 [details] [diff] [review]
Fixed messageWindow.xul

a=caillon

Comment 13

[Mike Shaver (:shaver emeritus)]

FIXED on trunk, thanks for reviews!