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)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: humph, Assigned: humph)

References

Details

Attachments

(1 file, 4 obsolete files)

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).
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 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 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 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-
> 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 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)
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)
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)
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 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 on attachment 366475 [details] [diff] [review]
Correct version this time (sorryx2)

r=me, with Standard8's comments fixed.
Attachment #366475 - Flags: review?(bienvenu) → review+
Attachment #366475 - Attachment is obsolete: true
Keywords: checkin-needed
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
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).
(related to bug #519792)?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: