Closed Bug 467768 Opened 16 years ago Closed 15 years ago

No way to make mail open in tabs by default

Categories

(Thunderbird :: Toolbars and Tabs, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: dave, Assigned: rain1)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 14 obsolete files)

172.44 KB, patch
asuth
: review+
mnyromyr
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.1b3pre) Gecko/20081201 Lightning/1.0pre Thunderbird/3.0b1

When mail is double clicked on a message always opens in a new window. There is no way to make the message open in a tab which would be nice with the new tabbed messaging.

Only way to get this behaviour is to right click on message and open it in a tab

Reproducible: Always

Steps to Reproduce:
1. Double click e-mail
2. Oh look it opens in a new window
3. Damn
It seems reasonable to have a setting to determine default behavior for opening mail. Is this planned for 3.0 final release?
Yes, it think it would be nice. It would help me a lot on my work.
I'd like to have these teo options:

[ ] Open messagge in new tab (instead than in new window)
[ ] Don't switch to the new message window when a message is open (this is activated only when the first one is enabled)

The last option is not so necessary, but the first one would be very nice.
Thank you
Michele Renda

PS. This would copy the Lotus Notes behaviour.
Flags: wanted-thunderbird3?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-thunderbird3?
Bug 435567 already implemented middle click to open new mails in tabs. I'd like us to stay close to what firefox does, so let double click do what it does now.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
OS: Windows Vista → All
Hardware: x86 → All
Mmm... I think is different: thunderbird is not a browser. It doesn't open a email in the same window of message list as firefox do opening a link in the same window.

Would be ok an entry in about:config.
Actually, if Firefox is what is the model, I'm fine with that. Firefox gives you the choice in Options of how you want a link to be handled. If Thunderbird had the same, you could choose how the opening of mail is handled, either in a new tab or a new window. No reason not to give the option to use the tabs by default.
a modifier key for middle click, to cause "load in background", would be great
I think the current choices in the Option Window could be supplemented to allow for opening messages in tabs via double-clicking by adding in the choices: [] A new tab and [] An existing message tab.
(In reply to comment #7)
> a modifier key for middle click, to cause "load in background", would be great

That's suggested in bug 480632. Shall this separate case be handled there
(in which case it should be confirmed as enhancement request) or a combined solution found here (in which case the other bug is a duplicate)?
(In reply to comment #4)
> Bug 435567 already implemented middle click to open new mails in tabs. I'd like
> us to stay close to what firefox does, so let double click do what it does now.

I don't understand this analogy: Firefox has no double-click handler at all.

I'm actually inclined to make the default double-click action be "open in new tab" instead of open-in-new-window. 

Marking as blocking, because I think we do need to at the very least make an explicit UX decision here before we ship Thunderbird 3.
Component: Mail Window Front End → Toolbars and Tabs
Flags: wanted-thunderbird3?
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Priority: -- → P1
QA Contact: front-end → toolbars-tabs
Whiteboard: [b3ux]
Target Milestone: --- → Thunderbird 3.0b3
Version: unspecified → Trunk
Assignee: nobody → dmose
(In reply to comment #10)
> I don't understand this analogy: Firefox has no double-click handler at all.

Exactly, firefox doesn't do anything differently for double click vs normal click. We do, but we already have the middle mouse button to open new messages in tabs. (The analogy is more like "think of tb as a super advanced web mail".)
OK, I get the analogy now.  The fact that Firefox doesn't do anything specific with double-click bindings seems like neither a feature nor a bug to me.  And since people don't have a broad expectation of double-click doing something conceptually incompatible, it seems completely reasonable to appropriate that action to help address the use-case here.
Only to put out my use case here; I do not use the preview pane at all now that the bug for rendering in a new tab is fixed when no preview. The middle click to open in new tab is helpful, but not necessarily as natural as double clicking. With middle click handling this now, I suppose that an extension might be the best way to alter the behavior of the how the mail is opened. Either way is fine with me, but it would be funny to have it set that both middle click and double click opens in new tab.
(In reply to comment #13)
> Either way is fine with me, but it would be funny to have it set that both
> middle click and double click opens in new tab.

I don't think that's a real issue, not everyone will discover middle click easily.

One suggestion would be that double click opens tab and brings it to focus, but a middle click opens it in the background.
Why would double click be so much more discoverable? (Old mac mouses aside.) 

Toggled background loading is shift+middle click in ff, I'd hope we use that too once someone implements background tab opening in tb.
Would this affect local saved .eml files too? I think Thunderbird should follow Firefox. If we open a local saved .html file in Firefox, it will be opened in a new Tab. If we open a local saved .eml file, Thunderbird should open and show the .eml file in a new tab too instead of opening a seperate window. So it should be the same behavior for .eml files, like if you open a mail from your inbox in a new tab directly. Or would this be a separate bug?
Whiteboard: [b3ux] → [b3ux] [m5]
Whiteboard: [b3ux] [m5] → [b3ux] [m4]
Status: NEW → ASSIGNED
I would advocate against using double-click as a way of opening messages in a new tab, since that would make it impossible to replace an already opened message tab with new content (without having to drag the message list entry onto the tab/window or something similar).

CTRL+BUTTON3 for foreground loading and SHIFT+BUTTON3 for background is imo a better idea, keeping BUTTON1 as a way to override the current message window content with a new message.
Whiteboard: [b3ux] [m4] → [b3ux][m4]
My $.02: have an option in the preferences to define the default behaviour. Even now that I know middle click works, I still expect double-click to do it. Thunderbird isn't a browser, and I don't expect it to behave that way.

Also, there should be a way to just configure "open *all* new windows as tabs", because I can't seem to find any way to open a reply in a new tab.

Right now, the tab feature just feels tacked on and not nearly as useful as it could be. I would love for everything that's not modal to live in a tab. That I can't have that, even through the config system, makes me feel like just turning it off entirely.
While this would be a great thing to have if someone feels like jumping on this, if this were the last bug standing, we wouldn't wait for it.  Switching from blocking+ to wanted+.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Whiteboard: [b3ux][m4]
Assignee: dmose → sid.bugzilla
Looks like there are a few major bugs in the tabmail/gDBView interaction that are either
- fixed by bug 474701, or
- just not worth fixing right now because of the extensive changes in that bug.

I'm putting this on hold until that lands.
Attached patch WIP (obsolete) — Splinter Review
This patch is built on top of the patches in bug 474701.

In this patch --
- stuff to open tabs in the background, so that we don't go crazy loading messages through our multiplexed message pane and get the URLs to cancel each other! Seems to work for messages and folders. I need to figure out the glodaSearch pane, though.
- a pref, hidden for now.
Depends on: 474701
Flags: blocking-thunderbird3- → blocking-thunderbird3?
Requesting blocking-thunderbird3 again, as I think this is a pretty high value feature, and it also looks quite doable with the folder display patch in bug 474701.
Attached patch WIP #2 (obsolete) — Splinter Review
This mostly works, but there seem to be a few edge cases where the selection code starts acting up. Looks like asuth's going to make some improvements to the selection code in the next iteration of his folder display patch.
Attachment #380391 - Attachment is obsolete: true
Attached patch WIP #3, now with UI (obsolete) — Splinter Review
The UI addition is an extra option in the "Open messages in" radio group. I've added code to migrate the pref if it's defined.
Attachment #380625 - Attachment is obsolete: true
Attached patch WIP #3, rebased (obsolete) — Splinter Review
Rebased over the newest folder display/gloda search patches. Needs fixing.
Attachment #381869 - Attachment is obsolete: true
Attached patch WIP #4 (obsolete) — Splinter Review
This works a lot better, mainly because we have a real fake selection tree now! I'm still waiting for a few bugs with the bug 474701 patch to get fixed.
Attachment #381977 - Attachment is obsolete: true
Attached patch WIP #5 (obsolete) — Splinter Review
This moves the messenger command line handler over to JS, and allows the search engines to handle translation from .wdseml/.mozeml to a message header.

Still to do:
- I haven't tested it thoroughly yet.
- Suite's broken with this patch -- fixing it should be fairly simple, though.
Attachment #382565 - Attachment is obsolete: true
Blocks: 498007
Attached patch WIP #6 (obsolete) — Splinter Review
Getting there -- need to get tests running, and hopefully that should be it.
Attachment #382954 - Attachment is obsolete: true
Attached patch WIP #6.5 (obsolete) — Splinter Review
Even closer...
Attachment #383438 - Attachment is obsolete: true
A few quick notes on the patch (which is looking great and I'd like to get into the tree as soon as possible):

- Please create and use constants for the tab behavior enumeration values.  Also, perhaps consider just using strings for the values instead of numbers.  I feel like "new tab", "existing window", and "new window" are much more readable as preferences go.  I think for symbolic analysis reasons we would still want the constants in the code (even if the values are now strings).

- Please add a unit test for your change to JSTreeSelection to its unit test file.

- I suggest moving the code that propagates the selection from the fake selection to the real selection to JSTreeSelection.  We've already started augmenting it with functionality that recognizes that it is used to be a fake, so we might as well centralize the nsITreeSelection logic there as much as possible when appropriate.

- If you already have mozmill test(s) covering the background tab stuff, please make sure to hg add them.  If you don't, please be sure to write then hg add :)

As a side note that I don't think your patch needs to address, the open message in tab/window/etc. stuff seem like great fodder for being exposed via STEEL.
Attached patch WIP #7 (obsolete) — Splinter Review
> - Please create and use constants for the tab behavior enumeration values.

Done.

> Also, perhaps consider just using strings for the values instead of numbers.

Sticking with numbers for now.

> - If you already have mozmill test(s) covering the background tab stuff, please
> make sure to hg add them.  If you don't, please be sure to write then hg add :)

I've fixed up the middle click tests to cover all the background tab stuff. I've also created a test-opening-messages.js, but I haven't figured out how to test opening multiple windows at once yet.
Attachment #383505 - Attachment is obsolete: true
Attached patch patch, v1 (obsolete) — Splinter Review
> - Please add a unit test for your change to JSTreeSelection to its unit test
> file.

Done.

> - I suggest moving the code that propagates the selection from the fake
> selection to the real selection to JSTreeSelection.  We've already started
> augmenting it with functionality that recognizes that it is used to be a fake,
> so we might as well centralize the nsITreeSelection logic there as much as
> possible when appropriate.

Done, and a couple of unit tests added.
Attachment #383957 - Attachment is obsolete: true
Attached patch patch, v1.01 (obsolete) — Splinter Review
Oops, missed the toggleSelect unit test for some reason.
Attachment #384004 - Attachment is obsolete: true
Attachment #384008 - Flags: superreview?(bienvenu)
Attachment #384008 - Flags: review?(mnyromyr)
Attachment #384008 - Flags: review?(bugmail)
Attachment #384008 - Flags: review?(bienvenu)
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

Asking asuth to review the changes to tabmail, the folder display widget and friends, and tests; Mnyromyr to review the suite parts (since I don't build suite, I hope I haven't broken it :) ); and bienvenu to review the commandline handler parts (as well as sr, because there's an API change).
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

Oops, forgot to ask Bryan to UI review the general tab interaction and pref dialog changes.
Attachment #384008 - Flags: ui-review?(clarkbw)
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

humph, it'd be great to have an additional review from you for the Spotlight changes.
Attachment #384008 - Flags: review?(david.humphrey)
Ugh, so xpcshell doesn't like importing resource://app URIs too early (i.e. during component registration) -- I've moved both the MailUtils.js imports in the command-line handlers to within the handle functions.
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

rich review (no login required):
http://reviews.visophyte.org/r/384008/

as per my blog post:
http://www.visophyte.org/blog/2009/06/20/review-board-and-bugzilla-reviews-take-2/

"exported" review for eternity:

This is fantastic work!  This constitutes my first review pass (on this patch,
no revised patch required); given the size of the patch, I plan to do another
pass in the near future (after I have run the unit tests and run with the
patch).


on file: mail/base/content/folderDisplay.js line 1725
>     let treeSelection = this.treeSelection; // potentially magic getter
>     if (treeSelection) {
>       let foundAll = true;
>       let treeSelection = this.treeSelection;

Please lose the redundant "let treeSelection".


on file: mail/base/content/mailWindowOverlay.js line 1690
>       /**
>        * @param aFolder The nsIMsgFolder to display.
>        * @param aMsgHdr Optional message header to display.
>        */
>       openTab: function(aTab, {folder: aFolder, msgHdr: aMsgHdr,
>                                background: aBackground}) {

I am fine with not updating this documentation (and its friends) right now.  I
have some documentation tooling in the works and I think it would be a waste
to try and fix-up this documentation right now if it is just going to need to
be re-done.

(I am saying this just in case any other reviewers call this out.)


on file: mail/base/content/mailWindowOverlay.js line 2011
> /**
>  * UI-triggered command to open the currently selected folder(s) in new tabs.
>  * @param aBackground [optional] if true, then the folder tab is opened in the
>  *                    background.
>  */
> function MsgOpenNewTabForFolder(aBackground)

Please specify the default behavior if aBackground is omitted.  Although
undefined pretty much ends up the same as false, some code will check for
undefined and do something special.


on file: mail/base/content/mailWindowOverlay.js line 2034
> function MsgOpenNewTabForMessage(aMsgHdr, aBackground)

I don't think this function needs to exist anymore, and by continuing to exist
it just adds confusion.  Its two callers (ThreadTreeContextMenuNewTab and
MsgOpenSelectedMessages) could easily use the openTab on their own.


on file: mail/base/content/mailWindowOverlay.js line 2059
>     // Open all the tabs in the background, except for the last one

This behavior should be documented somewhere.  MailConsts.OpenMessageBehavior
is probably best.


on file: mail/base/content/mailWindowOverlay.js line 2086
> function MsgOpenSelectedMessageInExistingWindow()
> {
>   return MailUtils.openMessageInExistingWindow(gFolderDisplay.selectedMessage,
>                                                gFolderDisplay.view);
> }

This method does not need to exist anymore if all it does is call a library
function.


on file: mail/base/content/messageDisplay.js line 328
>    *  @param aDontReloadMessage [optional] true if you don't want to make us
>    *                            call reloadMessage even if the conditions are
>    *                            right for doing so. Use with a great deal of
>    *                            discretion.

"Use with a great deal of discretion" is a red flag in a comment.  Please
explain why you would want to prevent the reload.  It's fine to explain the
one current case (and ideally cite it like |FolderDisplayWidget.foo|) rather
than pontificate on all the conceivable reasons one would want to pass true.


on file: mail/base/content/messageWindow.js line 120
>   /**
>    * Override this to make sure that we don't try to do stuff with quick-search
>    */
>   onDisplayingFolder:

Ideally we could just push the quick-search and related 3pane stuff down into
its own subclass.  You don't need to do that/probably shouldn't do that in
this patch, but please add an "XXX" and a comment (possibly what I just said)
so the ugly is more obvious.


on file: mail/base/content/messageWindow.js line 179
>   /**
>    * This is supposed to be true when we know we're going to load another message
>    * shortly, so clearDisplay et al shouldn't close the window. It is the
>    * responsibility of calling code to set this to true when needed. This is
>    * automatically set to false once a message has been loaded.
>    *
>    * This is true initially, because we know we're going to load another message
>    * shortly.
>    */
>   this.aboutToLoadMessage = true;

Maybe this should be fused with loadingMessage/loadedMessage that I introduced
for testing purposes?


on file: mail/base/content/messageWindow.js line 256
>   onSelectedMessagesChanged: function () {

If you understand why I had us call window.close() from two different places
(here and in clearDisplay), please add comments explaining.  If you don't,
please add an "XXX" comment that indicates we're not doing it for any
well-understood reason.


on file: mail/base/content/messageWindow.js line 440
>  * Load the given message into this window, and bring it to the front. This is
>  * supposed to be called whenever a message is supposed to be displayed in this
>  * folder.

Do you mean "in this window." at the end of the comment?


on file: mail/base/content/msgMail3PaneWindow.js line 1026
> function MigrateOpenMessageBehavior()

Hooray for preference migration!


on file: mail/base/content/widgetglue.js line 77
> function GetMsgFolderFromUri(uri, checkFolderAttributes)

Please give this a doxygen block that marks it as "@deprecated" and tells
everyone to just use the new MailUtils module.  Please cite the target like
"|MailUtils.getFolderForURI|", it will help my doc tooling.  Or if there is a
better standard, use that...


on file: mail/components/MailConsts.js line 38
> /**
>  * This is a place to store constants and enumerations that are needed only by
>  * JavaScript code, especially component/module code.
>  */

I do not think this file or MailUtils should be in a "components" directory. 
It confuses my brain.  Please make a "modules" directory or something like
that; ask bienvenu/dmose what they would like, really.


on file: mail/components/MailUtils.js line 57
>   get _prefBranch MailUtils_get_prefBranch()
>   {

Since you are creating new files here, you should use same-line squiggly
braces.

See: https://developer.mozilla.org/en/JavaScript_style_guide


on file: mail/components/MailUtils.js line 65
>   /**
>    * Discover all folders. This is useful during startup, when the folder tree
>    * hasn't been initialized yet.
>    */

Why is it useful?  Please document.


on file: mail/components/MailUtils.js line 79
>    * Get the nsIMsgFolder corresponding to this file. This just looks at all
>    * folders and does a direct match.

Please provide a brief explanation of why I would want to do this.  It's not
intuitively obvious.


on file: mail/test/mozmill/folder-display/test-right-click-middle-click.js line 271
> function _generate_background_foreground_tests(aTests) {

Clever!  It is indeed quite nice to avoid needless duplication of test code.


on file: mailnews/base/src/nsMailNewsCommandLineHandler.js line 49
>   get _messenger nsMailNewsCommandLineHandler_getMessenger()
>   {

well outside of my file-system stomping grounds, but... I suggest same-line
squiggly braces...


on file: mailnews/base/src/nsMailNewsCommandLineHandler.js line 136
>     let mailFlag = aCommandLine.handleFlag("mail", false);
>     if (mailFlag)
>     {
>       // Focus the 3pane window if one is present, else open one
>       let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
>                              .getService(Ci.nsIWindowMediator);
>       let mail3PaneWindow = windowMediator.getMostRecentWindow("mail:3pane");
>       if (mail3PaneWindow)
>       {
>         mail3PaneWindow.focus();
>       }
>       else
>       {
>         let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
>                               .getService(Ci.nsIWindowWatcher);
>         windowWatcher.openWindow(null, "chrome://messenger/content/", "_blank",
>             "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar,dialog=no",
>             null);
>       }
>       aCommandLine.preventDefault = true;
>     }

why isn't this in MailUtils?
Ugh, bad news about the object destructuring on the arguments, I get lots of:
Strict Warning: reference to undefined property "msgHdr" [chrome javascript]
    chrome://messenger/content/mailWindowOverlay.js:1695     (nostack)
(note that the syntax of the error is specific to my error-reporting extension, but the exception should show up in the error console too...)

Where the code in question is:
      openTab: function(aTab, {folder: aFolder, msgHdr: aMsgHdr,
                               background: aBackground}) {

So presumably this idiom is only good when all arguments are supplied.  I still think that passing the options dictionary style is the right thing, but I think we might have to bail on the cool syntax.  We could attempt to address the problem by having tabmail fix things up prior to dispatching the call to openTab, but I'm cringing as I type that.

I don't think we should introduce any new strict warnings, so that needs to be sorted.
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

Opening tabs in the background is awesome!

Please add a unit test that verifies that a background tab does the 'right thing' if the message gets deleted out from under it.  I know you spent a lot of time getting that to work, so presumably you want to make sure we don't regress that.  test-deletion-with-multiple-displays may be a reasonable existing test to leverage.
Attachment #384008 - Flags: review?(bugmail) → review+
Thank you very much for the work you've spent on this. I've tested your build under Windows XP and you did a great job. Opening messages in tabs as default action via double click is just working fine. That patch makes the whole tabbing feature interesting, because I always open message via mouse. I never used the right click menu to open a message. Opening external saved .eml files in a new tab, instead of a separate window isn't related to this bug, or? I guess this bug just depends on the default double click action within Thunderbird. So opening .eml files in a tab too, should be filled in another bug, or am I wrong?
One thing, I've tried your build and it is better but When you write a new e-mail, it would be nice to have it in a new tab instead of a new window or when you double click in the TAB BAR, it creates a new message window as if you wanted to send a new e-mail.
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

tabs in the background rock!  I got some threadpane movement when middle-clicking on messages.  Can we make sure it doesn't jump around?  Otherwise everything works great. Pref looks good too.

Thanks for the try server builds, that is really handy.
Attachment #384008 - Flags: ui-review?(clarkbw) → ui-review+
The patch also needs to be updated for the changes made on bug 498344, changeset http://hg.mozilla.org/comm-central/rev/e4ef3c76755d.
(In reply to comment #42)
> Ugh, bad news about the object destructuring on the arguments, I get lots of:
> Strict Warning: reference to undefined property "msgHdr" [chrome javascript]
>     chrome://messenger/content/mailWindowOverlay.js:1695     (nostack)
> (note that the syntax of the error is specific to my error-reporting extension,
> but the exception should show up in the error console too...)
> 
> Where the code in question is:
>       openTab: function(aTab, {folder: aFolder, msgHdr: aMsgHdr,
>                                background: aBackground}) {
> 
> So presumably this idiom is only good when all arguments are supplied.  I still
> think that passing the options dictionary style is the right thing, but I think
> we might have to bail on the cool syntax.  We could attempt to address the
> problem by having tabmail fix things up prior to dispatching the call to
> openTab, but I'm cringing as I type that.
> 
> I don't think we should introduce any new strict warnings, so that needs to be
> sorted.

Ugh, sucks. Can we supply and ask people to always supply all arguments? If not, bah. :(

(In reply to comment #44)
> Thank you very much for the work you've spent on this. I've tested your build
> under Windows XP and you did a great job. Opening messages in tabs as default
> action via double click is just working fine. That patch makes the whole
> tabbing feature interesting, because I always open message via mouse. I never
> used the right click menu to open a message. Opening external saved .eml files
> in a new tab, instead of a separate window isn't related to this bug, or? I
> guess this bug just depends on the default double click action within
> Thunderbird. So opening .eml files in a tab too, should be filled in another
> bug, or am I wrong?

That isn't part of this bug, and with the way we currently do things with saved eml files, will require a bit of additional work. Please file a followup bug for this.

(In reply to comment #45)
> One thing, I've tried your build and it is better but When you write a new
> e-mail, it would be nice to have it in a new tab instead of a new window or
> when you double click in the TAB BAR, it creates a new message window as if you
> wanted to send a new e-mail.

Please file a followup bug. With the current state of our compose code, this will probably require even more work than opening eml files in tabs. I'm not sure if it's even possible to do this.
I filled Bug499922, which asks for the ability to open external saved .eml files in a tab too. 

Opening  the compose window in a tab was requested before, there is already Bug 449299 for this.
Attachment #384008 - Flags: superreview?(bienvenu)
Attachment #384008 - Flags: superreview+
Attachment #384008 - Flags: review?(bienvenu)
Attachment #384008 - Flags: review+
Comment on attachment 384008 [details] [diff] [review]
patch, v1.01

modulo Andrew's comments, this looks good.
Depends on: 500147
Attached patch patch, v2 (obsolete) — Splinter Review
I've fixed most of asuth's review nits, and left comments at <http://reviews.visophyte.org/r/384008/>. I haven't fixed two things:

> on file: mail/base/content/messageWindow.js line 179
> >   /**
> >    * This is supposed to be true when we know we're going to load another message
> >    * shortly, so clearDisplay et al shouldn't close the window. It is the
> >    * responsibility of calling code to set this to true when needed. This is
> >    * automatically set to false once a message has been loaded.
> >    *
> >    * This is true initially, because we know we're going to load another message
> >    * shortly.
> >    */
> >   this.aboutToLoadMessage = true;
> 
> Maybe this should be fused with loadingMessage/loadedMessage that I introduced
> for testing purposes?

So aboutToLoadMessage, messageLoading and messageLoaded seem to represent different states of message loading.

If I understand correctly:
on message window open: messageLoading/messageLoaded = false, aboutToLoadMessage = true (or when displayMessage is called: aboutToLoadMessage -> true)
when onDisplayingMessage is called: aboutToLoadMessage -> false, messageLoading -> true, messageLoaded -> false
onEndAllAttachments: messageLoading -> false, messageLoaded -> true

So aboutToLoadMessage indicates a state before the message has started loading, and becomes false when a message starts to load (onDisplayingMessage).

Do you see a way to fuse aboutToLoadMessage with the other two? Right now I'd prefer to keep it separate, if only for clarity.

> on file: mail/base/content/widgetglue.js line 77
> > function GetMsgFolderFromUri(uri, checkFolderAttributes)
> 
> Please give this a doxygen block that marks it as "@deprecated" and tells
> everyone to just use the new MailUtils module.  Please cite the target like
> "|MailUtils.getFolderForURI|", it will help my doc tooling.  Or if there is a
> better standard, use that...
> 
> 
> on file: mail/components/MailConsts.js line 38
> > /**
> >  * This is a place to store constants and enumerations that are needed only by
> >  * JavaScript code, especially component/module code.
> >  */
> 
> I do not think this file or MailUtils should be in a "components" directory. 
> It confuses my brain.  Please make a "modules" directory or something like
> that; ask bienvenu/dmose what they would like, really.
> 
> 
> on file: mail/components/MailUtils.js line 57
> >   get _prefBranch MailUtils_get_prefBranch()
> >   {
> 
> Since you are creating new files here, you should use same-line squiggly
> braces.
> 
> See: https://developer.mozilla.org/en/JavaScript_style_guide
> 
> 
> on file: mail/components/MailUtils.js line 65
> >   /**
> >    * Discover all folders. This is useful during startup, when the folder tree
> >    * hasn't been initialized yet.
> >    */
> 
> Why is it useful?  Please document.
> 
> 
> on file: mail/components/MailUtils.js line 79
> >    * Get the nsIMsgFolder corresponding to this file. This just looks at all
> >    * folders and does a direct match.
> 
> Please provide a brief explanation of why I would want to do this.  It's not
> intuitively obvious.
> 
> 
> on file: mail/test/mozmill/folder-display/test-right-click-middle-click.js line
> 271
> > function _generate_background_foreground_tests(aTests) {
> 
> Clever!  It is indeed quite nice to avoid needless duplication of test code.
> 
> 
> on file: mailnews/base/src/nsMailNewsCommandLineHandler.js line 49
> >   get _messenger nsMailNewsCommandLineHandler_getMessenger()
> >   {
> 
> well outside of my file-system stomping grounds, but... I suggest same-line
> squiggly braces...
> 
> 
> on file: mailnews/base/src/nsMailNewsCommandLineHandler.js line 136
> >     let mailFlag = aCommandLine.handleFlag("mail", false);
> >     if (mailFlag)
> >     {
> >       // Focus the 3pane window if one is present, else open one
> >       let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
> >                              .getService(Ci.nsIWindowMediator);
> >       let mail3PaneWindow = windowMediator.getMostRecentWindow("mail:3pane");
> >       if (mail3PaneWindow)
> >       {
> >         mail3PaneWindow.focus();
> >       }
> >       else
> >       {
> >         let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
> >                               .getService(Ci.nsIWindowWatcher);
> >         windowWatcher.openWindow(null, "chrome://messenger/content/", "_blank",
> >             "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar,dialog=no",
> >             null);
> >       }
> >       aCommandLine.preventDefault = true;
> >     }
> 
> why isn't this in MailUtils?

Is this really useful enough for us to maintain two copies of it? I haven't moved it into MailUtils for now, but I don't mind doing so.

I've also modified the deletion with multiple displays unit test to care about background tabs.

humph, Mnyromyr: any chance you could review this soon?
Attachment #384008 - Attachment is obsolete: true
Attachment #384952 - Flags: superreview+
Attachment #384952 - Flags: review?(mnyromyr)
Attachment #384952 - Flags: review?(david.humphrey)
Attachment #384008 - Flags: review?(mnyromyr)
Attachment #384008 - Flags: review?(david.humphrey)
Attached patch patch, v2.01 (obsolete) — Splinter Review
Oops, missed some tabmail changes.

Since humph has indicated his unavailability, transferring review for spotlight code to bienvenu.
Attachment #384952 - Attachment is obsolete: true
Attachment #384960 - Flags: review?(mnyromyr)
Attachment #384960 - Flags: review?(bienvenu)
Attachment #384952 - Flags: review?(mnyromyr)
Attachment #384952 - Flags: review?(david.humphrey)
Depends on: 500284
Depends on: 500435
has this bit-rotted? hg qimport fails on mail/base/content/tabmail.xml - just a heads up - I'll look into it here.
yeah, standard8 bit-rotted you today
Attached patch patch, v2.02 (obsolete) — Splinter Review
Unbitrotted. This patch also has fixes to a few inconsistent comments.
Attachment #384960 - Attachment is obsolete: true
Attachment #385237 - Flags: review?(mnyromyr)
Attachment #385237 - Flags: review?(bienvenu)
Attachment #384960 - Flags: review?(mnyromyr)
Attachment #384960 - Flags: review?(bienvenu)
Comment on attachment 385237 [details] [diff] [review]
patch, v2.02

Spotlight opens messages in tabs now - lovely.
Attachment #385237 - Flags: review?(bienvenu) → review+
Mnyromyr, I don't think you were cc'ed so you may have missed that this cleared the other reviewing hurdles.  (Oh, bugzilla...)

Will you be able to get to this review soon?  If not, would bienvenu, Neil, Standard8, or someone else make acceptable stand-ins?

Thanks.
Comment on attachment 385237 [details] [diff] [review]
patch, v2.02

This is a first dry-run review comment, I'll finish after a functional check tomorrow.
I specifically didn't take a look at /mail code, just at /mailnews and /suite.


>diff --git a/mailnews/base/src/nsMailNewsCommandLineHandler.js b/mailnews/base/src/nsMailNewsCommandLineHandler.js
>+  handle: function nsMailNewsCommandLineHandler_handle(aCommandLine) {
...
>+    if (mailURL && mailURL.length > 0) {
>+      let msgHdr = null;
>+      if (mailURL.indexOf("mailbox-message://") == 0 ||
>+          mailURL.indexOf("imap-message://") == 0 ||
>+          mailURL.indexOf("news-message://") == 0) {

indexOf is pretty wasteful, especially if you only care for index 0. Use start-anchored regex instead.

>+        // This might be a standard message URI, or one with a messageID
>+        // parameter. Handle both cases.
>+        let messageIDIndex = mailURL.toLowerCase().indexOf("?messageid=");
>+        if (messageIDIndex != -1) {
>+          // messageID parameter
>+          // Convert the message URI into a folder URI
>+          let folderURI = mailURL.slice(0, messageIDIndex)
>+                                 .replace("-message", "");
>+          // Get the message ID ("?messageId=" is 11 characters)
>+          let messageID = mailURL.slice(messageIDIndex + 11);

Use a constant for "?messageId=" above and its length here.

>+        let neckoURL;
>+        try {
>+          neckoURL = ioService.newURI(mailURL, null, null);
>+        }
>+        catch (e) {
>+          // We failed to convert the URI. Oh well.
>+        }
>+
>+        if (neckoURL && neckoURL instanceof Ci.nsIMsgMessageUrl)
>+          msgHdr = neckoURL.messageHeader;

Properly initialize neckoURL; and you don't need to "neckoURL &&" it later. 

>diff --git a/suite/modules/MailUtils.js b/suite/modules/MailUtils.js

First of all, JavaScript modules should have .jsm as their file extension.
Second, the right place here would be b/suite/mailnews/modules/MailUtils.js.
Third, new files under /suite/mailnews/ should adhere to the "align {} vertically" policy (I really need to write a doc on that). 

>diff --git a/suite/modules/Makefile.in b/suite/modules/Makefile.in

Wrong file, see above.


More tomorrow.
(In reply to comment #58)

> First of all, JavaScript modules should have .jsm as their file extension.

FYI, in /mail land AFAIK we're moving away from that, because most IDEs don't know about .jsm extensions.
(In reply to comment #59)
> FYI, in /mail land AFAIK we're moving away from that, because most IDEs don't
> know about .jsm extensions.

Not even mxr understands.  It's very sad.
Attached patch patch, v3Splinter Review
Unbitrotted to trunk and fixed up tab save/restore. I've tried to address all of Mnyromyr's nits (apart from renaming MailUtils.js to MailUtils.jsm).

asuth: there are several non-trivial changes here compared to v1, so could you go over the patch once more?
Attachment #385237 - Attachment is obsolete: true
Attachment #385601 - Flags: review?(mnyromyr)
Attachment #385601 - Flags: review?(bugmail)
Attachment #385237 - Flags: review?(mnyromyr)
Attachment #385601 - Flags: review?(bugmail) → review+
Comment on attachment 385601 [details] [diff] [review]
patch, v3

Looks great!


on file: mail/locales/en-US/chrome/messenger/messenger.properties line 475
> fileNotFoundTitle = File Not Found
> #LOCALIZATION NOTE(fileNotFoundMsg): %S is the filename
> fileNotFoundMsg = The file %S does not exist.
> 
> fileEmptyTitle = File Empty
> #LOCALIZATION NOTE(fileNotFoundMsg): %S is the filename
> fileEmptyMsg = The file %S is empty.

Localization note for fileEmptyTitle references fileNotFoundMsg.
Attachment #385601 - Flags: review?(mnyromyr) → review+
Comment on attachment 385601 [details] [diff] [review]
patch, v3

Just one final nit: as talked about on IRC, SM's messageWindow.js doesn't copy with the aMsgHdr argument, it needs an URI string.

>+  openMessageInNewWindow: function MailUtils_openMessageInNewWindow(aMsgHdr)
>+  {
>+    let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"]
>+                          .getService(Ci.nsIWindowWatcher);
>+    windowWatcher.openWindow(null,
>+        "chrome://messenger/content/messageWindow.xul", "_blank",
>+        "all,chrome,dialog=no,status,toolbar", aMsgHdr);
>+  }

r=me with that.
Pushed to c-c with nits fixed: http://hg.mozilla.org/comm-central/rev/01bd846dbb7e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking-thunderbird3?
Blocks: 500488
I just pushed a bustage fix: http://hg.mozilla.org/comm-central/rev/e3793301cf0d

This fixes errors with loading the mailnews module, that would result in an empty 3-pane window layout being shown and errors on the error console such as:

Failed to load XPCOM component: /Users/moztest/comm/main/tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libmail.dylib
Depends on: 501225
Comment on attachment 385601 [details] [diff] [review]
patch, v3

>-  return catMan->AddCategoryEntry("command-line-handler", "m-mail",
>-                                  NS_MAILSTARTUPHANDLER_CONTRACTID,
>-                                  PR_TRUE, PR_TRUE, nsnull);

>+  contractID: "@mozilla.org/messenger/clh;1",
This broke suite, because it needs to find the mail startup handler via tha mail startup handler contract id :-(
Depends on: 501195
Flags: in-testsuite+
Depends on: 505044
This thing is not completely resolved and fixed.  When you open an attached e-mail, it opens in a new window.  Besides, I don't even see anything related to tabs in the configurations.

Also, when you open an e-mail faile in Windows (I am using presently Windows Vista) it opens the mail in a new window also.  It would have to work like Firefox, open any e-mail, from which ever way you do it, in a NEW TAB.  When you double click an e-mail in a folder, it works but not when you double click on an attached e-mail or on an eml file in Windows.

This thing would need to be fixed...
Attached e-mails and .eml files use the same code path and are only supported by standalone message windows right now.  I agree that it is an odd limitation.  Bug 499922 covers that problem.
You need to log in before you can comment on or make changes to this bug.