326814 - Port TB's dynamic 3-pane generation to mailnews

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Ian Neal]

Back in 2003 TB switched to using dynamic generation of 3-pane window and taking this will help us have some other features that rely on this (e.g. folder tooltips).

Comment 1

[Karsten Düsterloh]

Dupe of bug 105542 whose fix got checked in for Mozilla on 2004-11-26?
Or what are you after?

Comment 2

[Ian Neal]

Attachment: Provisional Patch v0.1

This patch:
* Ports TB changes to remove mail3PaneWindowVertLayout.xul
* Ports TB's use of decks for messenger.xul - will help lightning work with mailnews

Comment 3

[Karsten Düsterloh]

Comment on attachment 211779 [details] [diff] [review]
Provisional Patch v0.1

>--- base/resources/content/mail3PaneWindowVertLayout.xul	2 Nov 2005 00:46:27 -0000	1.110
>+++ base/resources/content/mail3PaneWindowVertLayout.xul	12 Feb 2006 23:49:05 -0000
>@@ -1,190 +0,0 @@

Yeah! \o/

Comment 4

[[e-mail]]

OK, so I get the account central changes but I see no need to lumber ourselves with TB's poorer layout generation code...

Comment 5

[Ian Neal]

Attachment: Revised patch v0.2

Changes since v0.1:
* Tried to make UpdateMailPaneConfig function more like mailnews version again
* Fixed aw-done.js not to check for mail3PaneWindowVertLayout.xul
* Fixed XRemoteService.cpp similar to nsMessengerBootstrap.cpp / nsNntpService.cpp
* Removed other references to mail3PaneWindowVertLayout.xul in contents/manifest files

Comment 6

[Ian Neal]

Attachment: Deck only patch v0.2

Changes since patch v0.2:
* Deck only version of patch

Comment 7

[[e-mail]]

Comment on attachment 214023 [details] [diff] [review]
Deck only patch v0.2

>+        <deck id="displayDeck" flex="1" selectedIndex="0"
>+              onselect="ObserveDisplayDeckChange(event)">
Since it's the deck that is next to the splitter, it needs to have all the height settings. Alternatively you might find it easier to put the deck inside the threadPaneBox. Also note that deck children don't need flex.

>+          <vbox id="threadPaneBox" orient="horizontal" flex="1"
>+                minheight="100" height="100" persist="height">
<vbox orient="horizontal"> is just plain silly. I don't think the orient gets changed anywhere, so you can just make this an hbox.

>+// When AccountCentral is shown via the displayDeck, we need to switch the
>+// displayDeck to show the accountCentralBox, collapse all the other
>+// UI elements that aren't meaningful for AccountCentral, and finally
>+// load the iframe in the AccountCentral box with corresponding page.
Not true, HidingThreadPane should collapse the elements that are only meaningful to the thread pane. In particular the Showing/Hiding functions should be a matched pair for example enabling/disabling the F8 key.

>+    if (!threadPaneSplitter.hidden && threadPaneSplitter.getAttribute("state") != "collapsed")
>+      GetMessagePane().collapsed = false;
Wrong indentation; this file uses 4 spaces.

>+    var deck = document.getElementById("displayDeck");
>+    var nowSelected = null;
>+    try { nowSelected = deck.selectedPanel.id; } catch (ex) { }
I'm not too keen on this, perhaps var selectedPanel = document.getElementById("displayDeck").selectedPanel; and either use nowSelected = selectedPanel ? selectedPanel.id : ""; or compare against document.getElementById("threadPaneBox");

Comment 8

[Ian Neal]

Attachment: Revised deck only patch v0.3

Changes since v0.2:
* Removed collapsed="true" from accountCentralBox
* Moved height settings to deck
* Removed flex from deck children
* Changed vbox orient="horizontal" to hbox
* Moved manipulation of threadpane elements to threadpane functions from accountcentral functions
* Rewrote comment for Thread Pane / Account Central
* Sorted out indentation
* Removed try from ObserveDisplayDeckChange as suggested
* Fixed issue with switching from account central for one to account to account central for another account
* Fixed issue with reopening mail window after closing with account central loaded - message pane was always collapsed and required two F8s to show.

Comment 9

[[e-mail]]

Comment on attachment 214128 [details] [diff] [review]
Revised deck only patch v0.3

> function ShowAccountCentral()
> {
>+    if (document.getElementById("displayDeck").selectedPanel == accountCentralBox)
>+        ShowingAccountCentral(); // force us to reload account central because the user has switched accounts
>+    else
>+        document.getElementById("displayDeck").selectedPanel = accountCentralBox;
>+}
Well, this looks hacky, but unfortunately so do my other two ideas.

The first idea is to remove the selectedIndex attribute on the display deck before setting the selected panel. This forces the select event to be sent:
var displayDeck = document.getElementById("displayDeck");
displayDeck.removeAttribute("selectedIndex");
displayDeck.selectedPanel = accountCentralBox;

The second idea is in two parts. Since hiding account central doesn't need to unload it, showing account central doesn't need to load it. Only ShowAccountCentral needs to:
document.getElementById("displayDeck").selectedPanel = accountCentralBox;
accountCentralPane.location.href = pref.getComplexValue("mailnews.account_central_page.url", Components.interfaces.nsIPrefLocalizedString).data;

Then to deal with the thread pane on startup issue, simply make account central the first pane in the XUL so that ShowingThreadPane() always gets called.

>+    if (document.getElementById("displayDeck").selectedPanel = threadPaneBox)
s/=/==/. r=me with this fixed.

Comment 10

[[e-mail]]

(In reply to comment #9)
>The second idea is in two parts.
Actually I'm warming to this idea. Can you give it a whirl please?

Comment 11

[Ian Neal]

Attachment: Tweaked deck only patch v0.3a

Changes since v0.3:
* Swapped order of elements in deck as per comment
* Moved setting of account central page to ShowAccountCentral

Carrying forward r=

Comment 12

[Ian Neal]

Attachment: Inside try deck only patch v0.3b (Checked in trunk)

Changes since v0.3a:
* Moved first line of ShowAccountCentral into try block for neatness
* Removed unneeded ShowThreadPane() from msgMail3PaneWindow.js

Carrying forward r=

Comment 13

[Ian Neal]

Comment on attachment 214250 [details] [diff] [review]
Inside try deck only patch v0.3b (Checked in trunk)

Checking in (trunk)
commandglue.js;
new revision: 1.265; previous revision: 1.264
mail3PaneWindowVertLayout.xul;
new revision: 1.111; previous revision: 1.110
mailWindow.js;
new revision: 1.104; previous revision: 1.103
messenger.xul;
new revision: 1.263; previous revision: 1.262
msgMail3PaneWindow.js;
new revision: 1.289; previous revision: 1.288
done

Comment 14

[Ian Neal]

Attachment: Regression fix due to deck change patch v0.3c

This patch:
* Fixes the regression introduced when deck order was switched (bug 330198) - because AccountCentral page is now the first item in the deck, ShowAccountCentral was no longer calling HidingThreadPane and ShowingAccountCentral, so change to calling these explicitly.

Comment 15

[Ian Neal]

Comment on attachment 214844 [details] [diff] [review]
Regression fix due to deck change patch v0.3c

Cancelling old request

Comment 16

[Ian Neal]

Attachment: Combined deck only patch for 1.8 branch v0.3d (Checked in 1.8 branch)

This patch for 1.8 branch:
* Combines trunk patch v0.3b with both patches from Bug 330198

Carrying forward r/sr

Comment 17

[Ian Neal]

Comment on attachment 217704 [details] [diff] [review]
Combined deck only patch for 1.8 branch v0.3d (Checked in 1.8 branch)

Requesting approval for SM1.1 branch - patch will enable us to eventually use lightning on SM1.1

Comment 18

[Robert Kaiser]

Comment on attachment 217704 [details] [diff] [review]
Combined deck only patch for 1.8 branch v0.3d (Checked in 1.8 branch)

I think this can only be good for us to have :)

a=me

Comment 19

[Ian Neal]

Comment on attachment 217704 [details] [diff] [review]
Combined deck only patch for 1.8 branch v0.3d (Checked in 1.8 branch)

Checking in (1.8 branch)
commandglue.js;
new revision: 1.258.4.7; previous revision: 1.258.4.6
mail3PaneWindowVertLayout.xul;
new revision: 1.106.2.5; previous revision: 1.106.2.4
mailWindow.js;
new revision: 1.100.2.3; previous revision: 1.100.2.2
mailWindowOverlay.xul;
new revision: 1.298.2.12; previous revision: 1.298.2.11
messenger.xul;
new revision: 1.258.2.5; previous revision: 1.258.2.4
msgMail3PaneWindow.js;
new revision: 1.282.2.7; previous revision: 1.282.2.6
done