Closed
Bug 326814
Opened 15 years ago
Closed 15 years ago
Port TB's dynamic 3-pane generation to mailnews
Categories
(SeaMonkey :: MailNews: Message Display, enhancement)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: iann_bugzilla, Assigned: iann_bugzilla)
References
Details
(Keywords: fixed-seamonkey1.1a)
Attachments
(2 files, 6 obsolete files)
11.41 KB,
patch
|
iann_bugzilla
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
18.62 KB,
patch
|
iann_bugzilla
:
review+
iann_bugzilla
:
superreview+
kairo
:
approval-seamonkey1.1a+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Dupe of bug 105542 whose fix got checked in for Mozilla on 2004-11-26? Or what are you after?
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•15 years ago
|
||
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•15 years ago
|
||
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)
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)
Attachment #213130 -
Flags: review?(neil)
Changes since patch v0.2: * Deck only version of patch
Attachment #213130 -
Attachment is obsolete: true
Attachment #214023 -
Flags: review?(neil)
Comment 7•15 years ago
|
||
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-
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 9•15 years ago
|
||
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+
Comment 10•15 years ago
|
||
(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?
Assignee | ||
Comment 11•15 years ago
|
||
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+
Assignee | ||
Comment 12•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #214250 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 13•15 years ago
|
||
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)
Assignee | ||
Comment 14•15 years ago
|
||
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)
Assignee | ||
Comment 15•15 years ago
|
||
Comment on attachment 214844 [details] [diff] [review] Regression fix due to deck change patch v0.3c Cancelling old request
Attachment #214844 -
Flags: review?(neil)
Assignee | ||
Comment 16•15 years ago
|
||
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+
Assignee | ||
Comment 17•15 years ago
|
||
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 18•15 years ago
|
||
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+
Assignee | ||
Comment 19•15 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•