288366 - Port Thunderbird's View Layout to MailNews

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ian Neal]

Back on 2003-08-11 20:12 Thunderbird got a Layout switcher under its View menu,
it would be nice to port that.

Comment 1

[Ian Neal]

Attachment: Provisional patch v0.1

This patch does the equivalent of the one that was checked in for TB in 2003.

Comment 2

[Ian Neal]

Attachment: Tweaked Patch v0.1a

Changes from v0.1
* Only adds strings to messages.dtd leaves unimplemented/obsolete strings alone

Comment 3

[[e-mail]]

Comment on attachment 179121 [details] [diff] [review]
Tweaked Patch v0.1a

>+function InitViewLayoutStyleMenu()
>+{
>+  var paneConfig = pref.getIntPref("mail.pane_config.dynamic");
Old profiles don't have this preference. Use GetMailPaneConfig() instead.

>+  switch (paneConfig) 
>+  {
>+    case kClassicMailLayout:
>+      id ="messagePaneClassic";
>+      break;
>+    case kWideMailLayout:	
>+      id = "messagePaneWide";
>+      break;
>+    case kVerticalMailLayout:
>+      id = "messagePaneVertical";
>+      break;
>+  }
>+
>+  var layoutStyleMenuitem = document.getElementById(id);
Well this is a bit of an ugly way of doing this. One alternative would be to
put the ids in an array i.e. ["messagePaneClassic", "messagePaneWide",
"messagePaneVertical"] or another alternative would be to note that the
menuitems are the first three items in the popup so that
event.target.childNodes[paneConfig] would work (assuming you passed in the
event).

>+function ChangeMailLayout(newLayout)
>+{
>+  gPrefBranch.setIntPref("mail.pane_config.dynamic", newLayout);
>+  return true;
>+}
You don't actually need to return true here.

>-          <menuseparator id="menu_showSearch_showMessage_Separator"/>
You didn't remove the code in HideMenus that refers to this.

>-          <menuitem id="menu_showMessagePane"
>-           type="checkbox"
>-           label="&showMessagePaneCmd.label;"
>-           accesskey="&showMessagePaneCmd.accesskey;"
>-           key="key_toggleMessagePane"
>-           oncommand="MsgToggleMessagePane();"/>
>-          <menuitem id="menu_showFolderPane"
>-           type="checkbox"
>-           label="&showFolderPaneCmd.label;"
>-           accesskey="&showFolderPaneCmd.accesskey;"
>-           key="key_toggleFolderPane"
>-           oncommand="MsgToggleSplitter('gray_vertical_splitter');"/>
Now you're moving these to a menu that you're already hiding you can remove the
code in HideMenus that hides them.

>+  var viewLayoutMenu = document.getElementById("menu_MessagePaneLayout");
>+  if (viewLayoutMenu)
>+    viewLayoutMenu.setAttribute("hidden", "true");
[Aside: if all these menuitems observed a broadcaster we could set them all to
hidden in XUL instead]

> <!-- Go Menu -->
>-
:-P

Comment 4

[Ian Neal]

Attachment: Revised Patch v0.1c

Changes since v0.1a
* Made use of GetMailPaneConfig() and event.target.childNodes[paneConfig] for
layout switching plus other tidy-ups as suggested by Neil
* Used a broadcaster to hide menus as suggested by Neil
* Removes unused function UpdateMailPaneConfigMenu from msgMail3PaneWindow.js
as suggested by Neil on IRC

There is possible scope for using another broadcaster for disabled menus when
account manager is open - see view_init function in mailWindowOverlay.js - but
that is probably for another bug

Comment 5

[[e-mail]]

Comment on attachment 179339 [details] [diff] [review]
Revised Patch v0.1c

>+function ChangeMailLayout(newLayout)
>+{
>+  gPrefBranch.setIntPref("mail.pane_config.dynamic", newLayout);
>+  return;
>+}
What I meant was that the return statement was unnecessary...

>+<broadcasterset id="mailBroadcasters">
>+  <broadcaster id="mailHideMenus" hidden="false"/>
>+</broadcasterset>
You shouldn't need to specify hidden here as it gets copied to all the
menuitems which you don't need for the 3 pane views. It might even be neater to
specify the broadcaster with hidden="true" in messageWindow.xul and without
hidden in the two 3 pane files.

>+                observes="cmd_renameFolder">
>+        <observes element="mailHideMenus" attribute="hidden"/>
In theory we should be able to switch these menuitems to command= which is
cheaper for menuitems and keys, and also allows you to use the observes
attribute.

Comment 6

[Ian Neal]

Attachment: Extended Patch v0.1e

Changes since v0.1c
* Removed return statement from ChangeMailLayout function
* Moved broadcaster out of mailWindowOverlay.xul and into the 3 views
* Added a new broadcaster to disable F8/F9 keys in messageWindow.xul
  - gets rid of MsgToggleMessagePane and MsgToggleSplitter errors in JS console

* Change all observes="cmd..." to command="cmd..."
* Change all <observes element="mail..." into simple observes="mail..."

Comment 7

[[e-mail]]

Comment on attachment 179636 [details] [diff] [review]
Extended Patch v0.1e

>+  <broadcaster id="mailHideKeys"/>
Disable :-P

Comment 8

[Ian Neal]

Attachment: Tweaked extended patch v0.1f (Checked in)

Changes since v0.1e
* Changed broadcaster name from mailHideKeys to mailDisableKeys as suggested by
Neil (I think)

Carrying forward r=

Comment 9

[Ian Neal]

Comment on attachment 179717 [details] [diff] [review]
Tweaked extended patch v0.1f (Checked in)

Checking in content/mail3PaneWindowVertLayout.xul;
/cvsroot/mozilla/mailnews/base/resources/content/mail3PaneWindowVertLayout.xul,
v  <--	mail3PaneWindowVertLayout.xul
new revision: 1.104; previous revision: 1.103
done
Checking in content/mailWindowOverlay.js;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v  <-- 
mailWindowOverlay.js
new revision: 1.217; previous revision: 1.216
done
Checking in content/mailWindowOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v  <-- 
mailWindowOverlay.xul
new revision: 1.290; previous revision: 1.289
done
Checking in content/messageWindow.js;
/cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v  <-- 
messageWindow.js
new revision: 1.103; previous revision: 1.102
done
Checking in content/messageWindow.xul;
/cvsroot/mozilla/mailnews/base/resources/content/messageWindow.xul,v  <-- 
messageWindow.xul
new revision: 1.79; previous revision: 1.78
done
Checking in content/messenger.xul;
/cvsroot/mozilla/mailnews/base/resources/content/messenger.xul,v  <-- 
messenger.xul
new revision: 1.256; previous revision: 1.255
done
Checking in content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mailnews/base/resources/content/msgMail3PaneWindow.js,v  <-- 
msgMail3PaneWindow.js
new revision: 1.277; previous revision: 1.276
done
Checking in locale/en-US/messenger.dtd;
/cvsroot/mozilla/mailnews/base/resources/locale/en-US/messenger.dtd,v  <-- 
messenger.dtd
new revision: 1.197; previous revision: 1.196
done

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using trunk build 2006-01-4-09 under Windows XP.

I checked; all 3 layouts (Classic, Wide, Vertical) are present and work correctly, as do the Message Pane and Folder Pane toggle options.