Closed
Bug 467159
Opened 16 years ago
Closed 15 years ago
Clicking on Growl alert should select folder with new mail
Categories
(MailNews Core :: Backend, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b3
People
(Reporter: humph, Assigned: humph)
References
Details
Attachments
(1 file, 4 obsolete files)
12.30 KB,
patch
|
Details | Diff | Splinter Review |
I just noticed that we don't properly use the folder URI for new mail in the alert code. ShowAlertNotification takes aAlertCookie, which we correct pass into nsMessengerOSXIntegration::ShowAlertNotification. However, we don't populate the string in nsMessengerOSXIntegration::FillToolTip (we currently set it to EmptyCString()), nor do we use it in the click handler when we call openMailWindow (using GetFirstFolderWithNewMail is going to be wrong a lot, because it always looks in the first folder in mFoldersWithNewMail, ignoring the rest).
Assignee | ||
Comment 1•15 years ago
|
||
Based on conversation with davida on irc, this fixes the fact that we don't send/use the folder's uri as part of the growl notification. This patch fixes that, and adds logic to use a message uri if there is only 1 message, or the folder uri for more than 1. Having been told not to use FindInReadable in a previous patch in here, I used Find in a hacky way, so let me know if I can improve this some other way.
Attachment #363380 -
Flags: superreview?(bugzilla)
Attachment #363380 -
Flags: review?(bienvenu)
Comment 2•15 years ago
|
||
Comment on attachment 363380 [details] [diff] [review] Add folder/message URIs for notification clicks + random whitespace fixes >+ nsCOMPtr<nsIDOMWindow> newWindow; >+ wwatch->OpenWindow(0, "chrome://messenger/content/messageWindow.xul", >+ "_blank", "all,chrome,dialog=no,status,toolbar", uri, >+ getter_AddRefs(newWindow)); Hmm, I'm not sure I like this. For those people who do like to have messages open in the standalone window, its probably for mine, but I use the message preview window > 99% of the time, so if a message comes in, I'd just like it to show me which folder it is in and select if possible. We should probably also think about the open in new tab case as well (when we get the pref for it). The rest of it looks reasonable though.
Comment 3•15 years ago
|
||
Comment on attachment 363380 [details] [diff] [review] Add folder/message URIs for notification clicks + random whitespace fixes why can't you just make openMailWindow take an nsCString &, since it's not an interface method. Then you could just use aUri.Find(NS_LITERAL_CSTRING...), I think. I also think you'd want to worry about the case where the folder has "-message" in the name, and hence the uri. So "-message" should be before the initial ':', e.g., the uri is of the form imap-message:// or mailbox-messge:// Re opening in the stand-alone window, I think that's the safest thing to do until we have a pref to open the message in a tab...and we have the same issue for opening spotlight results, so whatever solution we have for that, we should use here too.
Comment 4•15 years ago
|
||
Comment on attachment 363380 [details] [diff] [review] Add folder/message URIs for notification clicks + random whitespace fixes I think I agree with David's comments, so this needs a revision.
Attachment #363380 -
Flags: superreview?(bugzilla) → superreview-
Assignee | ||
Comment 5•15 years ago
|
||
> I also think you'd want to worry about the case where the folder has "-message" > in the name, and hence the uri. So "-message" should be before the initial ':', > e.g., the uri is of the form imap-message:// or mailbox-messge:// Filed bug 482296 to deal with this properly.
Depends on: 482296
Comment 6•15 years ago
|
||
Comment on attachment 363380 [details] [diff] [review] Add folder/message URIs for notification clicks + random whitespace fixes obsoleting - humph is going to do a new patch addressing comments.
Attachment #363380 -
Attachment is obsolete: true
Attachment #363380 -
Flags: review?(bienvenu)
Assignee | ||
Comment 7•15 years ago
|
||
This fixes the -message check using the new api change from bug 482296 (not currently available, but I did them together so I'm posting this along with the fix there). I think I've understood your comments to mean that I *should* open a stand-alone window to display the message? If I've misunderstood, let me know.
Attachment #366473 -
Flags: superreview?(bugzilla)
Attachment #366473 -
Flags: review?(bienvenu)
Assignee | ||
Comment 8•15 years ago
|
||
I've uploaded the old one again by mistake. Please see previous comment and this new patch. Sorry about that.
Attachment #366473 -
Attachment is obsolete: true
Attachment #366474 -
Flags: superreview?(bugzilla)
Attachment #366474 -
Flags: review?(bienvenu)
Attachment #366473 -
Flags: superreview?(bugzilla)
Attachment #366473 -
Flags: review?(bienvenu)
Assignee | ||
Comment 9•15 years ago
|
||
Clearly I need to log off now, I have made the same mistake again. Apologies.
Attachment #366474 -
Attachment is obsolete: true
Attachment #366475 -
Flags: superreview?(bugzilla)
Attachment #366475 -
Flags: review?(bienvenu)
Attachment #366474 -
Flags: superreview?(bugzilla)
Attachment #366474 -
Flags: review?(bienvenu)
Comment 10•15 years ago
|
||
Comment on attachment 366475 [details] [diff] [review] Correct version this time (sorryx2) >+ if (NS_FAILED(rv)) >+ return; nit: blank lines after the return statements please (at least 3 places). -static void openMailWindow(const nsACString& aFolderUri) +static void openMailWindow(const nsACString& aUri) ... > if (messengerWindowService) > messengerWindowService->OpenMessengerWindowWithUri( >- "mail:3pane", nsCString(aFolderUri).get(), nsMsgKey_None); >+ "mail:3pane", nsCString(aUri).get(), nsMsgKey_None); Given openMailWindow is an internal function, you might as well make it take const nsCString& and then you don't need to case back to nsCString for the .get(). sr=me with those fixed.
Attachment #366475 -
Flags: superreview?(bugzilla) → superreview+
Comment 11•15 years ago
|
||
Comment on attachment 366475 [details] [diff] [review] Correct version this time (sorryx2) r=me, with Standard8's comments fixed.
Attachment #366475 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #366475 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 13•15 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/cac87eeddd5a
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
Comment 14•15 years ago
|
||
This bug has returned. Clicking on Growl window does not select the correct folder, but instead brings up a message window with the new message. Additionally, the delete button is disabled (separate bug).
Comment 15•15 years ago
|
||
(related to bug #519792)?
You need to log in
before you can comment on or make changes to this bug.
Description
•