515218 - "Open in Folder" in search doesn't respect tab/window pref

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Kent James (:rkent)]

STR:

1) in folder pane, select a folder
2) right click, "Search"
3) enter a search term that matches some messages, say "Subject Contains Test"
4) click on one of the matched messages
5) select "Open Message Folder"

Expected (and previous, and current SM) results: a new window opens, with the message's folder selected in the message pane, and the message selected in the thread pane.

Actual results: new window opens, with the first account selected in the folder pane, and the no-message display in the thread pane (not sure what that's called). Error console shows "invalid 'in' operand window.arguments[1] msgMail3PaneWindow.js Line: 507"

Comment 1

[Mark Banner (:standard8)]

Yeah this seems broken, so we should fix it in some manner.

Comment 2

[Wayne Mery (:wsmwk)]

first, error console also shows

Warning: Empty string passed to getElementById().

Error: Components.classes['@mozilla.org/satchel/form-fill-controller;1'] is undefined
Source File: chrome://global/content/bindings/browser.xml
Line: 400

Comment 4

[David Ascher (:davida)]

Attachment: [checked in] patch v1

A couple of things:

1) the loadExtraTabs function was assuming that just because window.arguments.length was greater than 1, that one could look inside of window.arguments[1], but it was undefined, because...

2) the GoToFolder function was calling MsgOpenNewWindowForFolder with a message, as opposed to a folder and messagekey.

That said, looking at the UI w/ clarkbw, a few things seemed clear:

1) it's weird to have Open open in tab, but Open Message Folder open a new window.  For consistency, I changed the code to open (selecting the folder & the message) in a tab.

2) the words "Open Message Folder" are weird -- the message can be assumed as the object (cf. "Open" button two doors over), so "Open in folder" makes more sense.  Good thing the string freeze is temporarily lifted.

3) I feel like Harry when he goes somewhere he shouldn't have and Dumbledore scolds him for disobeying.  Some files are better left unobserved.

4) I feel like i'm turning into philor.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 401330 [details] [diff] [review]
[checked in] patch v1

like it

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 401330 [details] [diff] [review]
[checked in] patch v1

General comments:

- The "Open" button for opening a message respects the mail.openMessageBehavior preference. "Open in folder" doesn't.

- The main mail window isn't focussed when I open in folder (tab) versus just opening a message in a tab.


>-function GoToFolder()
>+function OpenInFolder()
> {
>-  MsgOpenNewWindowForFolder(gFolderDisplay.selectedMessage);
>+  let msg = gFolderDisplay.selectedMessage;
>+  let tabmail = window.opener.document.getElementById('tabmail');
>+  tabmail.openTab("folder", {folder: msg.folder, msgHdr: msg});
> }

It feels like we should move some of this function into MailUtils.js alongside the message opening ones, as it could be useful to extensions. Not necessary for this bug, but please file a new bug if you agree and choose not to fix it here.

Comment 7

[Rain [:rain1] (inactive)]

(In reply to comment #6)
> 
> It feels like we should move some of this function into MailUtils.js alongside
> the message opening ones, as it could be useful to extensions. Not necessary
> for this bug, but please file a new bug if you agree and choose not to fix it
> here.

Yes, that would be excellent. It would be a good idea to also handle the case where there are no 3pane windows open.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 401330 [details] [diff] [review]
[checked in] patch v1

Discussing this on irc. davida wishes to pass the patch to sid0. As sid0 may not get to it until after the weekend, and we want to get the strings landed, I've agreed that this patch leaves us in a better state than we are now (button does something semi-useful) and therefore we can land it with the follow up comments to be done by sid0.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 401330 [details] [diff] [review]
[checked in] patch v1

Checked in: http://hg.mozilla.org/comm-central/rev/930310b0e8c5

Comment 10

[Mark Banner (:standard8)]

The second patch should have no l10n impact, so changing whiteboard appropriately.

Comment 11

[Rimas Kudelis [use needinfo to ping]]

I think "Open in folder" is misleading, because the message pane may be hidden. May I suggest using "Show in folder" (much like "Show in Finder") instead?

Comment 12

[Rain [:rain1] (inactive)]

Attachment: WIP

There's a lot of ugliness in this patch, and still a lot more to do.

Comment 13

[Rain [:rain1] (inactive)]

Attachment: handle all cases where a 3pane window is open, v1

Per discussion with bienvenu, let's get this in first, and handle the case where no 3pane window is open in a separate patch.

Comment 14

[David :Bienvenu]

Comment on attachment 403301 [details] [diff] [review]
handle all cases where a 3pane window is open, v1

+function MsgDisplayMessageInFolderTab(aMsgHdr) {

has 3 temp vars that aren't strictly needed as vars:

openMessageBehavior, viewIndex, and index

I ran the mozmill tests, and they seemed fine. And a quick test of the basic bug looked good too, but I;d like to run with this a bit more.

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 403301 [details] [diff] [review]
handle all cases where a 3pane window is open, v1

I'm noticing that when clicking on ( Open in Folder ) and I have open in tab as the default the thread list doesn't scroll to show the selected message in view.  The message is displayed in the message reader but when the open window pref is used the reuse of a tab scrolls like this correctly.  

Otherwise things look good.

Comment 16

[Rain [:rain1] (inactive)]

Attachment: [checked in] handle all cases where a 3pane window is open, v2

clarkbw's bug fixed and tests for scrolling added.

David: I've removed viewIndex and index, but I think that without openMessageBehavior as a separate var it looks a bit too complicated. Does this look fine?

Comment 17

[David :Bienvenu]

(In reply to comment #16)
> 
> David: I've removed viewIndex and index, but I think that without
> openMessageBehavior as a separate var it looks a bit too complicated. Does this
> look fine?

yes, that's OK.

I'll try the patch this evening...

Comment 18

[David :Bienvenu]

Comment on attachment 403365 [details] [diff] [review]
[checked in] handle all cases where a 3pane window is open, v2

thx, Sid.

Comment 19

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 403365 [details] [diff] [review]
[checked in] handle all cases where a 3pane window is open, v2

looks good to me

Comment 20

[Rain [:rain1] (inactive)]

Comment on attachment 403365 [details] [diff] [review]
[checked in] handle all cases where a 3pane window is open, v2

http://hg.mozilla.org/comm-central/rev/e615de59a7d0

Comment 21

[David :Bienvenu]

So, what's left is handling the case where there's no 3-pane window up? What happens in that case? Does it still feel like a blocker?

Comment 22

[Rain [:rain1] (inactive)]

Attachment: [WIP] handle opening in a new window

There seems to be one issue -- the per-tab throbber keeps spinning till infinity -- but I see it without the patch too.

Comment 23

[Rain [:rain1] (inactive)]

(In reply to comment #22)
> There seems to be one issue -- the per-tab throbber keeps spinning till
> infinity -- but I see it without the patch too.

Filed bug 525777 about this.

Comment 24

[Rain [:rain1] (inactive)]

Attachment: handle opening in a new window, v1

Whoever gets to it first...

This seems to work fine, except for bug 525777, which seems to be IMAP only, so doesn't affect tests here. Tests pass for me (both the file and the entire suite).

Comment 25

[Rain [:rain1] (inactive)]

Attachment: [checked in] handle opening in a new window, v1.01

sorry, flipped if condition.

Comment 26

[David :Bienvenu]

this patch doesn't seem to apply on the 3.0 branch for me (and tabs don't work on the trunk so well, if I understand correctly...)

patching file mail/test/mozmill/shared-modules/test-folder-display-helpers.js
Hunk #1 FAILED at 76
Hunk #2 FAILED at 154
2 out of 3 hunks FAILED -- saving rejects to file mail/test/mozmill/shared-modul
es/test-folder-display-helpers.js.rej

Comment 27

[David :Bienvenu]

sorry, I was applying on top of gloda-delete patch.

Comment 28

[David :Bienvenu]

Comment on attachment 409927 [details] [diff] [review]
[checked in] handle opening in a new window, v1.01

this looks OK, but a bit scary, in terms of what could potentially regress vs. the use case that is fixed. I guess if we get this in before the gloda patch, it will get a fair amount of banging on :-)

Comment 29

[Rain [:rain1] (inactive)]

Comment on attachment 409927 [details] [diff] [review]
[checked in] handle opening in a new window, v1.01

I'll try to land this tomorrow.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 409927 [details] [diff] [review]
[checked in] handle opening in a new window, v1.01

Checked in:
http://hg.mozilla.org/comm-central/rev/039342c0fdc2
http://hg.mozilla.org/releases/comm-1.9.1/rev/494abbaa4b0a