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

RESOLVED FIXED in Thunderbird 3.0rc1

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: rkent, Assigned: sid0)

Tracking

({regression})

Trunk
Thunderbird 3.0rc1
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [no l10n impact])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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"
Flags: blocking-thunderbird3?
Yeah this seems broken, so we should fix it in some manner.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
Whiteboard: [no l10n impact]
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
Keywords: regression

Updated

10 years ago
Duplicate of this bug: 512081
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.
Assignee: nobody → david.ascher
Status: NEW → ASSIGNED
Attachment #401330 - Flags: ui-review?(clarkbw)
Attachment #401330 - Flags: review?(bugzilla)
Whiteboard: [no l10n impact] → [has l10n impact]
Comment on attachment 401330 [details] [diff] [review]
[checked in] patch v1

like it
Attachment #401330 - Flags: ui-review?(clarkbw) → ui-review+
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [has l10n impact] → [has l10n impact][needs review 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.
Attachment #401330 - Flags: review?(bugzilla) → review-
Whiteboard: [has l10n impact][needs review standard8] → [has l10n impact][needs new patch]
(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 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.
Attachment #401330 - Flags: review- → review+
Whiteboard: [has l10n impact][needs new patch] → [has l10n impact][needs second patch]
Attachment #401330 - Attachment description: patch v1 → [checked in] patch v1
The second patch should have no l10n impact, so changing whiteboard appropriately.
Whiteboard: [has l10n impact][needs second patch] → [no l10n impact][needs second patch]
Attachment #401330 - Attachment is obsolete: true
Assignee: david.ascher → sid.bugzilla
Summary: button "Open Message Folder" in search doesn't open the folder → "Open in Folder" in search doesn't respect tab/window pref

Comment 11

10 years ago
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?
Posted patch WIP (obsolete) — Splinter Review
There's a lot of ugliness in this patch, and still a lot more to do.
Per discussion with bienvenu, let's get this in first, and handle the case where no 3pane window is open in a separate patch.
Attachment #403137 - Attachment is obsolete: true
Attachment #403301 - Flags: ui-review?(clarkbw)
Attachment #403301 - Flags: review?(bienvenu)
(Assignee)

Updated

10 years ago
Whiteboard: [no l10n impact][needs second patch] → [no l10n impact][needs review bienvenu][needs review clarkbw]

Comment 14

10 years ago
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 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.
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?
Attachment #403301 - Attachment is obsolete: true
Attachment #403365 - Flags: ui-review?(clarkbw)
Attachment #403365 - Flags: review?(bienvenu)
Attachment #403301 - Flags: ui-review?(clarkbw)
Attachment #403301 - Flags: review?(bienvenu)

Comment 17

10 years ago
(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

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

thx, Sid.
Attachment #403365 - Flags: review?(bienvenu) → review+
Comment on attachment 403365 [details] [diff] [review]
[checked in] handle all cases where a 3pane window is open, v2

looks good to me
Attachment #403365 - Flags: ui-review?(clarkbw) → ui-review+
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
Attachment #403365 - Attachment description: handle all cases where a 3pane window is open, v2 → [checked in] handle all cases where a 3pane window is open, v2
(Assignee)

Updated

10 years ago
Whiteboard: [no l10n impact][needs review bienvenu][needs review clarkbw] → [no l10n impact][needs third patch]
(Assignee)

Updated

10 years ago
Attachment #401330 - Attachment is obsolete: false
(Assignee)

Updated

10 years ago
Blocks: 507593
Whiteboard: [no l10n impact][needs third patch] → [no l10n impact][needs third patch][at risk]

Comment 21

10 years ago
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?
(Assignee)

Updated

10 years ago
Whiteboard: [no l10n impact][needs third patch][at risk] → [no l10n impact][needs third patch][working on a patch]
There seems to be one issue -- the per-tab throbber keeps spinning till infinity -- but I see it without the patch too.
(Assignee)

Updated

10 years ago
Depends on: 525777
(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.
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).
Attachment #409626 - Attachment is obsolete: true
Attachment #409906 - Flags: review?(bugmail)
Attachment #409906 - Flags: review?(bienvenu)
Whiteboard: [no l10n impact][needs third patch][working on a patch] → [no l10n impact][has patch + unit test][needs review bienvenu or asuth]
sorry, flipped if condition.
Attachment #409906 - Attachment is obsolete: true
Attachment #409927 - Flags: review?(bugmail)
Attachment #409927 - Flags: review?(bienvenu)
Attachment #409906 - Flags: review?(bugmail)
Attachment #409906 - Flags: review?(bienvenu)

Comment 26

10 years ago
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

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

Updated

10 years ago
Attachment #409927 - Flags: review?(bienvenu) → review+

Comment 28

10 years ago
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 :-)
(Assignee)

Updated

10 years ago
Whiteboard: [no l10n impact][has patch + unit test][needs review bienvenu or asuth] → [no l10n impact][has patch + unit test][needs landing]
(Assignee)

Updated

10 years ago
Attachment #409927 - Flags: review?(bugmail)
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 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
Attachment #409927 - Attachment description: handle opening in a new window, v1.01 → [checked in] handle opening in a new window, v1.01
Flags: in-testsuite+
Whiteboard: [no l10n impact][has patch + unit test][needs landing] → [no l10n impact]
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.