Closed Bug 326814 Opened 14 years ago Closed 14 years ago

Port TB's dynamic 3-pane generation to mailnews

Categories

(SeaMonkey :: MailNews: Message Display, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

(Keywords: fixed-seamonkey1.1a)

Attachments

(2 files, 6 obsolete files)

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).
Dupe of bug 105542 whose fix got checked in for Mozilla on 2004-11-26?
Or what are you after?
Attached patch Provisional Patch v0.1 (obsolete) — Splinter Review
This patch:
* Ports TB changes to remove mail3PaneWindowVertLayout.xul
* Ports TB's use of decks for messenger.xul - will help lightning work with mailnews
Assignee: mail → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #211779 - Flags: review?(neil)
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/
OK, so I get the account central changes but I see no need to lumber ourselves with TB's poorer layout generation code...
Attachment #211779 - Flags: review?(neil)
Attached patch Revised patch v0.2 (obsolete) — Splinter Review
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
Attachment #211779 - Attachment is obsolete: true
Attachment #213130 - Flags: review?(neil)
Blocks: 313822
Attachment #213130 - Flags: review?(neil)
Attached patch Deck only patch v0.2 (obsolete) — Splinter Review
Changes since patch v0.2:
* Deck only version of patch
Attachment #213130 - Attachment is obsolete: true
Attachment #214023 - Flags: review?(neil)
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");
Attachment #214023 - Flags: review?(neil) → review-
Attached patch Revised deck only patch v0.3 (obsolete) — Splinter Review
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.
Attachment #214023 - Attachment is obsolete: true
Attachment #214128 - Flags: review?(neil)
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.
Attachment #214128 - Flags: review?(neil) → review+
(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?
Attached patch Tweaked deck only patch v0.3a (obsolete) — Splinter Review
Changes since v0.3:
* Swapped order of elements in deck as per comment
* Moved setting of account central page to ShowAccountCentral

Carrying forward r=
Attachment #214128 - Attachment is obsolete: true
Attachment #214222 - Flags: review+
Blocks: 329544
Changes since v0.3a:
* Moved first line of ShowAccountCentral into try block for neatness
* Removed unneeded ShowThreadPane() from msgMail3PaneWindow.js

Carrying forward r=
Attachment #214222 - Attachment is obsolete: true
Attachment #214250 - Flags: superreview?(bienvenu)
Attachment #214250 - Flags: review+
Attachment #214250 - Flags: superreview?(bienvenu) → superreview+
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
Attachment #214250 - Attachment description: Inside try deck only patch v0.3b → Inside try deck only patch v0.3b (Checked in trunk)
Depends on: 330198
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.
Attachment #214844 - Flags: review?(neil)
Comment on attachment 214844 [details] [diff] [review]
Regression fix due to deck change patch v0.3c

Cancelling old request
Attachment #214844 - Flags: review?(neil)
This patch for 1.8 branch:
* Combines trunk patch v0.3b with both patches from Bug 330198

Carrying forward r/sr
Attachment #214844 - Attachment is obsolete: true
Attachment #217704 - Flags: superreview+
Attachment #217704 - Flags: review+
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
Attachment #217704 - Flags: approval-seamonkey1.1a?
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
Attachment #217704 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
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
Attachment #217704 - Attachment description: Combined deck only patch for 1.8 branch v0.3d → Combined deck only patch for 1.8 branch v0.3d (Checked in 1.8 branch)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 361048
You need to log in before you can comment on or make changes to this bug.