Now that Lightning integrates into the tabbed interface in Thunderbird we might have to implement tabs in MailNews as well.

Bug 218999 - (thundertab) Thunderbird should use a tabbed interface (thundertab) Bug 392328 - [Meta] tracking bug for thunderbird tabbed interface problems Bug 313822 - Make Lightning work on SeaMonkey

The question is: does Lightning _depend_ on that tab implementation or does it just _use_ it "if it's there"?

And the difference is? If it isn't there Lightning doesn't use it => no Lightning UI visible. That's why I asked in #seamonkey if it is practicable to run the Lightning tab in a separate window like the old Calendar extension.

Having mail/news open in tabs ought to be easier for SeaMonkey than for Thunderbird, thanks to the Suite concept -- after all, we can already open chrome://messenger/content/messenger.xul (the 3-pane window) in a browser tab if we want to. (Try it: it looks fun, though there is some lost height due to the browser chrome at top.)

Tony: In a first step, we'd just port TB's way of doing tabs, to help getting Lightning up and running in SM. This, of course, is a bit orthogonal to our browser tabs and we definitely will revisit the potential in a tighter integration for SeaMonkey 3... Shyly marking as owned. ;-)

This is the first reviewable take at porting tabmail to SM. Basically, this means cloning the current tabmail.xml plus respective code in mailWindow.js/mailWindowOverlay.js/msg3PaneOverlay.js plus theme work plus additional fixes, in other words: bug 218999 + parts of fixed stuff in bug 392328/bug 362138/bug 435567/bug 464784/bug 466046/bug 468704 + fixes for broken behaviour like "toolbar status not updated on tab switch" or "open new tab menu items should behave like open new window". This patch was created under Linux, thus requesting additional Mac UI review from Stefan, before I'll dig deeper into there.

Could you wrap a <hbox id="appcontent"> around the <tabmail>. The reason I ask is that I have a Thunderbird extension that works well in TB2 but I was hard pressed to find a suitable anchor point in the TB3 messenger.xul to hang my UI off. Just as many Firefox extensions hang their UI off "appcontent" I suspect this would benefit more than just little old me.

Comment on attachment 354058 [details] [diff] [review] tabmail, v0 Minusing, because we definitely need to style those tabs on mac. It might be possible to re-use the parts of the tabbrowser-tab(s) styles (these tabs should probably be a bit smaller, though). One thing I wonder about is the close-tab button(right, we need a new one for mac - same as the tabbrowser one, I think). Right now the close button is on every tab which is sort of opposite of what we have in tabbrowser. Whatever we do, I think we should have the same close-button functionality in MailNews and tabbrowser. I think we also need a "list all tabs" button.

(In reply to comment #8) > One thing I wonder about is the close-tab button(right, we need a new one for > mac - same as the tabbrowser one, I think). Right now the close button is on > every tab which is sort of opposite of what we have in tabbrowser. I obviously need to take an in-depth stab at the various tab button pref settings and align them better with our browser tab styling. > Whatever we do, I think we should have the same close-button functionality > in MailNews and tabbrowser. Definitely. Anything else would be confusing at best. The question is: do we want to reuse our respective browser.* prefs or use TB's mail.* ones? Or both, like I already did for autoHide: use mail.tabs.autoHide if present, fall back to browser.tabs.autoHide else? > I think we also need a "list all tabs" button. Hm, that should be there already, at least it's shown under Linux. I'll wait for Ian's and Neil's comments before doing any updates.

(In reply to comment #9) > (In reply to comment #8) [...] > > Whatever we do, I think we should have the same close-button functionality > > in MailNews and tabbrowser. > > Definitely. Anything else would be confusing at best. > The question is: do we want to reuse our respective browser.* prefs or use TB's > mail.* ones? Or both, like I already did for autoHide: use mail.tabs.autoHide > if present, fall back to browser.tabs.autoHide else? [...] I think both sets should have an effect in the Suite; anything else would be confusing, and for two reasons: - Users may want different preferences for browser tabs and mailer tabs - If the Tb guys were thoughtful enough to provide a distinct set of prefs (thinking of us and of the above, maybe), sooner or later these Tb prefs are going to be documented (in the Mozillazine KB, maybe); having mail interfaces with identical widgets in Sm & Tb, but reacting to mail.* prefs in Tb and to browser.* in Sm mail would certainly confuse a lot of people. If it is desired to have mail.* prefs obey the browser.* ones when absent, it means the mail.* prefs MUST be hidden in about:config (i.e., unset by default), doesn't it?

(In reply to comment #10) > I think both sets should have an effect in the Suite; anything else would be > confusing, and for two reasons: > - Users may want different preferences for browser tabs and mailer tabs I'm not sure yet whether this is sensible at all - having tabs with close buttons in MailNews, but not in Browser seems odd. Of course, it's possible, and the cost would be rather cheap, if we don't provide a real pref panel for it. Iow: follow the browser settings, unless someone sets the hidden mail ones. > - If the Tb guys were thoughtful enough to provide a distinct set of prefs > (thinking of us and of the above, maybe), (I'd rather believe they didn't want to have "browser." in their pref tree, but that for sure isn't relevant. ;-) ) > sooner or later these Tb prefs are going to be documented (in the Mozillazine > KB, maybe); having mail interfaces with identical widgets in Sm & Tb, Only identical "as seen on tv", I had to fork the XBL. (Sharing non-toolkit widgets seems too dangerous, given the mail widgets bustage lately.) > but reacting to mail.* prefs in Tb and to browser.* in Sm mail would > certainly confuse a lot of people. People'd expect SM to have a unified pref at least, to touch all tabbrowsers at once... (I don't think implementing a third pref layer which inherits to both browser.* and mail.* would be sane, though.) > If it is desired to have mail.* prefs obey the browser.* ones when absent, it > means the mail.* prefs MUST be hidden in about:config (i.e., unset by > default), doesn't it? Yes, that's why I don't put them into the pref default files.

(In reply to comment #11) > Iow: follow the browser settings, unless someone sets the hidden mail ones. Sounds reasonable to me, esp. as I wonder if we could share between tabbrowser and tabmail in SeaMonkey in the future.

A bunch of additional fixes: - basic Mac theming (@stefan: finer details should probably go into spinoff bugs?) - based mail tab prefs onto browser tab prefs - newtab button, plus pref-controlled tabstrip mode (TB lacks this) - warn when closing more than one messenger tab (TB lacks this) - show biff status in tab icons (TB lacks this) - double click opens in tab, pref-controlled (TB lacks this) - remember message/thread pane splitter state (F8/F9) (TB lacks this) - make tabmail.xml adhere to a common coding style Don't be afraid of the size, about half of it is tabmail.xml: I made a hg copy to keep the history intact and then did fixing, refactoring and restyling - almost no two functions used the same style! Known issue: threadpane.js is still shared. This doesn't make sense and will change.

Comment on attachment 354748 [details] [diff] [review] tabmail, v1 I'm going to comment here before I actually do the review (maybe Karsten wants to put up a new version anyway). I assume it's only the mac classic css stuff that I should look at, but I'll of course have some general comments as well :-). tabmail.xml isn't exactly the same as tabbrowser.xml, so atm the tabs look different than the tabbrowser ones. For example, they're a bit higher and there's no bottom border surrounding non-selected tabs (the bottom border gives the "downwards opened" effect of the selected tab). I think we need to fix this before the release, because it looks quite bad. I'm not sure if the best solution here is to use tabbrowser.css either (and I guess the file should be re-named and moved to communicator if we decide to use it). -.tabs-bottom { +.tabs-bottom, .tabmail-tabs { border-bottom: 3px solid; -moz-border-bottom-colors: #888 rgba(0, 0, 0, 0.08); } This doesn't work very well :( tabbrowser.xml has this (tabbrowser-tabs binding): <content> <xul:stack flex="1" class="tabs-stack"> <xul:vbox> <xul:spacer flex="1"/> <xul:hbox class="tabs-bottom" align="center"/> </xul:vbox> the <stack> and the tabs-bottom hbox together with the above style rule and the .tabs-stack { margin-top -1px } rule is what makes the bottom border on the non-selected tabs. In fact, adding those elements to tabmail.xml fixed the differences with tabbrowser. Well, not that tabmail works that well with those elements added, but anyway ;-) I haven't investigated if it's doable without those elements. +/* ..... tabmail ..... */ +.tabmail-tab[type="message"] .tab-icon { + margin-top: -4px; + list-style-image: url("chrome://messenger/skin/icons/message-mail.png"); +} + +.tabs-newbutton { + list-style-image: url("chrome://navigator/skin/icons/tab-new.gif"); +} + +.tab-close-button, .tabs-closebutton { + list-style-image: url("chrome://communicator/skin/icons/close-button.gif"); +} + +.tabs-alltabs-button { + list-style-image: url("chrome://global/skin/tree/columnpicker.gif"); +} + +.tabs-alltabs-button .toolbarbutton-menu-dropmarker { + display: none; +} You should remove at least the .tabs-newbutton and the close-button style rules here because they override the tabbrowser.css style rules (but I guess all those rules are leftovers from the pre-tabbrowser.css version?) and mac uses a different close button. I'm not sure about the .tab-close-button rules, you hide the button in content/messenger.css - do you need the rule at all? General comments: 1) We need a new icon for the alltabs button - it looks a bit weird with a column picker icon ;-) 2) In the alltabs menu, you can't see which tab you currently have selected (menuitem label of selected tab should be bold, perhaps?) 3) I don't see how style rules that applies a list-style-image like ".tabmail-tab[type="message"] .tab-icon" and ".tabmail-tab[type="folder"][SpecialFolder="Inbox"] .tab-icon" are necessary (lots of these).. You should be able to just do .tabmail-tab[type="message"] { list-style-image: ... };, shouldn't you?

Some general comments from testing: 1. Each tab does not remember the individual state of its header pane: * If you expand the list of recipients, switch to another message tab and then switch back, it's back to unexpanded. * If you contract the header to its single line, it does it for all message tabs you already have open. 2. It would be useful to have some way of controlling what double clicking on a message does. 3. I am presuming pref pane and help changes will be in separate bugs? 4. At the moment it does not look like we implement things like forceHide, loadInBackground, maxOpenBeforeWarn 5. If you open a message in a new tab and then click on the new tab button, it creates a new tab with a label of "Inbox - ..." but the tab content is the same as the message you have just opened. If you click to message tab and then back to the new "Inbox - ..." tab it displays the Inbox but without the folder pane. If you click back to the message tab one more time and then back to the new "Inbox - ..." tab it is finally displaying the correct content. Probably more to come as I test more... As far as coding goes: I would be happier if we did a copy of the prefs for browser.tabs.* to mail.tabs.* on startup rather than observing/checking both sets of prefs. With the tabmail binding as it is I think we are having to re-read prefs when browser.tabs.* prefs get changed even if mail.tabs.* prefs exist.

Comment on attachment 354748 [details] [diff] [review] tabmail, v1 >-.tabbrowser-tab { >+.tabbrowser-tab, .tabmail-tab { > margin: 0; > padding: 0; > list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.png"); > border: solid transparent; > border-width: 0 2px; > -moz-appearance: none; > } > Whoops.

Comment on attachment 354748 [details] [diff] [review] tabmail, v1 So, I tried this out, and I ran into a few issues, in random importance: 1. Opening a message in a new tab from the context menu doesn't restore the selection in the thread pane in the original tab correctly. This means that when you click back in that tab, you load the message you just opened again. 2. When you switch to a tab, then the thread pane scrolls to the top, and this is only very slightly mitigated by scrolling the current message, if there is one, back into view, since it always ends up scrolled to the bottom. 3. When opening a message in its own tab, subsequent navigation does not update the tab's label. 4. The search and location bars don't update as you switch tabs. 5. When opening a message in a tab, the search and location bars are still available, but do nonintuitive things. (I'm tempted to suggest that the message window should handle opening messages in tabs.) 6. Renaming the folder splitter forgets my existing persistence. Hmm, each tab remembers the folder splitter state. Eww. Not only that, but the mail window always opens the folder pane uncollapsed, and I have to hit F9 twice. 7. Wrong tab icon for RSS/news messages. 8. No tooltips on the duplicate or close tab buttons. 9. Can't seem to open tabs in the background. 10. The browser doesn't have tab scrolling or tab menu yet. 11. Ctrl+W closes the window without even prompting! 12. No File/New/Tab menuitem. (Well, I guess it's not new...) 13. The styling for when there are no tabs looks different.

Some preliminary comments... (In reply to comment #14) > (From update of attachment 354748 [details] [diff] [review]) > I'm going to comment here before I actually do the review (maybe Karsten wants > to put up a new version anyway). I assume it's only the mac classic css stuff > that I should look at, but I'll of course have some general comments as well > :-). All comments are welcome. :) > tabmail.xml isn't exactly the same as tabbrowser.xml Yeah. :-/ I'd be happy if we had just one implementation for both, but that would probably make this patch explode. It would, otoh, bring us a good step forward (even towards SM3), because the tabmail.xml in some parts is more advanced than our tabbrowser.xml - but of course lacks in others. I generally think it'd be better to do the merging later in a different bug. I'll try to harmonize the markup, though, so that we may reuse as much styling as possible even now. > I guess the file should be re-named and moved to communicator Yes. > 1) We need a new icon for the alltabs button - it looks a bit weird with a > column picker icon ;-) Yeah, I know => followup bug. (In reply to comment #15) > 1. Each tab does not remember the individual state of its header pane: This is getting scary. (Hard to believe they deem TB's tabs to be ready for their release... :-O ) > 2. It would be useful to have some way of controlling what double clicking > on a message does. The current patch used the middleclick pref to control that (see ThreadPaneDoubleClick method). > 3. I am presuming pref pane and help changes will be in separate bugs? Yes. > 4. At the moment it does not look like we implement things like forceHide, > loadInBackground, maxOpenBeforeWarn True; fodder for the mentioned tab*.xml merge? > I would be happier if we did a copy of the prefs for browser.tabs.* to > mail.tabs.* on startup rather than observing/checking both sets of prefs. I actually want to avoid a second set of prefs if possible. > With the tabmail binding as it is I think we are having to re-read prefs when > browser.tabs.* prefs get changed even if mail.tabs.* prefs exist. Why? (In reply to comment #16) > Whoops. ??? (In reply to comment #17) > 1. Opening a message in a new tab from the context menu doesn't restore the > selection in the thread pane in the original tab correctly. This means that > when you click back in that tab, you load the message you just opened again. Which remember/scroll selection settings did you use? > (I'm tempted to suggest that the message > window should handle opening messages in tabs.) Actually, I had hoped that with a working tabmail implementation we could get rid of the standalone message window code altogether by just opening additional messenger.xul files in message mode... > 6. Renaming the folder splitter forgets my existing persistence. Well, we don't migrate them from SM1 anyway, do we? > Hmm, each tab remembers the folder splitter state. > Eww. Not only that, but the mail window always opens the folder pane > uncollapsed, and I have to hit F9 twice. I don't understand this comment, I think. The mail tabs are "fakes", they just rearrange the layout and rebase the current dbview. A message tab is just your normal layout with folderpane and threadpane hidden. While F9 works in a message tab after hitting it twice, it shouldn't at all... > 7. Wrong tab icon for RSS/news messages. Wow, I didn't even knew they had a different icon... > 10. The browser doesn't have tab scrolling or tab menu yet. See above on merging tab*.xml. > 13. The styling for when there are no tabs looks different. Different from/to what?

>> I would be happier if we did a copy of the prefs for browser.tabs.* to >> mail.tabs.* on startup rather than observing/checking both sets of prefs. > I actually want to avoid a second set of prefs if possible. I agree with stefanh. You are overloading the browser prefs unnecessarily. >> 6. Renaming the folder splitter forgets my existing persistence. > Well, we don't migrate them from SM1 anyway, do we? Every time Firefox does something like this and then decide that they need to migrate the old persistences in localstore.rdf, they write some javascript and everyone is happy. Until an unanticipated edge case crops up, so they fix this and everyone is happy again, until another edge case crops up, and then another one, and another one. In the end their migration code is a mess. I think we should use Firefox as a negative example here.

Sorry for not checking back on this before. (In reply to comment #18) > (In reply to comment #17) > > 1. Opening a message in a new tab from the context menu doesn't restore the > > selection in the thread pane in the original tab correctly. This means that > > when you click back in that tab, you load the message you just opened again. > Which remember/scroll selection settings did you use? Oh, so switching tabs forgets the selection? Makes it pointless to have two tabs open to the same folder then. > > (I'm tempted to suggest that the message > > window should handle opening messages in tabs.) > Actually, I had hoped that with a working tabmail implementation we could get > rid of the standalone message window code altogether by just opening additional > messenger.xul files in message mode... Well, it will need to improve somewhat before we can do that... > > Hmm, each tab remembers the folder splitter state. > > Eww. Not only that, but the mail window always opens the folder pane > > uncollapsed, and I have to hit F9 twice. > I don't understand this comment, I think. When I open the 3pane, the folder pane is always visible, even though I'd collapsed it before closing, and I have to hit F9 twice to hide it again. > > 10. The browser doesn't have tab scrolling or tab menu yet. > See above on merging tab*.xml. I was just thinking we should avoid too many differences between them, and implement the new features simultaneously in both windows in separate bugs. > > 13. The styling for when there are no tabs looks different. > Different from/to what? To what it is without the patch.

(In reply to comment #19) >>> I would be happier if we did a copy of the prefs for browser.tabs.* to >>> mail.tabs.* on startup rather than observing/checking both sets of prefs. >> I actually want to avoid a second set of prefs if possible. > I agree with stefanh. You are overloading the browser prefs unnecessarily. I agree too, either have a completely separate set of prefs for tabmail, or reuse the browser prefs, but not the half measure that you seem to have. >>> 6. Renaming the folder splitter forgets my existing persistence. >> Well, we don't migrate them from SM1 anyway, do we? > I think we should use Firefox as a negative example here. Yeah, that was a silly point, caused by being to used to running 2.0a builds.

(In reply to comment #21) > Yeah, that was a silly point, caused by being to used to running 2.0a builds. Bah, brain confused by too(!) many t*os ...

(In reply to comment #20) > > > 10. The browser doesn't have tab scrolling or tab menu yet. > > See above on merging tab*.xml. > I was just thinking we should avoid too many differences between them, and > implement the new features simultaneously in both windows in separate bugs. I don't see much reason though to (painfully?) strip code from the tabmail port just because tabbrowser doesn't support those things yet. We should get them to feature parity and make them as similar as possible once both are in the tree, though. (In reply to comment #21) > (In reply to comment #19) > >>> I would be happier if we did a copy of the prefs for browser.tabs.* to > >>> mail.tabs.* on startup rather than observing/checking both sets of prefs. > >> I actually want to avoid a second set of prefs if possible. > > I agree with stefanh. You are overloading the browser prefs unnecessarily. > I agree too, either have a completely separate set of prefs for tabmail, or > reuse the browser prefs, but not the half measure that you seem to have. I'd prefer just reusing the browser prefs, esp. in the light of a later merge of tab implementations.

I don't see SeaMonkey wanting special tabs for what's new, about etc in MailNews (which bug 477029 moved to a different file) - as it has a browser to display these things. Therefore removing bug 477029 from the deps list as I don't think it comes into play here.

This is in large parts a complete rewrite of the tab logic outside of tabmail.xml (hence Calendar integration shouldn't even notice it). Notable fixes in unsorted order: - I dropped TB's artificial distinction of "folder" and "message" tabs - they're just "3pane" tabs with different default layout. You're not stuck in that view, you can use F8/F9 and (new!) Shift-F8 to toggle messagepane/folderpane/threadpane visibility. Tab styling adheres to the stuff actually shown, not the default layout of tab creation. - Tabs adhere to (most) browser.tabs.* preferences, including .loadInBackground. + almost all mentioned in review comments (I hope). I also took the liberty to assemble certain pane layout functionality from all over the place near their new tabmail counterparts. I specifically did not touch the following issues, which should be split into follow-up bugs: - sync of tabbrowser.xml and tabmail.xml - implementation of browser.tabs.forceHide and other tabbrowser prefs - help documentation - folderpane twisty states persistence --- these can be *lots* and we will replace the pane by its RDF-less counterpart "soonishly" anyway I'm really sorry for the overlong delay, but there's a life outside of my cag^W^W^W... ;-)

Comment on attachment 376104 [details] [diff] [review] tabmail, v2 I've just tried the patch briefly, but I've 2 comments (already talked to Karsten about it) 1) You can't close any tab with the close-tab button if the leftmost tab is opened. 2) The icon for the show tabs button (at the right of the close-tab button) looks quite weird. I'd recommend replacing it with a down-arrow (should be possible to find one in our tree).

(In reply to comment #26) > I'd recommend replacing it with a down-arrow (should be > possible to find one in our tree). That is, some kind of triangle that points downwards.

(In reply to comment #26) > 1) You can't close any tab with the close-tab button if the leftmost tab is > opened. This is by design. Closing the last 3pane tab would destroy our mailnews functionality. But I have a fix now which allows to close any 3pane tab as long as there is one left. > 2) The icon for the show tabs button (at the right of the close-tab button) > looks quite weird. I'd recommend replacing it with a down-arrow (should be > possible to find one in our tree). I'd propose just dropping the last two .tabs-alltabs-button rules in mailwindow1.css, which will make the toolbarbutton lose its custom main icon and just display the dropdownmarker. This looks acceptable under Linux in both Classic and Modern (and is in fact what TB does). I'd like to get other comments before putting up the new patch.

Which file do I insert this code in to see the changes? I'm probably already supposed to know this, but haven't found it yet.

> Which file do I insert this code in to see the changes? I'm probably already > supposed to know this, but haven't found it yet. It's a patch, so first you need to download the source code for SeaMonkey 2.0b1pre (about 1GB). Then you apply the patch to the source using a patch manager like patch.exe. Then you build a new SeaMonkey (I assume you have a working C/C++ compiler, python, perl, mercurial, gnu make etc etc). Then you run that modified seamonkey.

Comment on attachment 376104 [details] [diff] [review] tabmail, v2 Issues noticed so far: * Can't hide the folder pane at all * Shift+F8 toggles the message pane * Two splitters when hiding the thread pane, except in Classic view

(In reply to comment #31) > Issues noticed so far: > * Can't hide the folder pane at all > * Shift+F8 toggles the message pane > * Two splitters when hiding the thread pane, except in Classic view I can't reproduce any of these issues. Which OS? Which layout? Did you try a clean profile? Do you get any related JS (strict) errors?

(In reply to comment #32) > (In reply to comment #31) > > Issues noticed so far: > > * Can't hide the folder pane at all > > * Shift+F8 toggles the message pane > > * Two splitters when hiding the thread pane, except in Classic view > I can't reproduce any of these issues. The Shift+F8 must be a VNC bug, it works fine in Windows XP. Note: I've since noticed that the splitter reverses itself when you try to hide the folder pane. > Which OS? Both Windows XP and Linux FC7. > Which layout? Any layout, except for the thread pane hiding issue. > Did you try a clean profile? Not yet. > Do you get any related JS (strict) errors? I don't know if these are related: Error: UpdateStatusQuota is not defined Source File: chrome://messenger/content/tabmail.js Line: 316 Error: An error occurred updating the cmd_sendUnsentMsgs command: [Exception... "'[JavaScript Error: "msgSendLater is not defined" {file: "chrome://messenger/content/mail3PaneWindowCommands.js" line: 851}]' when calling method: [nsIController::isCommandEnabled]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 71" data: yes] Source File: chrome://global/content/globalOverlay.js Line: 77

> Note: I've since noticed that the splitter reverses itself when you try > to hide the folder pane. "reverses itself" meaning what? I had to alter the way/case when the splitters would be hidden: - If you click upon a splitter's grippy, the splitter (de-)collapses and stays visible. - If you use an F-key to (de-)collapse a splitter, the splitter will hide when collapsing it "onto" the whole(!) border of the mailcontent rectangle. This is in concordance with the sidebar behaviour in the browser. > > Which layout? > Any layout, except for the thread pane hiding issue. In vertical layout, when hiding the thread pane in the middle, folder and messagepane splitter will have different collapse directions, hence you end up with two splitters in the middle. I'm not sure how to solve this in a more logical way - maybe special casing this as well and hiding the messagepane splitter as well? In wide layout, the splitter remains because it doesn't take up the complete outer border of mailcontent. I'll special case this in the next version. > > Do you get any related JS (strict) errors? > I don't know if these are related: > Error: UpdateStatusQuota is not defined Hm, I see where this comes from, and UpdateStatusQuota isn't defined indeed - what did you do to provoke this? > Error: An error occurred updating the cmd_sendUnsentMsgs command: I'll look into this - any specific steps to reproduce?

(In reply to comment #33) > Error: UpdateStatusQuota is not defined We didn't port bug 278096 yet, thus will remove the call. > "'[JavaScript Error: "msgSendLater is not defined" {file: > "chrome://messenger/content/mail3PaneWindowCommands.js" line: 851}]' Doesn't seem to be related.

Changes since v2: - Allow closing the first tab if at least one other 3pane tab remains. - Use dropdown marker as icon for list of open tabs. - Hide folderpane splitter if threadpane is hidden in wide layout. - Minor cleanup and refactoring.

Okay a few comments from testing (using Classic theme and Classic Layout on Linux): - If you open a message in a new tab, you get a folder pane and a message pane in the new tab. When you click on the splitter at the top of the new window you get a very wide folder pane and a small grey vertical pane about 2cm wide. - The status of the message pane splitter is not always remembered when opening a new tab from the folder context menu (so sometimes no message pane when parent had one showing, or the other way round). - Starting up with -mail flag gives following in Error Console Error: this.mPanelContainer is null Source File: chrome://navigator/content/tabbrowser.xml Line: 2216 - Starting up mail for first time, selecting server so account central shows, selecting open in new tab for a folder in the folder context menu, then selecting what should be the account central tab, causes the account central tab (the first one) to be replaced by the contents of the tab you have just opened (with message pane splitter hidden). Also seems to happen when you open account central in a new tab from folder context menu and then switch back to original tab (but not 100% of the time). - Starting up mail (in classic layout with message pane showing) and then opening a second tab from folder content menu. Switch layout to wide, then switch to first tab and select a message in the thread pane. Nothing shows in message pane, switch to second tab, select a message in the thread pane, message shows in message pane. Switching layout back to classic fixes display. Does the same thing if you start with any layout, open 2nd tab and switch to a different layout. I will continue testing tomorrow.

Actually F9 does not seem to hide the folder pane at all now. Also it wasn't until I closed down my testing that I spotted somewhere in my testing I start generating lots of the following at the console prompt: An error occurred updating the cmd_expandAllThreads command An error occurred updating the cmd_collapseAllThreads command

If you click the new tab icon quickly enough (usually 4+ times) you also get: Error: gDBView is null Source File: chrome://messenger/content/msgMail3PaneWindow.js Line: 94

Sometimes when I try to open a folder in a new tab (seems to happen after I opened 5-7 tabs), the tab refuses to open and I get this js error: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.msgDatabase]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://messenger/content/msgViewPickerOverlay.js :: GetFolderInfo :: line 182" data: no]

Please, when testing tabmail, at least delete your localstore.rdf first. It may contain old persistency values which are likely to confuse tabmail! (In reply to comment #37) > - If you open a message in a new tab, you get a folder pane and a message pane > in the new tab. Cannot reproduce. Opening a message in a tab should mean "folderpane hidden, threadpane hidden, messagepane visible" and this it does for me. How did you "open a message in a new tab", by button, by context menu or by menu? > When you click on the splitter at the top of the new window Sorry, I don't understand. Part of your broken new message in a tab? > - The status of the message pane splitter is not always remembered when > opening a new tab from the folder context menu (so sometimes no message > pane when parent had one showing, or the other way round). "Sometimes" is hard to reproduce. :-/ > - Starting up with -mail flag gives following in Error Console > Error: this.mPanelContainer is null > Source File: chrome://navigator/content/tabbrowser.xml > Line: 2216 Cannot reproduce. > - Starting up mail for first time, selecting server so account central shows, > selecting open in new tab for a folder in the folder context menu, then > selecting what should be the account central tab, causes the account central > tab (the first one) to be replaced by the contents of the tab you have just > opened (with message pane splitter hidden). Yes, I see that, too. > - Starting up mail (in classic layout with message pane showing) and then > opening a second tab from folder content menu. Switch layout to wide, then > switch to first tab and select a message in the thread pane. Yes, something's broken here, I (sometimes?!) don't even get the threadpane updated. What account type, IMAP or other? > Does the same thing if you start with any layout, open 2nd tab and switch to a > different layout. I'll see. I honestly didn't test intermediate layout changes too thoroughly. :-| (In reply to comment #38) > Actually F9 does not seem to hide the folder pane at all now. Cannot reproduce. > Also it wasn't until I closed down my testing that I spotted somewhere in my > testing I start generating lots of the following at the console prompt: > An error occurred updating the cmd_expandAllThreads command > An error occurred updating the cmd_collapseAllThreads command These might explain your F9 issue - too bad the actual error isn't dumped. :( Maybe, if you have Venkman open and set it to trace exceptions, you get more useful hints? (In reply to comment #39) > If you click the new tab icon quickly enough (usually 4+ times) you also get: > Error: gDBView is null Yes, but only if a folder is selected. (In reply to comment #40) > chrome://messenger/content/msgViewPickerOverlay.js :: GetFolderInfo :: line > 182" data: no] Yes, I see this for my Movemail account, too.

> Please, when testing tabmail, at least delete your localstore.rdf first. It may > contain old persistency values which are likely to confuse tabmail! If/When tabmail lands almost anybody would have a localstore.rdf with old confusing persistency values floating around so testing with a fresh localstore.rdf isn't very useful.

(In reply to comment #42) > If/When tabmail lands almost anybody would have a localstore.rdf with old > confusing persistency values floating around No. I'm not saying that it _does_ have confusing values, I'm just saying that killing it makes sure that it does not... > so testing with a fresh localstore.rdf isn't very useful. Wrong. Starting with a clean localstore.rdf makes reviewers and myself start on common ground, hence I can reproduce and fix stuff. How am I to fix bugs I cannot see which might even be caused by other bugs/patches leaving traces in localstore.rdf?

(In reply to comment #41) > Please, when testing tabmail, at least delete your localstore.rdf first. It may > contain old persistency values which are likely to confuse tabmail! > Okay deleted localstore.rdf before starting the testing. > (In reply to comment #37) > > - If you open a message in a new tab, you get a folder pane and a message pane > > in the new tab. > > Cannot reproduce. > Opening a message in a tab should mean "folderpane hidden, threadpane hidden, > messagepane visible" and this it does for me. > How did you "open a message in a new tab", by button, by context menu or by > menu? Still get this issue, I am using the context menu (not sure how I would do it by a button and I guess from the menu by changing pref settings but I've left my prefs as they are). > > > When you click on the splitter at the top of the new window > > Sorry, I don't understand. Part of your broken new message in a tab? Yes, the new tab with the message in and the folder pane that should not be there. > > > - The status of the message pane splitter is not always remembered when > > opening a new tab from the folder context menu (so sometimes no message > > pane when parent had one showing, or the other way round). > > "Sometimes" is hard to reproduce. :-/ I know but I could not ignore it. > > > - Starting up with -mail flag gives following in Error Console > > Error: this.mPanelContainer is null > > Source File: chrome://navigator/content/tabbrowser.xml > > Line: 2216 > > Cannot reproduce. Still get this. > > - Starting up mail (in classic layout with message pane showing) and then > > opening a second tab from folder content menu. Switch layout to wide, then > > switch to first tab and select a message in the thread pane. > > Yes, something's broken here, I (sometimes?!) don't even get the threadpane > updated. What account type, IMAP or other? Tested originally with IMAP but also see it in news feeds. Additionally in news feeds is that you get an error in the console: Error: this.browsers[i] is undefined Source file: chrome://navigator/content/tabbrowser.xml Line: 976 > > (In reply to comment #38) > > Actually F9 does not seem to hide the folder pane at all now. > > Cannot reproduce. All F9 seems to do (on Linux) is hide and show the splitter. If you look at the View->Layout->Folder Pane it shows as checked all the time. > > > Also it wasn't until I closed down my testing that I spotted somewhere in my > > testing I start generating lots of the following at the console prompt: > > An error occurred updating the cmd_expandAllThreads command > > An error occurred updating the cmd_collapseAllThreads command I will have to try and work out how these are generated first, seen it a few times.

(In reply to comment #44) > > > - Starting up with -mail flag gives following in Error Console > > > Error: this.mPanelContainer is null > > > Source File: chrome://navigator/content/tabbrowser.xml > > > Line: 2216 > > > > Cannot reproduce. > Still get this. mPanelContainer is a tab*browser* variable that isn't in this patch or mailnews but in navigator, see http://mxr.mozilla.org/comm-central/search?string=mPanelContainer - I wonder what this is, I'm not seeing it either. > Additionally in news > feeds is that you get an error in the console: > Error: this.browsers[i] is undefined > Source file: chrome://navigator/content/tabbrowser.xml > Line: 976 That one I'm also seeing, but only if news messages are displayed as web pages. This again is tab*browser* interestingly.

(In reply to comment #45) > (In reply to comment #44) > > > > - Starting up with -mail flag gives following in Error Console > > > > Error: this.mPanelContainer is null > > > > Source File: chrome://navigator/content/tabbrowser.xml > > > > Line: 2216 > > > > > > Cannot reproduce. > > Still get this. > > mPanelContainer is a tab*browser* variable that isn't in this patch or mailnews > but in navigator, see > http://mxr.mozilla.org/comm-central/search?string=mPanelContainer - I wonder > what this is, I'm not seeing it either. Do you have a message pane or just folder and thread panes? Do you have a mail start page? If I get rid of my message pane then the error does not occur, still there if all I do is not have a mail start page. > > > Additionally in news > > feeds is that you get an error in the console: > > Error: this.browsers[i] is undefined > > Source file: chrome://navigator/content/tabbrowser.xml > > Line: 976 > > That one I'm also seeing, but only if news messages are displayed as web pages. > This again is tab*browser* interestingly. News messages or news feeds? All my old feed items are web pages, all new ones are just summaries. Not sure how to change that!

(In reply to comment #46) > Do you have a message pane or just folder and thread panes? Do you have a mail > start page? If I get rid of my message pane then the error does not occur, > still there if all I do is not have a mail start page. I have the message pane there (standard 3-pane), start page and all kind of messages work fine, that error message doesn't appear. > > That one I'm also seeing, but only if news messages are displayed as web pages. > > This again is tab*browser* interestingly. > News messages or news feeds? All my old feed items are web pages, all new ones > are just summaries. Not sure how to change that! Erm, feed messages, and switching with View > Feed Message Body As adds the error again every time I'm switching to web page (on an Atom feed that I know contains a full entry as "summary", if that matters).

Karsten, could you integrate the hack done in bug 460252 here (with the appropriate XX comments as in the actual http://hg.mozilla.org/comm-central/rev/fab45ddceb14 checkin)? If we want Lightning to work with our tabmail, we need this - at least for now.

Fixed in this incarnation: - Switching from a folder to an account tab overwrote the account tab with the folder tab data (comment #37). - Many click protection for Clone Tab button (comment #39). - Exception in GetFoldeRInfo (comment #40). - Classic layout: messagepane splitter was visible although threadpane was hidden. - Incorporated bug 460252 for Calendar integration (comment #48). Not fixed: Switching layouts seems to hurt the messagesink, not sure why yet. This is an edge case I'll need to look into, but it shouldn't affect further testing. @IanN: Can you narrow your issues to a specific (default?) account type by using a clean profile and adding accounts until they show up? Btw: tabmail widgets are derived(!) from tabbrowser, so they do inherit(!) stuff from there - I just dunno why I don't see these issues...

> Btw: tabmail widgets are derived(!) from tabbrowser Why? Thunderbird doesn't do this. I think a better way of doing things is to have a generic tabbrowser widget that both the navigator and mailnews tabbrowsers can inherit from.

(In reply to comment #50) > > Btw: tabmail widgets are derived(!) from tabbrowser > Why? Thunderbird doesn't do this. For some strange reason Thunderbird doesn't want to be an internet suite. ;-) The eventual goal is to have just one tabbrowser widget, used by both browser and mail. This will most probably mean taking tabmail.xml code as the base, plus any code it lacks from tabbrowser.xml (like preview and such). In order to aid proper styling and code reuse, thus (hopefully) minimizing later unification pain without spending too much time on the unification now, I made tabmail.xml extend tabbrowser.xml. > I think a better way of doing things is to have a generic tabbrowser widget > that both the navigator and mailnews tabbrowsers can inherit from. There's (or rather: will be) no need for different widgets. The 'tab type' extension pattern used in tabmail.xml is so powerful that mere browser tabs are almost a no-brainer. ;-)

Fixed bitrot by porting bug 493700, otherwise unchanged.

Comment on attachment 377857 [details] [diff] [review] tabmail, v3 OK, so I've still only got around to testing this version :-( and I didn't try a new profile :-( but I only noticed two bugs: a) access key conflict between open in new tab and get selected messages b) if folder pane has been collapsed by splitter then opening message in tab does not hide the splitter except when using wide view.

(In reply to comment #51) > The eventual goal is to have just one tabbrowser widget, used by both browser > and mail. This will most probably mean taking tabmail.xml code as the base, > plus any code it lacks from tabbrowser.xml (like preview and such). I'm not convinced by this... although I think it was good that you made tabmail inherit from tabbrowser, thus making it easier to migrate some of the features we don't have in tabbrowser yet such as the tab scrolling and menu.

Comment on attachment 381372 [details] [diff] [review] tabmail, v5 >+ <box id="tabmail-buttons" orientation="horizontal"/> Not used? >+const kTabShowNoPane = 0 << 0; Just 0 on its own will do! >+ // Each tab gets its own messenger instance; >+ // I assume this is so each one gets its own undo/redo stack? [Back/Forward too, no?] >+ // update the message header only if we're the current tab >+ // - warum? weil sich sonst background-tab aktualisiert auf den Titel des aktuellen >+ // - aber mit dieser Einschränkung wird bei "open message tab in background" nur ein folder tab erzeugt... No German comments in checkin, please! >+ if (aTabNode.parentNode.selectedItem == aTabNode) if (aTabNode.selected) >+ aTabNode.setAttribute("Offline", Boolean(flags & nsMsgMessageFlags.Offline)); [!!() also works] + // + // nsIController implementation + // I don't suppose we can inherit this somehow? >+ aSplitter.setAttribute("state", null); [removeAttribute("state"); works] >+function UpdateFolderPaneLayoutVisibility(aTuneLayout) Should be named UpdateFolderPaneFlex, no? >+ messagesBox.removeAttribute("flex"); Isn't it always collapsed at this point anyway? >+ if (folderPaneVisible || threadPaneVisible) >+ { >+ forceMessageSplitter = true; Shouldn't this require the message pane to be visible too? >+ if (folderPaneVisible && (threadPaneVisible || messagePaneVisible)) >+ forceFolderSplitter = true; >+ if (messagePaneVisible && (threadPaneVisible || folderPaneVisible)) >+ forceMessageSplitter = true; So if only the thread pane is hidden you still get two splitters... if (messagePaneVisible && threadPaneVisible) forceMessageSplitter = true; >+ if (hideFolderSplitter) >+ GetFolderPaneSplitter().collapsed = true; >+ if (hideMessageSplitter) >+ GetThreadAndMessagePaneSplitter().collapsed = true; >+ if (forceFolderSplitter) >+ GetFolderPaneSplitter().collapsed = false; >+ if (forceMessageSplitter) >+ GetThreadAndMessagePaneSplitter().collapsed = false; It seems possible for both hideFolderSplitter and forceFolderSplitter to be set simultaneously, which is inefficient; two suggestions: if (forceFolderSplitter) GetFolderPaneSplitter().collapsed = false; else if (hideFolderSplitter) GetFolderPaneSplitter().collapsed = true; or if (forceFolderSplitter || hideFolderSplitter) GetFolderPaneSplitter().collapsed = !forceFolderSplitter; > <field name="tabTypes" readonly="true"> >- new Object() >+ new Object(); I know this is an expression statement, not an expression, but I still don't like the ; (I know tabbrowser has some cases with ; too) >+ this.tabContainer.selectedIndex = this.tabInfo.indexOf(desiredTab); this.tabContainer.selectedItem = desiredTab; >+ this.removeTabByNode( >+ this.tabContainer.childNodes[this.tabContainer.selectedIndex]); this.removeTabByNode(this.tabContainer.selectedItem); >+ aTabNode = this.tabContainer.childNodes[this.tabContainer.selectedIndex]; [Exercise for the reader] >+ var anonid = event.originalTarget.getAttribute("anonid"); >+ if (anonid == "close-button") >+ this.mOverCloseButton = true; Why not add onmouseover/onmouseout to the close button? >+ <handler event="overflow"> >+ <![CDATA[ >+ // filter underflow events which were dispatched on nested scrollboxes >+ if (event.target != this) >+ return; Does <handler event="overflow" phase="target"> work? (Probably not, depending on what's overflowing.) >+ // try to set the aField to a value read from a pref Why not add the default values to browser-prefs.js? > <field name="_xulWindow"> Unused? >+ if (!aEvent.isTrusted) >+ return; Untrusted events in the all tabs popup would be the least of your problems! >+ case "command": >+ this._menuItemOnCommand(aEvent); Command events bubble, so convert _menuItemOnCommand into a <handler> >+ aTab.addEventListener("DOMAttrModified", this, false); Please use broadcasters instead. See menulist.xml for example. >+ <handler event="click" button="0"> <handler event="command"> (if it works) >+.tabmail-tab[type="message"] .tab-icon { >+ margin-top: -4px; ??? >+/* the news icons are only 14px high, unfortunately */ Actally I think you'll find all message icons are only 14px high (we can fix the icon sizes in a followup patch to avoid bloating this one.)

> (From update of attachment 381372 [details] [diff] [review]) >>+ <box id="tabmail-buttons" orientation="horizontal"/> > Not used? Used by Lightning. As I understand it "tabmail-buttons" is there for extensions to overlay since normally extensions trying to overlay UI generated by XBL have to do rather hacky stuff.

More testing using a new profile: * In classic layout - Make sure you have message pane showing, open a folder in new tab, then select server so you get account central, switch back to first pane and you see the message pane has been hidden. * In wide layout - have two tabs open and make one of your tabs be account central, if you switch to the other tab the width of the folder pane is wider than it should be until you toggle the message pane. Switching to the account central tab and back again, makes the folder pane width too large again. * In wide or vertical layout - pressing F9 in a tab which has account central open does strange things! If no additional tabs open, only way to get back thread/message panes is to open a new tab. * In vertical layout - make sure you have a message pane showing, open a folder in a new tab, then select server so you get account central, switch back to first tab and all you see is the folder pane, both the thread and message pane are missing (or just the thread pane) until you press F8 or F9 twice.

In bug 467768 Thunderbird seems to implement changes to the tabbed interface which might be also interesting for this bug.

(In reply to comment #58) > In bug 467768 Thunderbird seems to implement changes to the tabbed interface > which might be also interesting for this bug. It's hard enough right now to stay on top of the changes Thunderbird people actually check into the tree, let's get a working version of our tabmail in and then care about further improvements on top of it, please. Meanwhile, bug 496647 and bug 474701 landed tabmail changes which Karsten needs to update the patch for anyways.

I'm not sure how much more Thunderbird will be changing tabmail.xml over the next couple of weeks. So, a suggestion: why not commit just a copy of tabmail.xml into hg and then build this patch on top of that? Make a note of the revision copied, and then further Thunderbird changes can be pulled in later if required.

Comment on attachment 381372 [details] [diff] [review] tabmail, v5 >--- a/suite/mailnews/messenger.xul >+++ b/suite/mailnews/messenger.xul >- <browser id="messagepane" >- context="mailContext" >+ <browser id="messagepane" >+ context="messagePaneContext" I think this change is unintentional, reverting it restores the missing context menu in message content.

Addressed all review comments from #55, #57 and #61 except the following: (In reply to comment #55) > (From update of attachment 381372 [details] [diff] [review]) > >+ <box id="tabmail-buttons" orientation="horizontal"/> > Not used? See comment #56. > >+ aTabNode.setAttribute("Offline", Boolean(flags & nsMsgMessageFlags.Offline)); > [!!() also works] But looks more ugly (here!). > >+ aSplitter.setAttribute("state", null); > [removeAttribute("state"); works] No, it hurts persistency. > >+ messagesBox.removeAttribute("flex"); > Isn't it always collapsed at this point anyway? No, only its children. > So if only the thread pane is hidden you still get two splitters... Yes, but those "point" in different directions. I don't think there's a "right" way to handle this, but I'm open to reasons why we should one but not the other... > >+ this.tabContainer.selectedIndex = this.tabInfo.indexOf(desiredTab); > this.tabContainer.selectedItem = desiredTab; No, tabInfo != tabNode. > >+ aTabNode = this.tabContainer.childNodes[this.tabContainer.selectedIndex]; > [Exercise for the reader] Doesn't exist anymore. > >+.tabmail-tab[type="message"] .tab-icon { > >+ margin-top: -4px; > ??? Otherwise the tab icons look uglily kludged to the bottom... I also reassigned Ctrl-T to "Open new Tab" for consistency reasons (browser tabs), which means that I had to reassign "Get new Mail (for)" as well. Since "getting" mail is usually some kind of "download" I considered Ctrl-(Shift)-D a suitable replacement (not that I had much choice, almost all keys are assigned anyway). This, otoh, means that Lightning's usage of Ctrl-D for "New Task" clashes... Finally, this patch doesn't drag in TB's current tabmail.xml anymore, we forked revision 2808 into suite.

Testing using a new profile: * If you open a folder in a second tab and then make it an account central one, switching from 1st to 2nd tab gives the following message in the error console: Error: gDBView is null Source File: chrome://messenger/content/msgHdrViewOverlay.js Line: 869 * If you select a message from the thread pane in your first tab, then open another folder into a second tab, in the second tab set it to account central, switch back to the first tab, back to the second tab and select a folder, the message pane shows the header for message from the first tab - probably related to the problem above. * If you change views with more than one tab open, any other tab you switch to does not display messages in the message pane when you select them in the thread pane. * If you open a second tab by selecting Open in New Tab from context menu on account (so it opens account central in second tab) and then select a folder from the thread pane, the message pane is always hidden - it should match the hidden status of the 1st pane.

(In reply to comment #62) > > >+ aSplitter.setAttribute("state", null); > > [removeAttribute("state"); works] > No, it hurts persistency. In that case I'd prefer you to use "open". > > >+ this.tabContainer.selectedIndex = this.tabInfo.indexOf(desiredTab); > > this.tabContainer.selectedItem = desiredTab; > No, tabInfo != tabNode. Bad naming then, if it's not clear whether it's the desired info or node...

(In reply to comment #62) > > >+.tabmail-tab[type="message"] .tab-icon { > > >+ margin-top: -4px; > > ??? > Otherwise the tab icons look uglily kludged to the bottom... Oh, this is because the thread pane icons are a different height? For some reason the tab strip isn't properly collapsed when there are no tabs. In "message" mode, the folder picker is disabled, but the Go menu items aren't. (I haven't decided which of the two is the one I think is correct!) I can't remember how :-( but I got the folder pane into a state where it didn't seem to want to open. Showing the thread pane seemed to fix things.

Updating patch to comments #63-#65. (In reply to comment #63) > Error: gDBView is null I couldn't reproduce this. But it could only happen due to irregular header pane, hence it should be fixed now. (In reply to comment #65) > For some reason the tab strip isn't properly collapsed when there are no tabs. TB's tabmail could get away with doing the hiding on the tabs element, but we have surrounding layout. I moved the entire tab hiding logic into the outer tabmail binding. > In "message" mode, the folder picker is disabled, but the Go menu items > aren't. (I haven't decided which of the two is the one I think is correct!) I think the Go menu entries should get disabled as well, because switching folders without a visible folderpane can be very irritating, especially when selecting a server node or a folder without a selected message...

Testing using a new profile: * Have message pane showing, select a message from thread pane, open a folder into a second tab and select a message. Switch to a different layout, message pane is missing any message details (Subject, From, Date, To headings still show just nothing after them or in the body), selecting another message from the thread pane is similarly empty, switching to the other tab and back again fixes it.

I guess there are new patches coming and since I'll be away for a week, I'd just like to say that I'm ok with how it looks on mac classic now - just so I'm not holding things back if there's a final patch coming up while I'm gone. I'd like to see some kind of indicator in the all tabs list to what tab is the active one, though.

Fixed review comments #67 + #68, plus: - Tabmail v7 accidently lacked the tabbox pref observer

As mentioned on IRC, I seem to regularly not get prompted to enter a password for my inbox with this patch applied (switching to another folder prompts me to enter a password). This is on an IMAP account. When you switch between tabs you initially see the previous tabs message pane body information jumping around as the header vanishes and is refreshed before the message pane body is. Is there anyway to make this less obvious?

(In reply to comment #70) > As mentioned on IRC, I seem to regularly not get prompted to enter a password > for my inbox with this patch applied (switching to another folder prompts me > to enter a password). This is on an IMAP account. On startup? Or later on? I did a lot of restarts now, but I always got prompted for my IMAP password... (IMAP with SSL, but without secure auth.) > When you switch between tabs you initially see the previous tabs message pane > body information jumping around as the header vanishes and is refreshed before > the message pane body is. Is there anyway to make this less obvious? Your computer is too fast (as is mine). :-/ Actually, the messagepane and headers do get cleared by tabmail.js calling ClearMessagePane - you can see it happen in the debugger. Not calling ClearMessagePane will flicker less, but then we get residual header panes again under certain circumstances (eg switching to a tab with an already moved message)). If someone has any ideas here, I'm all ears...

Can't open a message in a new window: Error: GetHeaderPane is not defined Source File: chrome://messenger/content/msgHdrViewOverlay.js Line: 266

Comment on attachment 389577 [details] [diff] [review] tabmail, v8 Haven't looked at tabmail.* yet. > function InitGoMessagesMenu() I need to try this to see if it disables when I expected it to. >+.tabmail-tabs > .tabmail-tab When would you get a .tabmail-tab not in a .tabmail-tabs? >+.tabmail-tabs .tabs-newbutton-box > .tabs-newbutton { Why does this depend on tabmail-tabs? If it does, why not add an extra class to tabs-newbutton-box in the tabmail-tabs binding? > function TreeOnMouseDown(event) What happens if you aren't allowed to open in tabs on middle click? The code here will still set the tree into "fake selection" mode. >+ catch (ignored) {} Isn't catch (e) {} standard? >+.tabmail-tab[type="folder"][ServerType="nntp"][NewMessages="true"], As far as I can see, you don't dynamically update NewMessages, so this goes out of date really quickly. >+/* the news icons are only 14px high, unfortunately */ Is this a Classic theme issue only? How easy would it be to fix?

The previous patch(es?) killed the standalone message window (also see comment #72), so I had to bunch of special casing to apply now. Also fixed the issues in comment #73, but note: > >+ catch (ignored) {} > Isn't catch (e) {} standard? The most common pattern is catch (ex). > >+.tabmail-tab[type="folder"][ServerType="nntp"][NewMessages="true"], > As far as I can see, you don't dynamically update NewMessages But I do, see tabmail.js::onTitleChanged method. Or what do you mean? > >+/* the news icons are only 14px high, unfortunately */ > Is this a Classic theme issue only? How easy would it be to fix? No, both Classic's and Modern's icons (png/gif) are all just 16x14. Because tabbrowser.css fixes the tab icons at 16x16, the icons get scaled. This causes very visibly distortions for news and rss. Do the other icons look misscaled as the CSS suggests?! (They don't for me.) Other fixes: * Introduced a new pref browser.tabs.opentabfor.doubleclick for opening tabs on double click, which renders mailnews.reuse_thread_window2 unused (TB overwrites the current default anyway). Without smtabmail, double clicking a folder would either do nothing or open a new messenger.xul. Now, double clicking a folder will either open the folder in a new tab or open a new messenger.xul. * Solved an issue with switching tabs from Account Central to message tab. * Killed a useless field/property combination in tabmail.xml.

Comment on attachment 391731 [details] [diff] [review] tabmail, v9 >+ aTabNode.setAttribute("NewMessages", msgSelectedFolder.hasNewMessages); So although this does get updated when the title changes, hasNewMessages can change on biff or when you read a message in another tab.

(In reply to comment #75) > >+ aTabNode.setAttribute("NewMessages", msgSelectedFolder.hasNewMessages); > So although this does get updated when the title changes, hasNewMessages can > change on biff or when you read a message in another tab. Yes. That'll make OnItemIntPropertyChanged fire and the tab title and icon change accordingly...

Changes in this patch: - fixed opening messages from the search dialog results - additional tabmail-related standalone window fixes

(In reply to comment #76) > (In reply to comment #75) > > >+ aTabNode.setAttribute("NewMessages", msgSelectedFolder.hasNewMessages); > > So although this does get updated when the title changes, hasNewMessages can > > change on biff or when you read a message in another tab. > Yes. That'll make OnItemIntPropertyChanged fire and the tab title and icon > change accordingly... Ah, so I wonder why it went wrong for me then...

(In reply to comment #78) > Ah, so I wonder why it went wrong for me then... Maybe you have exact steps to reproduce when it fails for you? Could be Windows vs. Linux or whatever...

Comment on attachment 392103 [details] [diff] [review] tabmail, v10 Comments on tabmail.js follow; only tabmail.xml left to go! >+let pref = null; Top-level let doesn't make sense. (This is true directly inside a function declaration too but there are too many of those to fix... actually I even spotted you using a var that wasn't top-level, so there!) >+ pref = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch2); Nit: wrong indent >+ let showThreadPane = aTabInfo.modeBits & kTabShowThreadPane || showAcctCentral; >+ let showMessagePane = aTabInfo.modeBits & kTabShowMessagePane || showAcctCentral; I don't understand. Account central is one pane, not two. [Could use modeBits & (kTabShowThreadPane | kTabShowAcctCentral)] >+ let msgHdr = aTabInfo.hdr; >+ let msgId = aTabInfo.selectedMsgId; >+ SelectFolder(aTabInfo.uriToOpen); The SelectFolder call changes these variables? >+ folderTreeSelection.selectEventsSuppressed = true; >+ folderTreeSelection.select(row); >+ treeBoxObj.ensureRowIsVisible(row); >+ folderTreeSelection.selectEventsSuppressed = false; Suppressing events won't help because unsuppressing causes an event anyway. >+ if (gDBView) >+ { >+ // This sets the thread pane tree's view to the gDBView view. >+ UpdateSortIndicators(gDBView.sortType, gDBView.sortOrder); >+ RerootThreadPane(); >+ UpdateLocationBar(gMsgFolderSelected); Why does the location bar not update for account central? >+ // we need this if once we have a custom toolbar item for mailviews >+ //if (document.getElementById("mailviews-container")) Unlike toolkit, our custom toolbars never remove elements from the DOM. >+ // We clear the selection in order to generate an event when we >+ // re-select our message. This destroys aTabInfo.selectedMsgId. Why not call ThreadPaneSelectionChanged() directly? >+ if (!msgSelectedFolder) >+ { >+ // Don't show "undefined" as title when there is no account. >+ aTabInfo.title = ""; >+ } >+ else >+ { >+ aTabInfo.title = msgSelectedFolder.prettyName; >+ if (!msgSelectedFolder.isServer && multipleRealAccounts) >+ aTabInfo.title += " - " + msgSelectedFolder.server.prettyName; >+ } >+ if (!msgSelectedFolder) >+ return; Why not return the first time? >+ aTabInfo.title = ""; Could set this by default, rather than in two different cases >+ // XXX remove the MessageWindowController stuff once we kill messageWindow.xul >+ __proto__: "DefaultController" in window && window.DefaultController || >+ "MessageWindowController" in window && window.MessageWindowController I was thinking that the caller could set this but I guess that's not worth it if you're killing it. >+ if (!displayDeck) >+ return true; >+ return displayDeck.collapsed; Could write this as !displayDeck || displayDeck.collapsed (same for folder pane) >+ size: aSplitter.getAttribute("orient") ? pane.getAttribute("height") >+ : pane.getAttribute("width") Nit: orient, height and width are all properties as well + let vertical = aSplitter.getAttribute("orient") == "vertical"; Nit: inconsistent with above code >+ if (!onlyMessagePane) >+ { >+ UpdateFolderPaneFlex(!threadPaneVisible && !acctCentralVisible); >+ // hide the folderpane splitter in this special case >+ GetFolderPaneSplitter().collapsed = !threadPaneVisible; >+ if (folderPaneVisible || threadPaneVisible || acctCentralVisible) >+ { >+ forceMessageSplitter = messagePaneVisible; >+ forceFolderSplitter = folderPaneVisible && >+ (threadPaneVisible || acctCentralVisible); Surely !onlyMessagePane implies one of the other panes is visible? Also, would setting hideFolderSplitter always true work? >+ if (folderPaneVisible && (threadPaneVisible || messagePaneVisible || acctCentralVisible)) folderPaneVisible && !onlyFolderPane perhaps? >+ gDBView.loadMessageByUrl("about:blank"); >+ gDBView.selectionChanged(); Side note: this apparently causes a bug :-( selectionChanged() causes a reload, and the view tries to set the charset, but that fails because about:blank is loading. It may or may not suffice to call getWebNavigation().stop() in between. >+ if (!GetFolderPane().collapsed) >+ document.getElementById("folderTree").focus(); >+ ShowingAccountCentral(); Duplicate code, ShowingAccountCentral() already does this.

Here are some comments on tabmail.xml - note that I commented on the patched version, so I don't know which are "your" bits and which are Thunderbird's. removeTabByNode - why not call it removeTab as per tabbrowser? context menuitems should use mContextTab as document.popupNode gets stale :-( tabbox onselect - strange condition, why not: if (event.target.localName == 'tabs' && 'updateCurrentTab' in this.parentNode) this.parentNode.updateCurrentTab(); feels strange to have the prefs on the tabcontainer object this.mAutoHide = prefs.getBoolPref("browser.tabs.autoHide"); if (this.mAutoHide) this.doAutoHide = true; you're setting mAutoHide which already "sets" doAutoHide? doAutoHide is a bad name anyway, it should be !stripVisible. (tabbrowser does it the hard way, using get/setStripVisibility) nit: for a field, new Array(); doesn't need the ; it's possible to make the object its own observer this avoids the "this.tabmail." closure awkwardness <implementation implements="nsIObserver"> <method name="observe"> <param name="aSubject"/> <param name="aTopic"/> <param name="aData"/> <body> // etc. t.setAttribute('type', tabInfo.mode.type); nit: inconsistent quoting - elsewhere you use double quotes for each (let [i, tabMonitor] in Iterator(this.tabMonitors)) nit: each only affects in Object or in Array, no difference for in Iterator <method name="selectTabByIndex"> <parameter name="aEvent"/> <parameter name="aIndex"/> the lone caller doesn't pass in an event?

Comment on attachment 392103 [details] [diff] [review] tabmail, v10 When you hit the close tab button, the last tab (rightmost) close. In browser, the current, active tab close. I think we should match the browser behaviour here. Hmm, I think this worked before...

Fixed comments #80-#82 , except: (In reply to comment #80) > >+ let msgHdr = aTabInfo.hdr; > >+ let msgId = aTabInfo.selectedMsgId; > >+ SelectFolder(aTabInfo.uriToOpen); > The SelectFolder call changes these variables? Yes. > >+ // We clear the selection in order to generate an event when we > >+ // re-select our message. This destroys aTabInfo.selectedMsgId. > Why not call ThreadPaneSelectionChanged() directly? Because it doesn't work. Or rather: I'm not sure which code you have in mind I could replace by it? > + let vertical = aSplitter.getAttribute("orient") == "vertical"; > Nit: inconsistent with above code No?! > Also, would setting hideFolderSplitter always true work? No, because I want to distinguish key presses like F9 (-> collapse pane, hide splitter) from clicking the splitter (-> collapse pane, keep splitter visible). > >+ gDBView.loadMessageByUrl("about:blank"); > >+ gDBView.selectionChanged(); > Side note: this apparently causes a bug :-( selectionChanged() causes a > reload, and the view tries to set the charset, but that fails because > about:blank is loading. It may or may not suffice to call > getWebNavigation().stop() in between. I've no idea how to reproduce this problem... (In reply to comment #81) > feels strange to have the prefs on the tabcontainer object Why?

(In reply to comment #83) > (In reply to comment #80) > > >+ // We clear the selection in order to generate an event when we > > >+ // re-select our message. This destroys aTabInfo.selectedMsgId. > > Why not call ThreadPaneSelectionChanged() directly? > Because it doesn't work. > Or rather: I'm not sure which code you have in mind I could replace by it? I was thinking that the code that re-selects the message could call ThreadPaneSelectionChanged(); > > >+ let vertical = aSplitter.getAttribute("orient") == "vertical"; > > Nit: inconsistent with above code > No?! This was the "above" code, which doesn't check the value of the attribute: >+ size: aSplitter.getAttribute("orient") ? pane.getAttribute("height") >+ : pane.getAttribute("width") > > Also, would setting hideFolderSplitter always true work? > No, because I want to distinguish key presses like F9 (-> collapse pane, hide > splitter) from clicking the splitter (-> collapse pane, keep splitter visible). Then how does this line of code distinguish F9 from clicking the splitter: >+ GetFolderPaneSplitter().collapsed = !threadPaneVisible; > (In reply to comment #81) > > feels strange to have the prefs on the tabcontainer object > Why? Because tabmail is the "master" object here.

(In reply to comment #84) > > Or rather: I'm not sure which code you have in mind I could replace by it? > I was thinking that the code that re-selects the message could call > ThreadPaneSelectionChanged(); Doesn't work - or I may be missing something essential. > > > >+ let vertical = aSplitter.getAttribute("orient") == "vertical"; > > > Nit: inconsistent with above code Fixed. > Then how does this line of code distinguish F9 from clicking the splitter: > >+ GetFolderPaneSplitter().collapsed = !threadPaneVisible; You can't hide the displayDeck by clicking a splitter, and you can't hide the displayDeck here (in Wide View) if the folderpane is hidden while the messagepane is not. > > > feels strange to have the prefs on the tabcontainer object I provided tabmail with its own mPrefs field now. I also made the single close button tooltip text resemble its browser cousin.

Comment on attachment 393879 [details] [diff] [review] tabmail, v12 I'll review a file at a time... >diff --git a/suite/mailnews/commandglue.js b/suite/mailnews/commandglue.js >@@ -117,42 +117,45 @@ function LoadMessageByUri(uri) > function setTitleFromFolder(msgfolder, subject) > { > var title = subject || ""; > > if (msgfolder) > { > if (title) > title += " - "; >- > title += msgfolder.prettyName; > > if (!msgfolder.isServer) > { > var server = msgfolder.server; > var middle; > var end; > if (server.type == "nntp") { > // <folder> on <hostname> > middle = gMessengerBundle.getString("titleNewsPreHost"); > end = server.hostName; > } else { > // <folder> for <accountname> > middle = gMessengerBundle.getString("titleMailPreHost"); > end = server.prettyName; > } >- if (middle) title += " " + middle; >- if (end) title += " " + end; >+ if (middle) >+ title += " " + middle; >+ if (end) >+ title += " " + end; You're switching from tab-width: 4 to tab-width: 2 but you're not changing the indentation on code additions to match that consistently. > } > } >- > if (!/Mac/.test(navigator.platform)) > title += " - " + gBrandBundle.getString("brandShortName"); >+ document.title = title; Nit: Indentation. > >- document.title = title; >+ // Notify the current tab, it might want to update also. >+ GetTabMail().saveCurrentTabState(); // gDBView may have changed! >+ GetTabMail().setTabTitle(); > } Not consistent with further on where you do: let tabmail = GetTabMail(); etc Do one or the other please. (correct indentation though!) >@@ -883,52 +852,54 @@ function FolderPaneSelectionChange() >+ let tabmail = GetTabMail(); >+ tabmail.saveCurrentTabState(); // gDBView may have changed! >+ tabmail.setTabTitle(); >+ } >+ else >+ { >+ msgWindow.openFolder = null; >+ ClearThreadPane(); >+ } > >- if (gAccountCentralLoaded) >- UpdateMailToolbar("gAccountCentralLoaded"); >- else if (gFakeAccountPageLoaded) >- UpdateMailToolbar("gFakeAccountPageLoaded"); >- else >- document.getElementById('advancedButton').setAttribute("disabled" , !(IsCanSearchMessagesEnabled())); >+ if (gAccountCentralLoaded) >+ UpdateMailToolbar("gAccountCentralLoaded"); >+ else >+ document.getElementById('advancedButton') >+ .setAttribute("disabled" , !(IsCanSearchMessagesEnabled())); Nit: Using double quotes would be more consistent. Spurious space between "disabled" and the comma.

(In reply to comment #85) > (In reply to comment #84) > > Then how does this line of code distinguish F9 from clicking the splitter: > > >+ GetFolderPaneSplitter().collapsed = !threadPaneVisible; > You can't hide the displayDeck by clicking a splitter, and you can't hide the > displayDeck here (in Wide View) if the folderpane is hidden while the > messagepane is not. Sorry, but I don't think I'm talking about the displayDeck. I just want to know why you hide the folder splitter and then show it again. >I provided tabmail with its own mPrefs field now. I guess this argument will disappear when we switch to Application.prefs ;-)

Comment on attachment 393879 [details] [diff] [review] tabmail, v12 >diff --git a/suite/mailnews/mailWindowOverlay.js b/suite/mailnews/mailWindowOverlay.js >@@ -157,32 +159,42 @@ function InitEditMessagesMenu() > { > goSetMenuValue('cmd_delete', 'valueDefault'); > goSetAccessKey('cmd_delete', 'valueDefaultAccessKey'); > document.commandDispatcher.updateCommands('create-menu-edit'); > } > > function InitGoMessagesMenu() > { >+ // deactivate the folders in the go menu if we don't have a folderpane >+ document.getElementById("mailFolderPane").setAttribute("disabled", IsFolderPaneCollapsed()); Nit: Put .setAttribute indented on next line as line is too long as is. >+ if (message_menuitem && !message_menuitem.hidden) >+ { > message_menuitem.setAttribute('checked', !IsMessagePaneCollapsed()); > message_menuitem.setAttribute('disabled', gAccountCentralLoaded); > } > >+ var threadpane_menuitem = document.getElementById('menu_showThreadPane'); >+ if (threadpane_menuitem && !threadpane_menuitem.hidden) >+ { >+ threadpane_menuitem.setAttribute('checked', !IsDisplayDeckCollapsed()); >+ threadpane_menuitem.setAttribute('disabled', gAccountCentralLoaded); You might as well sort out the indentation on the lines above and consistency on single vs double quote.

Comment on attachment 393879 [details] [diff] [review] tabmail, v12 Looks good to me. As we discussed on irc, the new mailWindow1.css doesn't need the -moz-appearance: none rule and there are 2 splitter rules that needs to be changed (one in the old file). There is an issue with the tabs which is related to the current design: there's no bottom border on a selected tab and this doesn't play well with the trees underneath the tabs, because the trees lacks a top border. The solution here might be to introduce some padding/border-bottom on the tabstrip, but that could surely go in a follow-up. It feels a bit weird with the all-tabs button and the tab-close button next to each other - could be that I'm not used to it, though.

(In reply to comment #89) > (From update of attachment 393879 [details] [diff] [review]) > Looks good to me. As we discussed on irc, the new mailWindow1.css doesn't need > the -moz-appearance: none rule and there are 2 splitter rules that needs to be > changed (one in the old file). > > There is an issue with the tabs which is related to the current design: there's > no bottom border on a selected tab and this doesn't play well with the trees > underneath the tabs, because the trees lacks a top border. The solution here > might be to introduce some padding/border-bottom on the tabstrip, but that > could surely go in a follow-up. Comments about design/css above deals with mac classic, of course.

Addressed comments #86-#90.

Comment on attachment 395693 [details] [diff] [review] tabmail, v13 >+ // hide the folderpane splitter in this special case I'm confused. You removed the code, and left the comment. Did you forget to replace the code with some other code, or did you mean to remove the code?

Revamping the splitter hiding code into something more logical.

Comment on attachment 395906 [details] [diff] [review] tabmail, v14 >diff --git a/suite/mailnews/mailWindowOverlay.xul b/suite/mailnews/mailWindowOverlay.xul >@@ -542,17 +562,21 @@ > accesskey="&copyCmd.accesskey;" > command="cmd_copy"/> > <menuitem id="context-searchselect" > oncommand="MsgOpenSearch(gContextMenu.searchSelected(), event.shiftKey);"/> > <menuitem id="mailContext-openNewWindow" > label="&contextOpenNewWindow.label;" > accesskey="&contextOpenNewWindow.accesskey;" > oncommand="MsgOpenNewWindowForMessage();"/> >- <menuseparator id="mailContext-sep-open"/> >+ <menuitem id="mailContext-openNewTab" >+ label="&contextOpenNewTab.label;" >+ accesskey="&contextOpenNewTab.accesskey;" >+ oncommand="MsgOpenNewTabForMessage();"/> >+ <menuseparator id="mailContext-sep-open"/> Nit: indentation not correct for menuseparator. >@@ -931,25 +955,30 @@ > <menupopup id="menu_FilePopup" onpopupshowing="file_init();"> > <menu id="menu_New"> > <menupopup id="menu_NewPopup" onpopupshowing="menu_new_init();"> > <menuitem label="&newNewMsgCmd.label;" > accesskey="&newNewMsgCmd.accesskey;" > key="key_newMessage" > oncommand="MsgNewMessage(null);"/> > <menuitem id="menu_newFolder" label="&newFolderCmd.label;" >- oncommand="MsgNewFolder(NewFolder);" >- accesskey="&newFolderCmd.accesskey;"/> >+ oncommand="MsgNewFolder(NewFolder);" >+ accesskey="&newFolderCmd.accesskey;"/> > <menuitem id="menu_newVirtualFolder" label="&newVirtualFolderCmd.label;" > oncommand="MsgVirtualFolderProperties(false);" > accesskey="&newVirtualFolderCmd.accesskey;"/> > <menuitem id="newAccountMenuItem" label="&newAccountCmd.label;" >- accesskey="&newAccountCmd.accesskey;" >- oncommand="MsgAccountWizard();"/> >+ accesskey="&newAccountCmd.accesskey;" >+ oncommand="MsgAccountWizard();"/> > <menuseparator id="newPopupMenuSeparator"/> >+ <menuitem id="menu_newTab" >+ label="&newTabCmd.label;" >+ accesskey="&newTabCmd.accesskey;" >+ key="key_newTab" >+ oncommand="MsgOpenNewTab();"/> Nit: can we try and keep the order of attributes consistent if we are making changes - so accesskey then oncommand perhaps? >@@ -1389,16 +1433,17 @@ > accesskey="&startPageCmd.accesskey;" command="cmd_goStartPage" > observes="mailHideMenus"/> > <menuseparator id="goNextAfterStartPageSeparator" observes="mailHideMenus"/> > </menupopup> > <template> > <rule iscontainer="true" isempty="false"> > <menupopup> > <menu uri="rdf:*" class="folderMenuItem menu-iconic" >+ observes="mailFolderPane" Nit: I know the rest are not correctly indented but can we line up the one we are adding? >@@ -1418,16 +1463,17 @@ > <menuseparator/> > </menupopup> > </menu> > </menupopup> > </rule> > <rule> > <menupopup> > <menuitem uri="rdf:*" value="rdf:*" class="folderMenuItem menuitem-iconic" >+ observes="mailFolderPane" Nit: same again.

Comment on attachment 395906 [details] [diff] [review] tabmail, v14 >diff --git a/suite/mailnews/messenger.xul b/suite/mailnews/messenger.xul >+ <box id="messagesBox" orient="vertical" flex="1"> >+ <deck id="displayDeck" >+ flex="1" >+ selectedIndex="0" >+ minheight="100" >+ height="100" >+ persist="height" >+ onselect="ObserveDisplayDeckChange(event)"> Nit: missing ; >diff --git a/suite/mailnews/msgHdrViewOverlay.js b/suite/mailnews/msgHdrViewOverlay.js > function OnUnloadMsgHeaderPane() > { > pref.removeObserver("mail.showCondensedAddresses", MsgHdrViewObserver); > > // dispatch an event letting any listeners know that we have unloaded the message pane > var event = document.createEvent('Events'); > event.initEvent('messagepane-unloaded', false, true); >- var headerViewElement = document.getElementById("msgHeaderView"); >+ var headerViewElement = GetHeaderPane(); > headerViewElement.dispatchEvent(event); Can't we just do GetHeaderPane().dispatchEvent(event); ? >diff --git a/suite/mailnews/msgMail3PaneWindow.js b/suite/mailnews/msgMail3PaneWindow.js > OnItemAdded: function(parentItem, item) { }, >- > OnItemRemoved: function(parentItem, item) { }, > >- OnItemPropertyChanged: function(item, property, oldValue, newValue) { }, >+ OnItemPropertyChanged: function(item, property, oldValue, newValue) {}, >+ OnItemBoolPropertyChanged: function(item, property, oldValue, newValue) {}, >+ OnItemUnicharPropertyChanged: function(item, property, oldValue, newValue) {}, >+ OnItemPropertyFlagChanged: function(item, property, oldFlag, newFlag) {}, > >- OnItemIntPropertyChanged: function(item, property, oldValue, newValue) { >- if (item == gMsgFolderSelected) { >- if(property.toString() == "TotalMessages" || property.toString() == "TotalUnreadMessages") { >+ OnItemIntPropertyChanged: function(item, property, oldValue, newValue) Nit: Is there consistency on blank lines between OnItem... and whether there is a space between braces at then end? >+ { >+ // handle the currently visible folder >+ if (item == gMsgFolderSelected) >+ { >+ if (property.toString() == "TotalMessages" || property.toString() == "TotalUnreadMessages") Nit: Split this line over two as it is too long. >+ let tabmail = GetTabMail(); >+ for (let i = 0; i < tabmail.tabInfo.length; ++i) >+ { >+ // if we never switched away from the tab, we only have just one >+ let tabFolder = tabmail.tabInfo[i].msgSelectedFolder || gMsgFolderSelected; Nit: split on the line as it is too long. >@@ -229,17 +250,17 @@ var folderListener = { > // gSearchNotificationListener.OnSearchDone (see searchBar.js). > if (!scrolled && !(gMsgFolderSelected.flags & nsMsgFolderFlags.Virtual)) > ScrollToMessageAfterFolderLoad(msgFolder); > SetBusyCursor(window, false); > } > // Folder loading is over, > // now issue quick search if there is an email address. > viewDebug("in folder loaded gVirtualFolderTerms = " + gVirtualFolderTerms + "\n"); >- viewDebug("in folder loaded gMsgFolderSelected = " + gMsgFolderSelected.URI + "\n"); >+ viewDebug("in folder loaded gMsgFolderSelected = " + gMsgFolderSelected && gMsgFolderSelected.URI + "\n"); Nit: split the line as it is too long.

Comment on attachment 395906 [details] [diff] [review] tabmail, v14 >diff --git a/suite/mailnews/searchBar.js b/suite/mailnews/searchBar.js >@@ -188,19 +188,19 @@ function initializeSearchBar() > > function onEnterInSearchBar() > { > if (!gSearchBundle) > getDocumentElements(); > viewDebug ("onEnterInSearchBar gSearchInput.value = " /* + gSearchInput.value + " showing criteria = " + gSearchInput.showingSearchCriteria */ +"\n"); > if (gSearchInput.value == "" /* || gSearchInput.showingSearchCriteria */) > { >- >- if (gDBView.viewType == nsMsgViewType.eShowQuickSearchResults >- || gDBView.viewType == nsMsgViewType.eShowVirtualFolderResults) >+ let viewType = gDBView && gDBView.viewType; >+ if (viewType == nsMsgViewType.eShowQuickSearchResults || >+ viewType == nsMsgViewType.eShowVirtualFolderResults) > { > statusFeedback.showStatusString(""); > disableQuickSearchClearButton(); > > viewDebug ("onEnterInSearchBar gDefaultSearchViewTerms = " + gDefaultSearchViewTerms + "gVirtualFolderTerms = " > + gVirtualFolderTerms + "gXFVirtualFolderTerms = " + gXFVirtualFolderTerms + "\n"); > var addTerms = gDefaultSearchViewTerms || gVirtualFolderTerms || gXFVirtualFolderTerms; > if (addTerms) Nit: sort out the indentation on the new lines please. >diff --git a/suite/mailnews/tabmail.js b/suite/mailnews/tabmail.js >+ // The user may have changed folders, triggering our onTitleChanged callback. >+ // Update the appropriate attributes on the tab. >+ aTabNode.setAttribute("SpecialFolder", getSpecialFolderString(msgSelectedFolder)); >+ aTabNode.setAttribute("ServerType", msgSelectedFolder.server.type); >+ aTabNode.setAttribute("IsServer", msgSelectedFolder.isServer); >+ aTabNode.setAttribute("IsSecure", msgSelectedFolder.server.isSecure); >+ aTabNode.setAttribute("NewMessages", msgSelectedFolder.hasNewMessages); >+ aTabNode.setAttribute("ImapShared", msgSelectedFolder.imapShared); Nit: You've lined up the 2nd argument in all but the first statement, why not do it for all? >diff --git a/suite/mailnews/tabmail.xml b/suite/mailnews/tabmail.xml >- - functions can defer to the tab type functions by calling >+ - functions can defer to the tab type functions by calling >+ > - this.functionName(). (This should prove convenient.) Nit: why the blank line? >+ - * openTab(aTabInfo, arguments...): Called when a tab of the given mode is Is this going to need to be used by Add-on's, if so is this compatible with TB's API? Similarly for the other ones in this section? >+ - Mode definition functions for menu/toolbar commands (see nsIController): >+ - * supportsCommand(aCommand, aTabInfo): Called when a menu or toolbar needs Would these just be used internally? If so, not a problem with argument order, otherwise should we match TB? Same for rest of this section.

No functional changes, just bitrot fixes.

Fixed review comments #94-#96. Notes: (In reply to comment #95) > >+ let tabFolder = tabmail.tabInfo[i].msgSelectedFolder || gMsgFolderSelected; > Nit: split on the line as it is too long. Only 4 chars over par after rewrapping, thus ignored. (In reply to comment #96) > >+ - * openTab(aTabInfo, arguments...): Called when a tab of the given mode is > Is this going to need to be used by Add-on's, if so is this compatible with > TB's API? Similarly for the other ones in this section? Yes. > >+ - Mode definition functions for menu/toolbar commands (see nsIController): > >+ - * supportsCommand(aCommand, aTabInfo): Called when a menu or toolbar needs > Would these just be used internally? If so, not a problem with argument order, > otherwise should we match TB? Same for rest of this section. This is part of the API, so we should match TB, but TB's current implementation is suboptimal, which I'll fix up in bug 500506.

Oh, forgot this: (In reply to comment #94) > Nit: can we try and keep the order of attributes consistent if we are making > changes - so accesskey then oncommand perhaps? Yes, of course! This was even violating my own spec... :-/ <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>

Forgot some additional tabmail Mac theme changes by stefanh, else no differences to v15.

Comment on attachment 396552 [details] [diff] [review] tabmail, v15.1 >--- a/suite/locales/en-US/chrome/mailnews/messenger.dtd >+++ b/suite/locales/en-US/chrome/mailnews/messenger.dtd >@@ -36,47 +36,59 @@ > <!ENTITY getNewMsgCmd.label "Get New Messages"> > <!ENTITY getNewMsgCmd.accesskey "G"> >-<!ENTITY getNewMsgCmd.key "t"> >+<!ENTITY getNewMsgCmd.key "d"> > <!ENTITY getNewMsgForCmd.label "Get New Messages for"> > <!ENTITY getAllNewMsgCmd.label "Get All New Messages"> >-<!ENTITY getAllNewMsgCmd.key "t"> >+<!ENTITY getAllNewMsgCmd.key "d"> > <!ENTITY getNextNMsgCmd.label "Get Next 500 News Messages"> > <!ENTITY getNextNMsgCmd.accesskey "t"> When changing a commandkey you should alter its name as well.

> When changing a commandkey you should alter its name as well. Why?

(In reply to comment #102) > > When changing a commandkey you should alter its name as well. > Why? Because localizers need to act on it (they are supposed to copy the value from en-US, but they need to change this in their files as well in that case), and changing the ID is the only way of universally give that message right now. We know it sucks, but that's the way things are.

Heh mid-air collision, Robert said it all. This's what I wanted to say, maybe it clears (or confuses? :) things a bit up: In general, localizers are not supposed to localize the commandkeys. So we are leaving .keys as they are, i.e. in English. Once you change a .key to use a new value and then you reuse the old one for a new .key without changing the entity name, localizers won't get notified about the changed key, just about the new entity -> beng, you've got a collision in localized builds that no one is aware of. Until there're automated tests for this and similar issues, developers should try to avoid any possible collisions by changing entity names. You don't need to revise entity names when changing accesskeys though.

On linux: Modern theme does not seem to have left and right scroll arrows for when you have lots of tabs. Default theme the left and right scroll arrows are fairly small, could do with being bigger. There is also an issue to do with message panes and account central... 1) Start SM and open mail. 2) Make sure message pane is showing. 3) Open a second tab and select server so it shows account central. 4) Quit SM. 5) Start SM and open mail. Actual Result 1) Message pane is hidden. Expect Result 1) Message pane is showing as it was on the non-account central tab previously.

*sigh* (In reply to comment #105) > On linux: Here, too. > Modern theme does not seem to have left and right scroll arrows for when you > have lots of tabs. Yes, that's bug 477710, which is already blocking this one since February. > Default theme the left and right scroll arrows are fairly small, could do > with being bigger. These come from toolkit, I'm not sure it's worth tinkering with at all, definitely not worth here. > There is also an issue to do with message panes and account central... ... > Actual Result > 1) Message pane is hidden. > Expect Result > 1) Message pane is showing as it was on the non-account central tab previously. This is WFM. After applying the tabmail patch, I occasionally lack the New Tab button to the left of the tabstrip. I need to run 'make -f client.mk build' in the comm-central directory (with MOZCONFIG set) to fix it. Maybe this makes it work for you as well?

String changes only for premature check-in before string freeze.

Comment on attachment 397046 [details] [diff] [review] tabmail, v16.0 (string changes only) >+# tabmail: warning when closing multiple tabs (as in browser) >+tabs.closeWarningTitle=Confirm close >+tabs.closeWarning=This messenger window has %S tabs open. Do you want to close it and all its tabs? >+tabs.closeButton=Close all tabs >+tabs.closeWarningPromptMe=Warn me when closing multiple messenger tabs. I thought we were using "Mail & Newsgroups" everywhere and not "messenger". For consistency, it would be good to use the same name for the component everywhere. In any case, that discussion shouldn't block this from getting in before the string freeze, we can care for more consistency even at a later stage (though perhaps with string ID changes again), after we unfreeze again.

(In reply to comment #108) > For consistency, it would be good to use the same name for the component > everywhere. In any case, that discussion shouldn't block this from getting in > before the string freeze, we can care for more consistency even at a later > stage (though perhaps with string ID changes again), after we unfreeze again. Yes, I'll file a bug on that. String changes landed as <http://hg.mozilla.org/comm-central/rev/168dfc122fb4> - this means that ^T and Shift-^T have already been replaced by ^D and Shift-^D NOW!

Okay new steps to reproduce my problem: 1) Start SM with -mail 2) Make sure message pane is showing (classic view) 3) Right click on server and select open in new tab - gives account central 4) Right click on folder and select open in new tab 5) Select account central tab 6) Quit SM 7) Start SM with -mail Expected Result 1) Message pane is showing Actual Result 1) Message pane is hidden

Comment on attachment 396552 [details] [diff] [review] tabmail, v15.1 >+function MsgCollapseSplitter(aSplitter, aCollapse) >+{ >+ // generally, allowCollapse needs to be true to hide a splitter, >+ // but splitters with uncollapsed panes may always hide >+ var allowCollapse = aSplitter.allowCollapse || >+ aSplitter.getAttribute("state") != "collapsed"; >+ aSplitter.collapsed = aCollapse && allowCollapse; >+} aCollapse is false if both panes are visible, so we always need the splitter. allowCollapse is false if the splitter was clicked with the mouse, so again we need the splitter. But I don't understand the need for the check for the splitter state? [Note: I'd prefer "collapsible" to "allowCollapse"]

(In reply to comment #110) > Okay new steps to reproduce my problem: > 1) Start SM with -mail > 2) Make sure message pane is showing (classic view) > 3) Right click on server and select open in new tab - gives account central > 4) Right click on folder and select open in new tab > 5) Select account central tab > 6) Quit SM > 7) Start SM with -mail > Expected Result > 1) Message pane is showing We have a structural problem here: - we already *do* store the last pane config, but - showing account central means message pane is hidden, thus => on next startup, message pane is hidden. We don't remember the last URI/message URI, but open the default Inbox, but with the last (wrong?) layout... This might or might not hurt user expectations: (1) "I want SM to remember my last folder/message/layout" (2) "I want SM to always start with default Inbox and full pane layout" Let's spin off this into a follow-up bug.

Filed bug 513515 on comments 110/112.

Comment on attachment 396552 [details] [diff] [review] tabmail, v15.1 r=me with that issue spun off and relnoted

(In reply to comment #111) > >+ var allowCollapse = aSplitter.allowCollapse || > >+ aSplitter.getAttribute("state") != "collapsed"; > >+ aSplitter.collapsed = aCollapse && allowCollapse; ... > But I don't understand the need for the check for the splitter state? Given all three panes are shown in classic layout and you click(!) the folderpane splitter, it will move to the edge and stay visible. Now hit shift-F8 to hide the threadpane -> the folderpane splitter should stay visible, we must not hide collapsed splitters in this case! > [Note: I'd prefer "collapsible" to "allowCollapse"] Done. I also removed some obsolete arguments and had to add an existence check.

On startup, splitters don't had the collapsible member set yet which confused the hiding code, but could be cured by checking the splitter's state. This shouldn't be necessary if the lack of a collapsible member is interpreted as collapsible=true.

Comment on attachment 397570 [details] [diff] [review] tabmail, v18 [Checkin: Comment 119] Interdiff looks good ;-)

Comment on attachment 397570 [details] [diff] [review] tabmail, v18 [Checkin: Comment 119] http://hg.mozilla.org/comm-central/rev/7b0ece4c9b53 Something mangled your name on checkin though :(

Please log any issues in followup bugs.

Many thanks for the checkin! (In reply to comment #119) > Something mangled your name on checkin though :( No, that's just a display bug caused by one of the hg web servers.

I think this is originally my fault since I provided the diff, but the folderpane-splitter has wrong id name in mac classic. Here's the bustage fix ;-)

Comment on attachment 397898 [details] [diff] [review] Fix wrong id on folderpane-splitters in mac classic (pushed) http://hg.mozilla.org/comm-central/rev/0c7943df7694

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query