938759 - When activating "Remind Me Later" from menus, current attachment reminder notification bar should be hidden

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #521158 +++

A leftover nit from bug 521158 which I mentioned there but apparently it got lost somewhere along the journey:

STR

1 compose
2 type magic attachment keyword like "attachment" into msg body
3 don't touch the notification bar, just proceed with step 4
4 from Attach Button, check "Remind Me Later" (same from main menu and attachment pane whitespace context)

Actual result

- Manual Attachment Reminder is active (checkmark on "Remind me later")
- but notification bar is still visible (although you've just opted for "remind me later", implying that you won't need the bar right now)

Expected result

- the notification bar should disappear as soon as user activates "Remind me later" from the menus (as it also disappears when user clicks "Remind me later" button on the notification -> ux-consistency)
- when user activates "Remind me later", he implies that he does NOT want to care or be reminded about attachments right now, so henceforth the notification bar is just clutter
- especially the "Remind me later" button on bar is confusing because "Remind me later" is already active (ux-mode-error)

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

Possible fix.
Though this sort of favors duplication, I'll remove the duplication in a followup bug.

Thanks.

Comment 2

[:aceman]

Comment on attachment 832756 [details] [diff] [review]
Patch

Review of attachment 832756 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't run with this yet, but the code looks OK (copied from already existing code).

The cleanup bug Suyash mentions is bug 938829.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3168,5 @@
>            .setAttribute("checked", aState);
>    gMsgCompose.compFields.attachmentReminder = aState;
> +  let nBox = document.getElementById("attachmentNotificationBox");
> +  let notification = nBox.getNotificationWithValue("attachmentReminder");
> +  if (aState && notification != null)

(aState && notification)

Comment 3

[Thomas D. (:thomas8)]

Comment on attachment 832756 [details] [diff] [review]
Patch

Review of attachment 832756 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3169,5 @@
>    gMsgCompose.compFields.attachmentReminder = aState;
> +  let nBox = document.getElementById("attachmentNotificationBox");
> +  let notification = nBox.getNotificationWithValue("attachmentReminder");
> +  if (aState && notification != null)
> +    nBox.removeNotification(notification);

That looks like it could do the trick as a bandaid fix (can't test).

However, as hinted by Suyash, it also looks like unnecessary duplication of code.
I'm actually failing to understand why the bar doesn't automatically go away as soon as we set gManualAttachmentReminder=true;

Looking at the code of the remindButton on notification, all it did/does is to set gManualAttachmentReminder=true; (formerly gRemindLater, but that shouldn't matter as we just renamed it):

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1849

1852       callback: function (aNotificationBar, aButton)
1853       {
1854         toggleAttachmentReminder(true);
1855       }

So before bug 521158, current line 1854 was:
gRemindLater = true;
and we still do the same thing in the toggle function by setting:
gManualAttachmentReminder=true;

Iow, the change of that variable was enough to make the bar go away (and still is, when clicking reminderButton on notification).
So how come that if we change the variable from somewhere else, the continuous keywords search  listener functions do not pick that up and hide the bar?

So far, I thought that changing the variable will make the bar go away, perhaps somewhere around here:

http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1939

1939   if (!CheckForAttachmentNotification.shouldFire || gManualAttachmentReminder)
1940     return;
1941   if (!event)

Having said which, I do not fully understand how the notification bar functions work and interact, they look more complex than necessary and poorly documented, so indeed to clean this up starting from bug 938829 looks like a great starting point to me.

Comment 4

[:aceman]

(In reply to Thomas D. from comment #3)
> So far, I thought that changing the variable will make the bar go away,
> perhaps somewhere around here:
> 
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#1939
> 
> 1939   if (!CheckForAttachmentNotification.shouldFire ||
> gManualAttachmentReminder)
> 1940     return;
> 1941   if (!event)
> 
> Having said which, I do not fully understand how the notification bar
> functions work and interact, they look more complex than necessary and
> poorly documented, so indeed to clean this up starting from bug 938829 looks
> like a great starting point to me.

See, if gManualAttachmentReminder is true, the function returns, it does not actively hide the notification bar. Changing the variable is not enough, you need code actively checking for the state and act accordingly (show/hide). So this could be made more robust without duplicating the hiding code. Maybe here, maybe in the other bug.

Comment 5

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #3)
> Comment on attachment 832756 [details] [diff] [review]
> Patch
> 
> Review of attachment 832756 [details] [diff] [review]:
> That looks like it could do the trick as a bandaid fix (can't test).

Please accept the band-aid here and I'll operate it in bug 938829 :)

Comment 6

[Thomas D. (:thomas8)]

You the band-aid if you want.

But when we click on "Remind me later" button on notification, it looks as if the callback funcion only sets gManualAttachmentReminder=true, and the hiding of bar then happens automatically. Or not?

If somebody could explain to me why (or how) it works for the button, but not for the menu, that would be enlightening. Iow, what is the flow of action after the "Remind me later" button on notification is clicked?

Comment 7

[Suyash Agarwal (:sshagarwal)]

I think it is the property of the notification, that the notification is hidden whenever a button on it is clicked.
I tried this with a button with an empty callback, still the notification disappeared after the button was clicked.

But in our case, when we are setting the reminder from the menu, CheckForAttachmentNotification() will make sure that even if attachment specific keywords are added in the message body, the bar will not appear (if it doesn't exist already), but if the notification already exists, we have to manually hide it as attachmentWorker.onmessage() will work only on some new activity in the body, so to instantaneously hide the notification as soon as we check the "reminder" option from the menu, we have to actively hide it.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Also, our callbacks don't return anything so,
result in http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml#439
is false, resulting in closing of the notification.
Thanks to aleth for telling me this.

Comment 9

[Suyash Agarwal (:sshagarwal)]

Sorry, result is undefined so !result casts to true, resulting in the closing of the notification.

Comment 10

[:aceman]

I'd like to point out bug 938783 where the notification bar is not closed properly so any tests on trunk are currently affected by this problem and may give you misleading results. I assume Suyash is developing with the patch from bug 938783 applied so he can properly test the behavior he is changing.

Comment 11

[Suyash Agarwal (:sshagarwal)]

Ya, so this patch is to be applied above the patch for bug 938783 while reviewing.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 832756 [details] [diff] [review]
Patch

Review of attachment 832756 [details] [diff] [review]:
-----------------------------------------------------------------

r=mkmelin

Comment 13

[Thomas D. (:thomas8)]

Comment on attachment 832756 [details] [diff] [review]
Patch

Per comment 0, by design, this is a necessary and obvious part of bug 521158 (i.e. nitfixing an omission by that bug) which already has bwinton's ui-r+ (and bwinton is cc'ed here).

-> forwarding bwinton's ui-r+ from Bug 521158 Comment 168.

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Carrying over review from mkmelin.

Comment 15

[:aceman]

Comment on attachment 833333 [details] [diff] [review]
Patch for check-in

Review of attachment 833333 [details] [diff] [review]:
-----------------------------------------------------------------

I see this is a small change, but it is missing ui-r yet.

Comment 16

[Josiah Bruner [:jsbruner]]

Comment on attachment 833333 [details] [diff] [review]
Patch for check-in

Review of attachment 833333 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, looks good! ui-r=me. 
Clearing Blake's ui-review flag because I don't think this bug needs two ui-reviews.

Comment 17

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #15)
> I see this is a small change, but it is missing ui-r yet.

Bug 521158, comment 91, number 4

((1+1)==2)==(true) q.e.d. [snip]

Comment 18

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in with ui-r

Carrying over review from mkmelin and ui-r from JosiahOne.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f33c1d84a55b

Comment 20

[Thomas D. (:thomas8)]

Awesome! It's raining patches, so nice to see them all fall into place...
Thanks!