Closed
Bug 515218
Opened 15 years ago
Closed 15 years ago
"Open in Folder" in search doesn't respect tab/window pref
Categories
(Thunderbird :: Folder and Message Lists, defect)
Thunderbird
Folder and Message Lists
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0rc1
People
(Reporter: rkent, Assigned: rain1)
References
Details
(Keywords: regression, Whiteboard: [no l10n impact])
Attachments
(3 files, 4 obsolete files)
3.96 KB,
patch
|
standard8
:
review+
clarkbw
:
ui-review+
|
Details | Diff | Splinter Review |
53.54 KB,
patch
|
Bienvenu
:
review+
clarkbw
:
ui-review+
|
Details | Diff | Splinter Review |
21.66 KB,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•15 years ago
|
||
Yeah this seems broken, so we should fix it in some manner.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
Updated•15 years ago
|
Whiteboard: [no l10n impact]
Comment 2•15 years ago
|
||
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
Comment 4•15 years ago
|
||
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)
Updated•15 years ago
|
Whiteboard: [no l10n impact] → [has l10n impact]
Comment 5•15 years ago
|
||
Comment on attachment 401330 [details] [diff] [review] [checked in] patch v1 like it
Attachment #401330 -
Flags: ui-review?(clarkbw) → ui-review+
Updated•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Updated•15 years ago
|
Whiteboard: [has l10n impact] → [has l10n impact][needs review standard8]
Comment 6•15 years ago
|
||
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-
Updated•15 years ago
|
Whiteboard: [has l10n impact][needs review standard8] → [has l10n impact][needs new patch]
Assignee | ||
Comment 7•15 years ago
|
||
(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•15 years ago
|
||
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+
Updated•15 years ago
|
Whiteboard: [has l10n impact][needs new patch] → [has l10n impact][needs second patch]
Updated•15 years ago
|
Attachment #401330 -
Attachment description: patch v1 → [checked in] patch v1
Comment 9•15 years ago
|
||
Comment on attachment 401330 [details] [diff] [review] [checked in] patch v1 Checked in: http://hg.mozilla.org/comm-central/rev/930310b0e8c5
Comment 10•15 years ago
|
||
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]
Updated•15 years ago
|
Attachment #401330 -
Attachment is obsolete: true
Updated•15 years ago
|
Assignee: david.ascher → sid.bugzilla
Updated•15 years ago
|
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•15 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?
Assignee | ||
Comment 12•15 years ago
|
||
There's a lot of ugliness in this patch, and still a lot more to do.
Assignee | ||
Comment 13•15 years ago
|
||
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•15 years ago
|
Whiteboard: [no l10n impact][needs second patch] → [no l10n impact][needs review bienvenu][needs review clarkbw]
Comment 14•15 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 15•15 years ago
|
||
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.
Assignee | ||
Comment 16•15 years ago
|
||
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•15 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•15 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 19•15 years ago
|
||
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+
Assignee | ||
Comment 20•15 years ago
|
||
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•15 years ago
|
Whiteboard: [no l10n impact][needs review bienvenu][needs review clarkbw] → [no l10n impact][needs third patch]
Assignee | ||
Updated•15 years ago
|
Attachment #401330 -
Attachment is obsolete: false
Updated•15 years ago
|
Whiteboard: [no l10n impact][needs third patch] → [no l10n impact][needs third patch][at risk]
Comment 21•15 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•15 years ago
|
Whiteboard: [no l10n impact][needs third patch][at risk] → [no l10n impact][needs third patch][working on a patch]
Assignee | ||
Comment 22•15 years ago
|
||
There seems to be one issue -- the per-tab throbber keeps spinning till infinity -- but I see it without the patch too.
Assignee | ||
Comment 23•15 years ago
|
||
(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.
Assignee | ||
Comment 24•15 years ago
|
||
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)
Updated•15 years ago
|
Whiteboard: [no l10n impact][needs third patch][working on a patch] → [no l10n impact][has patch + unit test][needs review bienvenu or asuth]
Assignee | ||
Comment 25•15 years ago
|
||
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•15 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•15 years ago
|
||
sorry, I was applying on top of gloda-delete patch.
Updated•15 years ago
|
Attachment #409927 -
Flags: review?(bienvenu) → review+
Comment 28•15 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•15 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•15 years ago
|
Attachment #409927 -
Flags: review?(bugmail)
Assignee | ||
Comment 29•15 years ago
|
||
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•15 years ago
|
||
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
Updated•15 years ago
|
Flags: in-testsuite+
Whiteboard: [no l10n impact][has patch + unit test][needs landing] → [no l10n impact]
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•