Clicking on Growl alert should select folder with new mail

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Backend
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: humph, Assigned: humph)

Tracking

Trunk
Thunderbird 3.0b3
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

9 years ago
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.
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 3

9 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 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

9 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

9 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

9 years ago
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.
Attachment #366473 - Flags: superreview?(bugzilla)
Attachment #366473 - Flags: review?(bienvenu)
(Assignee)

Comment 8

9 years ago
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.
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

9 years ago
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.
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 11

8 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

8 years ago
Created attachment 369524 [details] [diff] [review]
Patch v3 with Standard8's comments addressed.
Attachment #366475 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/cac87eeddd5a
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3

Comment 14

8 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

8 years ago
(related to bug #519792)?
You need to log in before you can comment on or make changes to this bug.