Last Comment Bug 467159 - Clicking on Growl alert should select folder with new mail
: Clicking on Growl alert should select folder with new mail
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Thunderbird 3.0b3
Assigned To: David Humphrey (:humph)
:
Mentors:
Depends on: 482296
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-29 06:45 PST by David Humphrey (:humph)
Modified: 2009-12-18 14:16 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add folder/message URIs for notification clicks + random whitespace fixes (12.42 KB, patch)
2009-02-20 13:20 PST, David Humphrey (:humph)
standard8: superreview-
Details | Diff | Splinter Review
Updated patch, uses new isMessageUri attribute (12.38 KB, patch)
2009-03-09 19:25 PDT, David Humphrey (:humph)
no flags Details | Diff | Splinter Review
Correct version this time (sorry) (12.38 KB, patch)
2009-03-09 19:29 PDT, David Humphrey (:humph)
no flags Details | Diff | Splinter Review
Correct version this time (sorryx2) (12.31 KB, patch)
2009-03-09 19:31 PDT, David Humphrey (:humph)
mozilla: review+
standard8: superreview+
Details | Diff | Splinter Review
Patch v3 with Standard8's comments addressed. (12.30 KB, patch)
2009-03-26 09:50 PDT, David Humphrey (:humph)
no flags Details | Diff | Splinter Review

Description David Humphrey (:humph) 2008-11-29 06:45:16 PST
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).
Comment 1 David Humphrey (:humph) 2009-02-20 13:20:09 PST
Created attachment 363380 [details] [diff] [review]
Add folder/message URIs for notification clicks + random whitespace fixes

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.
Comment 2 Mark Banner (:standard8) 2009-03-02 06:18:44 PST
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 David :Bienvenu 2009-03-02 09:36:26 PST
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 Mark Banner (:standard8) 2009-03-08 14:49:36 PDT
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.
Comment 5 David Humphrey (:humph) 2009-03-09 12:18:26 PDT
> 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.
Comment 6 David :Bienvenu 2009-03-09 13:40:21 PDT
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.
Comment 7 David Humphrey (:humph) 2009-03-09 19:25:32 PDT
Created attachment 366473 [details] [diff] [review]
Updated patch, uses new isMessageUri attribute

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.
Comment 8 David Humphrey (:humph) 2009-03-09 19:29:02 PDT
Created attachment 366474 [details] [diff] [review]
Correct version this time (sorry)

I've uploaded the old one again by mistake.  Please see previous comment and this new patch.  Sorry about that.
Comment 9 David Humphrey (:humph) 2009-03-09 19:31:33 PDT
Created attachment 366475 [details] [diff] [review]
Correct version this time (sorryx2)

Clearly I need to log off now, I have made the same mistake again.  Apologies.
Comment 10 Mark Banner (:standard8) 2009-03-19 08:53:14 PDT
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.
Comment 11 David :Bienvenu 2009-03-24 11:56:47 PDT
Comment on attachment 366475 [details] [diff] [review]
Correct version this time (sorryx2)

r=me, with Standard8's comments fixed.
Comment 12 David Humphrey (:humph) 2009-03-26 09:50:14 PDT
Created attachment 369524 [details] [diff] [review]
Patch v3 with Standard8's comments addressed.
Comment 13 Mark Banner (:standard8) 2009-03-26 10:05:37 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/cac87eeddd5a
Comment 14 Eugene 2009-12-18 14:10:10 PST
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 Eugene 2009-12-18 14:16:41 PST
(related to bug #519792)?

Note You need to log in before you can comment on or make changes to this bug.