The new feature where typing certain attachment keywords brings up an info bar where you can choose "Remind me later" to add attachment before sending is really awsome. In addition, it would be great if we could use that very feature without doing keyword magic. Actual - user can only activate "remind me later" by typing attachment keywords expected - in addition, implement a menu from which "remind me later" can be activated manually without doing any keyword magic. - menu option should be included in all "Attachment" menus - should be toggle button for true or false (show tick when activated) e.g. for attachment button, dropdown menu should look like this Attach > Files... Web page... Personal card --------- [ ] Remind me later This feature will be highly beneficial for all those users who would prefer to add their attachment later, upon sending (cf. discussion in bug 378046). This workaround is almost as good as the feature, only that you can't specify a file name in advance.

This RFE also comes with the extra benefit of enabling users to check the actual status of attachment reminder after they have activated or dismissed it. This addresses a shortcoming of the current UI that after dismissing or activating Attachment Reminder, user has no way of knowing (say 2 hours later) if the current status of Attachment Reminder is "active" or not.

Bwinton, another one-minute ui-review candidate :) ui-review?bwinton Many benefits, zero cost, about 6 lines of code: This bug proposes to make "Remind me later" (aka Attachment Reminder) available/accessible as a checkmark menu option from the two attachment fly-out menus (File > Attach > ..., and the [Attach|v] button on Composition Toolbar. Attach > Files... Web page... Personal card --------- [ ] Remind me later Key benefits: * Make "Attachment Reminder" available *any time* on menu without the need for typing exactly any of the magic keywords, even before you start writing * Give users easy access to "Attachment Reminder" even if their message text does not happen to contain magic words, e.g. for msg text like "Hi John, here's the numbers you requested" - users can't really add any of these words to the attachment keyword list (because they are too common), and yet it's a perfectly sufficient phrase to send an attachment - this is where activating "Attachment Reminder" with two clicks from the attach button comes in handy. * Good workaround for users who prefer adding their attachments just before sending (bug 722929) - almost as good as the feature! (but we might want to test if attachmend reminder status actually survives saving as draft, closing and reopening etc.) * Provide a way of visually checking the current status of Attachment reminder, especially for long or interrupted compositions. Cost: near zero (add 1 menuitem to the 3 menuitems on existing attachment dropdown menu, so it's very unobtrusive and exactly in the right place)

Clearing Blake's needinfo and replacing with ui-review based on this mockup. Blake, this image explains/shows the use of this feature, for more information, see Thomas's explanation above.

Though the reviewers will probably not be happy with the patch as there is duplication, but I couldn't find ways to pass variables by reference in JS. Also, I would like suggestions on the text visible on the bar.

(In reply to Suyash Agarwal (:sshagarwal) from comment #4) > Created attachment 766258 [details] [diff] [review] > Patch > > Though the reviewers will probably not be happy with the patch as there is > duplication, but I couldn't find ways to pass variables by reference in JS. Perhaps this can help? (random google) http://snook.ca/archives/javascript/javascript_pass > Also, I would like suggestions on the text visible on the bar. Looks good to me as in the patch, "Remind Me Later"

If we want to try and land this for TB24, perhaps we should ask for simultaneous review? too...

Comment on attachment 766258 [details] [diff] [review] Patch Review of attachment 766258 [details] [diff] [review]: ----------------------------------------------------------------- Good work there for implementing the general functionality :) Bugs in the patch that need to be addressed: - clicking the yellow bar opens the preferences dialog with editing the keywords which trigger the attachment bar. You probably copied this from the original bar but I think it is not needed here. After the preferences dialog is closed, the bar disappears but the menuitem "remind me later" still has the check. - When the menuitem "remind me later" still has the check but the bar is not shown, and I click it to uncheck it, the reminder bar actually appears. - When the bar is invoked via the Attach menu in the toolbar, the menu in the File -> Attach does not get the tick (check). And vice versa. - Very often just clicking into the message body makes the reminder bar disappear. - Even if there already is an attachment, using the "remind me later" item shows the bar. Some thought need to be put into this if that is actually wanted. If yes, how do you determine if a NEW attachment was added after that? - When the reminder bar is shown and there is no attachment, sending the message is successful without any warning. So you may request ui-review for the general idea, but unfortunately the patch is not ready yet. And yes, I do not like the duplicated code taken from the existing attachment bar. ::: mail/components/compose/content/MsgComposeCommands.js @@ +2442,5 @@ > + msg.appendChild(msgText); > + msgText.id = "attachmentReminder"; > + msgText.setAttribute("crop", "end"); > + msgText.setAttribute("flex", "1"); > + let textValue = "Reminder for Attachments"; This needs to be made localizable (fetch is from some .properties file).

(In reply to :aceman from comment #7) > Comment on attachment 766258 [details] [diff] [review] > Patch > > Review of attachment 766258 [details] [diff] [review]: > ----------------------------------------------------------------- > > Good work there for implementing the general functionality :) > > Bugs in the patch that need to be addressed: > - clicking the yellow bar opens the preferences dialog with editing the > keywords which trigger the attachment bar. You probably copied this from the > original bar but I think it is not needed here. Pls do not remove clickability of the bar for changing keywords. It's useful and comfortable. I alwas prefer doing useful things from big click targets over doing nothing. We've got similar behaviour for attachment bar. Actually, I'm not sure why the bar should be involved in this bug. I was imagining something much more simple and straightforward: "Remind me later" reflects the status of the reminder (probably held in gRemindLater variable) - is it activated or not? Checking or unchecking "Remind me later" just toggles the active status of the reminder. No need for showing the bar at all. We might want to tweak the behaviour when 1 or more attachments have been added. If attachments added after activation cause the reminder to keep silent, than that should reflect in reminder checkbox status as unchecked. If I re-activate the checkbox, it should remind me even if there are attachments. The fact that I have one or more attachments does not imply I have them all, reminder should be (activate-able) independent of that. > After the preferences dialog > is closed, the bar disappears it should not, and does not on tb17 > but the menuitem "remind me later" still has > the check. probably shouldn't > - When the menuitem "remind me later" still has the check but the bar is not > shown, and I click it to uncheck it, the reminder bar actually appears. and should not > - When the bar is invoked via the Attach menu in the toolbar, the menu in > the File -> Attach does not get the tick (check). And vice versa. yep, pls keep menus in sync. I've recently done menu syncying in Bug 507103, which might serve as a template how to do it (if possible, connect menus with a respective command which will then update more or less automatically the menus) > - Very often just clicking into the message body makes the reminder bar > disappear. but should not > - Even if there already is an attachment, using the "remind me later" item > shows the bar. Actually, clicking "Remind me later" checkbox from menu should never show the bar. As said above, I think it should be much simpler. > Some thought need to be put into this if that is actually > wanted. If yes, how do you determine if a NEW attachment was added after > that? > - When the reminder bar is shown and there is no attachment, sending the > message is successful without any warning. bad, but as I said, we don't have to touch all that I think > So you may request ui-review for the general idea, but unfortunately the > patch is not ready yet. > > And yes, I do not like the duplicated code taken from the existing > attachment bar. > > ::: mail/components/compose/content/MsgComposeCommands.js > @@ +2442,5 @@ > > + msg.appendChild(msgText); > > + msgText.id = "attachmentReminder"; > > + msgText.setAttribute("crop", "end"); > > + msgText.setAttribute("flex", "1"); > > + let textValue = "Reminder for Attachments"; > > This needs to be made localizable (fetch is from some .properties file).

Comment on attachment 766258 [details] [diff] [review] Patch Note, you do not actually need to request ui-review on a patch if something else has been given ui-review for you. In this case the mockup I added was taken straight from your patch and gives the reviewer an easier way to review. Unless you modify your patch to stray from the mockup, you can just keep the single ui-review. Removing duplicate ui-review flag.

Okay, so removing the bar, simply putting up the notification popup at the sending time. Moreover, if after the reminder is manually selected, user attaches few items, the pop up will not show up, if this isn't desired and the popup is required, please let me know, I'll make the change.

Comment on attachment 766335 [details] [diff] [review] Patch v2 Review of attachment 766335 [details] [diff] [review]: ----------------------------------------------------------------- Nice simpler version and actually works :) Let's see if this covers what Thomas wanted. ::: mail/components/compose/content/MsgComposeCommands.js @@ +2432,5 @@ > } > > +function invokereminder() > +{ > + let reminder = !gManualAttachmentReminder.showReminder; Try to call it showReminder so that it is obvious what its value means. @@ +2437,5 @@ > + gManualAttachmentReminder.showReminder = reminder; > + document.getElementById("remindLater").setAttribute("checked", reminder); > + document.getElementById("fremindLater").setAttribute("checked", reminder); > + Components.utils.reportError("var: "+reminder+"rl: "+document.getElementById("remindLater").hasAttribute("checked")+"frl:"+ > + document.getElementById("fremindLater").hasAttribute("checked")); To be removed in the final version. ::: mail/components/compose/content/messengercompose.xul @@ +106,5 @@ > <command id="cmd_attachFile" oncommand="goDoCommand('cmd_attachFile')"/> > <command id="cmd_attachCloud" oncommand="attachToCloud(event.target.cloudProvider); event.stopPropagation();"/> > <command id="cmd_attachPage" oncommand="goDoCommand('cmd_attachPage')"/> > <command id="cmd_attachVCard" checked="false" oncommand="ToggleAttachVCard(event.target)"/> > + <command id="cmd_remindLater" oncommand="invokereminder()"/> Would setting "checked" on the command instead of the menuitems work? @@ +439,5 @@ > <menuitem label="&attachPageCmd.label;" accesskey="&attachPageCmd.accesskey;" command="cmd_attachPage"/> > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator/> > + <menuitem id="fremindLater" type="checkbox" checked="false" autocheck="false" label="&remindLater.label;" command="cmd_remindLater"/> "fremindLater" is strange. Try remindLater_button and remindLater_menu. ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd @@ +27,5 @@ > <!ENTITY attachPageCmd.accesskey "W"> > <!--LOCALIZATION NOTE attachVCardCmd.label Don't translate the term 'vCard' --> > <!ENTITY attachVCardCmd.label "Personal Card (vCard)"> > <!ENTITY attachVCardCmd.accesskey "P"> > +<!ENTITY remindLater.label "Remind Me Later"> Please add an accesskey too and use it in the menuitems.

Made the changes suggested by aceman sir.

Comment on attachment 766415 [details] [diff] [review] Patch v2 (revised) Review of attachment 766415 [details] [diff] [review]: ----------------------------------------------------------------- Very nice, thanks! Now only to find out what Thomas wants to happen when the number of attachments changes after the reminder was set. ::: mail/components/compose/content/messengercompose.xul @@ +439,5 @@ > <menuitem label="&attachPageCmd.label;" accesskey="&attachPageCmd.accesskey;" command="cmd_attachPage"/> > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator/> > + <menuitem id="remindLater_menu" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" OK, so now that the attributes can be set on the command, you don't need IDs on the menuitems.

It's not easy to understand the whole behaviour of this, neither current nor after the patch. Two things from what I understand: 1) My original intention included showing the status of the automatical attachment reminder on the menu, and I miss the link between the bar (e.g. after clicking remind me later from the bar), and the menus. - if the bar shows because of keywords found, and I press "Remind me later" button from bar, then I'd expect the manual reminder menus to be checked. I think the bar only shows when there are no attachments yet, so the bar algorithm will have the same effect as our manual algorithm - show on 0 attachments, not show on 1+ attachments 2) I'm not entirely sure, but I wonder about this: - when I have 3 attachments, then set manual reminder from menu, it will trigger upon send if I still have only 3 (good). - when I add the 4th attachment, and still have 4 at the time of sending, it won't trigger - so practically the reminder is now OFF, but in the UI it is still checked! - then what if I WANT it to trigger (manually set) for the 5th attachment again? So I think we should try to add something to the "Add file attachment" routine, or a listener for the bucket's count of attachment items or such, that does this: - switches the manual reminder OFF when the lastnum+1 th attachment is added, and sets showreminder to false, and reminder-menu unchecked; but leave automatical reminder on for case of attachmentscount=0 - switches the manual reminder ON when attachitems.count goes down to 0, and "remind me" was triggered manually from bar's button OR menu, or when aggressive and the bar still shows. So at any time, user can check status in menu if he will ACTUALLY be reminded or not (i.e. whether he did or did not add another attachment (by counting) since invoking reminder), and he can manually re-enable the reminder to be reminded about another attachment again. I'm not very good at explaining this, but it should be simple. I'm not 100% sure about any of current behaviour, behaviour after current patch, and the behaviour I propose. I think I'll need more time for that.

Comment on attachment 766415 [details] [diff] [review] Patch v2 (revised) As explained above, this is going in the right direction, but I think we need to link the manual reminder with the existing keyword-based reminder feature that checks only if there are any attachments at all -> clearing feedback. (To have them as separate features while using the same labels in UI would be confusing). I really like the added usefulness of the manual reminder: a) keyword-based auto-reminder only triggers when there are no attachments (trigger for numAtt=0, don't trigger for numAtt>=1) b) manual reminder can be set to trigger independently of the existing number of attachments, so if there are 3 attachments and I set the reminder: trigger for numAtt>=3, don't trigger for numAtt>=4. Since a) is just a subset of b), it should be possible to get the interactions right and link them up. Which also means switching manual reminder OFF should cancel the gRemindLater flag of auto-reminder. To avoid confusions, we want a combined reminder status on the manual reminder menus (checked vs unchecked) that reflects both a) and b); to make b) work, that combined status needs to reflect if the reminder will actually trigger (as opposed to reflecting if any reminder is "sharp"). Iow, if there's no checkmark, I will not see a reminder when sending, and if there's a checkmark, I will definitely see a reminder when sending. So we need to track the actual number of attachments during composition and compare that against reminder conditions. For which I think it's OK to assume that when the count goes up by 1 attachment after whichever reminder was set, the conditions are met (attachment was added) so that reminder is OFF (shown by checked=false). Only the auto-reminder should probably (offer to?) reactivate itself when numAtt=0. That's the point about which I am not clear. I think we need to examine the exact workflow of the existing auto-reminder, especially its conditions for NOT alerting.

(In reply to :aceman from comment #13) > OK, so now that the attributes can be set on the command, you don't need IDs > on the menuitems. In general it's still useful to have ids for stuff - especially useful for extensions in case someone wants to overlay to add/change things.

(In reply to Magnus Melin from comment #16) > (In reply to :aceman from comment #13) > > OK, so now that the attributes can be set on the command, you don't need IDs > > on the menuitems. > > In general it's still useful to have ids for stuff - especially useful for > extensions in case someone wants to overlay to add/change things. While this is true, in this case we do not want any Ids. This is a very specific menu item and should not be customizable. Therefore it is not needed here. But for most things it is indeed useful.

How do you know someone won't want to do something with it? Hide it. Or add something below/above it? That's the whole point of extensions... you never know.

I think this has to wait for some time, as the attachment reminder bar that appears when keywords like "attach" appear in the message body, isn't appearing in the current build. Reason: The following error occures when launching the compose window, Timestamp: 06/26/2013 06:07:49 PM Error: TypeError: spellchecker is null Source File: resource://gre/modules/InlineSpellChecker.jsm Line: 157 The bug for this is bug 887010. Thanks.

(In reply to Magnus Melin from comment #18) > How do you know someone won't want to do something with it? Hide it. Or add > something below/above it? That's the whole point of extensions... you never > know. While that is a valid point, if we wanted to make the menu items extension-aware then a new bug would need to be filed (which perhaps we should do). Right now though, :aceman was suggesting that we follow the existing behavior, which I agree with. Having only one element with an ID doesn't really make sense. Thanks, Josiah

We tend to add ids as we go, otherwise there's a lot of unnecessary code churn. Though for tb3 i recall there was a bigger effort adding ids.

(In reply to Magnus Melin from comment #21) > We tend to add ids as we go, otherwise there's a lot of unnecessary code > churn. Though for tb3 i recall there was a bigger effort adding ids. I agree with that, but in this case it's too late. I filed bug 887242 to add more IDs and will have a patch up later for that. For any more discussion, please comment in that bug. Thanks.

(In reply to Suyash Agarwal (:sshagarwal) from comment #19) > I think this has to wait for some time, as the attachment reminder bar that > appears when keywords like "attach" appear in the message body, isn't > appearing in the current build. > Reason: The following error occures when launching the compose window, Confirming. The bar does only appear sometimes but not consistently and does not prevent a message to be sent if it contains the "attachment" keyword.

Comment on attachment 766154 [details] Mockup - For UI review It seems like the patch is taking a different (and more consistent) direction, so I'm clearing out the ui-review request…

Possible Fix, addressing the previous comments -> The check on the menu item works in sync with user actions. -> If at any point the no. of attachments goes down the no. that was at the time of setting the reminder, the reminder is re-enabled. -> If the manual Reminder check box is unchecked, gRemindLater is also disabled.

Comment on attachment 798206 [details] [diff] [review] Patch v3 Review of attachment 798206 [details] [diff] [review]: ----------------------------------------------------------------- - providing feedback - Suyash, thanks a lot Sir for the new patch :) I'm still having a hard time reading and imagining the behaviour from just looking at the code (can't test), but I tried hard and afasics this should work largely as expected (and described in my earlier comments). Great step ahead! Still, I'd really need to see this in action for finetuning... I also have a few minor comments and questions... ::: mail/components/compose/content/MsgComposeCommands.js @@ +136,5 @@ > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > gHideMenus = false; > gRemindLater = false; > + gManualAttachmentReminder.showReminder = false; > + gManualAttachmentReminder.lastNumberOfAttachments = 0; I have an odd feeling about this. What if it's a forwarded message or composition from commandline coming with pre-attached attachments? lastNumberOfAttachments must always match the *real* number of attachments (itemcount), otherwise I think we'll end up switching off the checkmark in some cases where the automagical reminder is still on (and by design, the checkmark on "Remind me later" menu is a combined indication of both manual and automagical reminder, where manual toggling can overrun automagical). Perhaps the only exception where we will actually remind without showing checkmark on menu is this (as in current code, no changes required): - notification bar shown (not dismissed, reminder button not pressed) - Remind me later on menu not set by user - attachment keywords found - automagical-aggressive = on So basically, user did nothing at all (hence no explicit checkmark) but we have reason to believe from body text that an attachment might be missing. Btw, does setting the manual reminder survive closing and reopening the draft? STR: - new composition - activate manual reminder ("Remind me later") from menu, but do not add attachment - save, close, and reopen draft - what is the status of "Remind me later" checkmark? expected: - "Remind me later" should survive closing and reopening the draft @@ +412,4 @@ > } > }, > > cmd_attachCloud: { Ideally, cloud attachments should also count as attachments... And as they are probably counted in attachmentbucket.itemcount, I think we need to handle them the same way as their local file friends. @@ +670,5 @@ > return selectedCount > 0; > }, > doCommand: function() { > RemoveSelectedAttachment(); > + if (gManualAttachmentReminder.showReminder && Does this command cover removal of any of - local file attachments - web pages - cloud files? We need to catch them all. @@ +2444,5 @@ > > gAutoSaveKickedIn = false; > } > > +function invokereminder() I think that... function toggleAttachmentReminder() ...would describe this much better, because "invoke" sounds to me like "switch on" but we are really toggling on/off depending on previous state. @@ +2450,5 @@ > + let showReminder = !gManualAttachmentReminder.showReminder; > + gManualAttachmentReminder.showReminder = showReminder; > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > + if (!showReminder) > + gRemindLater = false; As explained below, shouldn't we set this here, too: gManualAttachmentReminder.showReminder=false @@ +2453,5 @@ > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = > + document.getElementById("attachmentBucket").itemCount; I think when user sets the manual attachment reminder from menu to true ("Remind me, please!"), we should also do this: - gManualAttachmentReminder.showReminder=true (explained below, so that .showReminder is always reflecting life status of our new manual reminder) - gRemindLater=true (same effect as if user pressed remind me button on the notification bar) - hide the reminder bar if it's currently shown (because after manual activation from menu, reminder is already active, so the bar is pointless henceforth) Bar should reappear as usual when circumstances require (e.g. all attachments removed and attachment words found in body). Sorry I'm still wrapping my head around the desired behaviour, but actually this is not yet contradicting what I said before ("no need to show bar when reminder comes on"). @@ +2771,5 @@ > + if (document.getElementById("cmd_remindLater").getAttribute("checked") == "true") { > + gManualAttachmentReminder.showReminder = true; > + } else { > + gManualAttachmentReminder.showReminder = false; > + } This looks strange and somewhat duplicated codewise, to set .showReminder from "checked" just shortly before we actually remind. I think we should probably always toggle .showReminder in the code before whenever we toggle "checked" (to keep them in sync), and then just read .showReminder here. Or should we keep the "checked" check as a security, e.g. when addons (or we) just change "checked" but forget to change .showReminder? @@ +2777,2 @@ > document.getElementById("attachmentNotificationBox").currentNotification)) && > + ShouldShowAttachmentNotification(false)) || gManualAttachmentReminder.showReminder) { Yes, this is where we finally play the trick :) ::: mail/components/compose/content/messengercompose.xul @@ +106,5 @@ > <command id="cmd_attachFile" oncommand="goDoCommand('cmd_attachFile')"/> > <command id="cmd_attachCloud" oncommand="attachToCloud(event.target.cloudProvider); event.stopPropagation();"/> > <command id="cmd_attachPage" oncommand="goDoCommand('cmd_attachPage')"/> > <command id="cmd_attachVCard" checked="false" oncommand="ToggleAttachVCard(event.target)"/> > + <command id="cmd_remindLater" checked="false" autocheck="false" oncommand="invokereminder()"/> For added clarity, I think this would be better: id="cmd_attachmentRemindLater" remindLater might be concise, but it's not very clear what we are reminding about. in the same line, invokereminder should be changed to something better like "toggleAttachmentReminder" as explained above. @@ +443,5 @@ > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator/> > + <menuitem id="remindLater_menu" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" > + command="cmd_remindLater"/> dito Personally, I'd also prefer id="attachmentRemindLater_menu" to provide more context. @@ +712,5 @@ > + <menuseparator/> > + <menuitem id="remindLater_button" type="checkbox" > + label="&remindLater.label;" > + accesskey="&remindLater.accesskey;" > + command="cmd_remindLater"/> dito re command="..." Personally, I'd also prefer id="attachmentRemindLater_button" to provide more context.

Comment on attachment 798206 [details] [diff] [review] Patch v3 see feedback in my comment 26

(In reply to Thomas D. from comment #26) > Comment on attachment 798206 [details] [diff] [review] > Patch v3 > > Review of attachment 798206 [details] [diff] [review]: > ----------------------------------------------------------------- > > - providing feedback - > Suyash, thanks a lot Sir for the new patch :) Sorry :( but as I said, it wasn't working because of bug 887010. > > I'm still having a hard time reading and imagining the behaviour from just > looking at the code (can't test), but I tried hard and afasics this should > work largely as expected (and described in my earlier comments). Great step > ahead! > You try this patch, you'll like it :) > Still, I'd really need to see this in action for finetuning... > > I also have a few minor comments and questions... > > ::: mail/components/compose/content/MsgComposeCommands.js > @@ +136,5 @@ > > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > > gHideMenus = false; > > gRemindLater = false; > > + gManualAttachmentReminder.showReminder = false; > > + gManualAttachmentReminder.lastNumberOfAttachments = 0; > Ya, this is interesting, I'll make this change. > @@ +412,4 @@ > > cmd_attachCloud: { > > Ideally, cloud attachments should also count as attachments... And as they > are probably counted in attachmentbucket.itemcount, I think we need to > handle them the same way as their local file friends. > No idea, there was no code for attachFile() inside the do section, so I skipped this as well, I think, this is being covered. > @@ +670,5 @@ > > return selectedCount > 0; > > }, > > doCommand: function() { > > RemoveSelectedAttachment(); > > + if (gManualAttachmentReminder.showReminder && > > Does this command cover removal of any of > - local file attachments > - web pages > - cloud files? > We need to catch them all. > Yes, this is the only removeAttachment() function that I found, so will work for all. > > +function invokereminder() > function toggleAttachmentReminder() > > ...would describe this much better, because "invoke" sounds to me like > "switch on" but we are really toggling on/off depending on previous state. > Will do. > @@ +2450,5 @@ > > + let showReminder = !gManualAttachmentReminder.showReminder; > > + gManualAttachmentReminder.showReminder = showReminder; > > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > > + if (!showReminder) > > + gRemindLater = false; > > As explained below, shouldn't we set this here, too: > gManualAttachmentReminder.showReminder=false > Okay, I was dying for someone to point this out :P Here I go: Actually showReminder is keeping track of the fact that user triggered manual reminder, and rest in between toggling of the check based on the number of attachments is being handled by the "checked" attribute. E.g.: Assume You enabled the manual reminder when you had 'k' attachments and then added 'k+1'th attachment, the check box will disappear. But when you reduced the number of attachments back to 'k' or less, how will the check re-appear? This job is being done here. > - hide the reminder bar if it's currently shown (because after manual > activation from menu, reminder is already active, so the bar is pointless > henceforth) I didn't think of this & hence not sure of the behavior, will look at this. > Bar should reappear as usual when circumstances require (e.g. all > attachments removed and attachment words found in body). > This behavior didn't exist previously, so I don't think this is needed, as once you press "Remind Me Later", whatever you do during the mail composition, if at the time of sending, you have 0 attachments, you will get the attachment reminder alert. > @@ +2771,5 @@ > This looks strange and somewhat duplicated codewise, to set .showReminder > from "checked" just shortly before we actually remind. > It is not according to what I have implemented, I have tried to explain this above. If this behavior isn't desired please let me know, I'll revert it back. Will do the .xul part. And on an unrelated note, this time I expected a f+ but you again cleared the feedback request :(

Comment on attachment 798206 [details] [diff] [review] Patch v3 Review of attachment 798206 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch. I set feedback- because it doesn't support drafts. ::: mail/components/compose/content/MsgComposeCommands.js @@ +76,5 @@ > var gRemindLater; > +var gManualAttachmentReminder = { > + showReminder: false, > + lastNumberOfAttachments: 0 > +}; The variable should only be declared here, InitializeGlobalVariables() for initialization. @@ +136,5 @@ > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > gHideMenus = false; > gRemindLater = false; > + gManualAttachmentReminder.showReminder = false; > + gManualAttachmentReminder.lastNumberOfAttachments = 0; Maybe save it as header, e.g. X-Mozilla-Attachments-Needed: 1 @@ +2453,5 @@ > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = > + document.getElementById("attachmentBucket").itemCount; In my opinion, there should be permanent UI if attachments are missing, e.g. by styling the border of the attachment box in construction style (black and yellow diagonal stripes). It shouldn't be a surprise when the user tries to send the mail. @@ +2771,5 @@ > + if (document.getElementById("cmd_remindLater").getAttribute("checked") == "true") { > + gManualAttachmentReminder.showReminder = true; > + } else { > + gManualAttachmentReminder.showReminder = false; > + } You can write that as: gManualAttachmentReminder.showReminder = (document.getElementById("cmd_remindLater").getAttribute("checked") == "true");

(In reply to Archaeopteryx [:aryx] from comment #29) > Comment on attachment 798206 [details] [diff] [review] > Patch v3 > > Review of attachment 798206 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for the patch. I set feedback- because it doesn't support drafts. > Thanks. > @@ +136,5 @@ > > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > > gHideMenus = false; > > gRemindLater = false; > > + gManualAttachmentReminder.showReminder = false; > > + gManualAttachmentReminder.lastNumberOfAttachments = 0; > > Maybe save it as header, e.g. X-Mozilla-Attachments-Needed: 1 > Sorry but what do you mean by this? > @@ +2453,5 @@ > > + if (!showReminder) > > + gRemindLater = false; > > + if (showReminder) > > + gManualAttachmentReminder.lastNumberOfAttachments = > > + document.getElementById("attachmentBucket").itemCount; > > In my opinion, there should be permanent UI if attachments are missing, e.g. > by styling the border of the attachment box in construction style (black and > yellow diagonal stripes). It shouldn't be a surprise when the user tries to > send the mail. This seems to be a good idea, but will this show up in Linux as well? And is it CSS part, where can I look for this? Any example or the link to the place where this code resides will be great. Thanks again.

> > Maybe save it as header, e.g. X-Mozilla-Attachments-Needed: 1 > > > Sorry but what do you mean by this? I mean storing the number of necessary attachments in that header when storing the message as draft. > > In my opinion, there should be permanent UI if attachments are missing, e.g. > > by styling the border of the attachment box in construction style (black and > > yellow diagonal stripes). It shouldn't be a surprise when the user tries to > > send the mail. > > This seems to be a good idea, but will this show up in Linux as well? > And is it CSS part, where can I look for this? Any example or the link to the > place where this code resides will be great. This can be done in CSS using repeating-linear-gradient, see the third example in https://developer.mozilla.org/en-US/docs/Web/CSS/gradient

Work in Progress patch.

Comment on attachment 805944 [details] [diff] [review] WIP patch Review of attachment 805944 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +1695,5 @@ > + msg.appendChild(msgText); > + msgText.id = "attachmentReminderText"; > + msgText.setAttribute("crop", "end"); > + msgText.setAttribute("flex", "1"); > + let textValue = "You had enabled Attachment Reminder while saving the mail"; Is this displayed to the user? If so it needs to be in the message catalog. @@ +1738,5 @@ > + let buttons = notification.childNodes[0]; > + notification.insertBefore(msg, buttons); > + } > +} > + nit: trailing white space @@ +2047,5 @@ > if (args.body) > composeFields.body = args.body; > } > } > + else Components.utils.reportError("they work " + params.composeFields.attachmentReminder); Always put the else body on the next line. Styles differ between source files, but we're moving toward always using braces around if() and else blocks, even when they only contain a single statement. @@ +2199,5 @@ > + gMsgCompose.compFields.attachmentReminder = showReminder; > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = Trailing white space ::: mailnews/compose/src/nsMsgCompUtils.cpp @@ +464,5 @@ > + PUSH_STRING("uuencode=0; "); > + if (fields->GetAttachmentReminder()) > + PUSH_STRING("reminder=1"); > + else > + PUSH_STRING("reminder=0"); You never parse this information out of the HEADER_X_MOZILLA_DRAFT_INFO header when the draft is restored - the other fields are extracted around https://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1272 ::: mailnews/compose/src/nsMsgCompose.cpp @@ +1276,2 @@ > if (m_compFields) > + { New code should have the { on the same line as the "if (...)" I know this file isn't consistent about that style.

Thanks irving for mimedrft.cpp. The patch is almost complete, just that the attachment bar doesn't persist and hides quickly (I am not sure why). Thanks.

Comment on attachment 806054 [details] [diff] [review] Patch v4 Review of attachment 806054 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +1734,5 @@ > + notification.insertBefore(msg, buttons); > + } > + CheckForAttachmentNotification.shouldFire = true; > +} > + trailing whitespace @@ +2067,5 @@ > document.getElementById("cmd_attachVCard") > .setAttribute("checked", gMsgCompose.compFields.attachVCard); > + if (gMsgCompose.compFields.attachmentReminder) { > + attachmentNotificationBar(); > + } I don't see why we need this notification. It should just remind if you're about to send without an attachment. @@ +2186,5 @@ > + gMsgCompose.compFields.attachmentReminder = showReminder; > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = trailing whitespace ::: mail/components/compose/content/messengercompose.xul @@ +723,5 @@ > label="&attachVCardCmd.label;" > accesskey="&attachVCardCmd.accesskey;" > command="cmd_attachVCard"/> > + <menuseparator/> > + <menuitem id="remindLater_button" type="checkbox" not a button, also use the same scheme as the other ids around here ::: mailnews/base/public/nsIMsgIdentity.idl @@ +60,5 @@ > */ > attribute boolean attachVCard; > > /** > + * Should we show the attachment bar? remind about missing attachments @@ +62,5 @@ > > /** > + * Should we show the attachment bar? > + */ > + attribute boolean attachmentReminder; This doesn't belong in nsIMsgIdentity... Should be just the compose field

Comment on attachment 806054 [details] [diff] [review] Patch v4 Review of attachment 806054 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +1729,5 @@ > + notification = nBox.appendNotification("", "1", > + "null", > + nBox.PRIORITY_WARNING_MEDIUM, > + [addButton, remindButton]); > + let buttons = notification.childNodes[0]; Use firstElementChild instead of childNodes[0], see https://developer.mozilla.org/en-US/docs/Web/API/element

Comment on attachment 806054 [details] [diff] [review] Patch v4 Suggestions regarding making the bar persistent are most welcome.

Comment on attachment 806054 [details] [diff] [review] Patch v4 Review of attachment 806054 [details] [diff] [review]: ----------------------------------------------------------------- (for some reason this didn't compile for me, dunno why, the error looked to be false)

(In reply to Suyash Agarwal (:sshagarwal) away till Oct 5 from comment #37) > Comment on attachment 806054 [details] [diff] [review] > Patch v4 > > Suggestions regarding making the bar persistent are most welcome. Suyash, could you spell out with a bit more detail under which circumstances/STR the notification bar does not "persist" for you with patch applied? Notification bar persistence also touched in bug 833909 for scenario of closing draft, then edit as new. Something worse is bug 919286 where we forget about user's explicit instruction "Remind me later" when closing and re-opening draft. That scenario will be more frequent and more severe after implementing manual attachment reminder in this bug 521158, so we should fix it asap. More frequent: Per this bug, user can activate attachment reminder any time, not just when having keywords, and not only if there's no attachment yet. More severe: As this bug will allow activating the reminder even when you already have attachments, it will be all but impossible to know if an attachment is missing from just looking at attachment pane. So users really need to rely on the reminder to be persisted regardless of closing/reopening draft.

By "bar doesn't persist" I mean, the bar definitely shows up, but hides quickly. I think this issue is different than both the other bugs you mentioned. But, this patch fixes them as well, as now we have a notification bar both in the re-opening of the draft and editing the draft as new. So, if this lands, all these three are supposed to be fixed. So, the question that remains left is, do you want the notification bar the next time or just the attachment reminder alert when the user clicks on send with no notification of any sort of whether the user ever chose to be reminded of the attachments? Thanks.

(In reply to Suyash Agarwal (:sshagarwal) away till Oct 5 from comment #40) > By "bar doesn't persist" I mean, the bar definitely shows up, but hides > quickly. So you're not touching anyting in the UI and the notification bar at the bottom of compose window just goes away by itself after some seconds? > But, this patch fixes them as well, as now we have a notification bar both > in the re-opening of the draft and editing the draft as new. For re-opening the draft ("Edit" button in message reader), we only want to show the notification bar if it was still shown at the time of saving the draft. I'd think the same applies for "Edit as new", which should also preserve the same status as when it was saved. So if the notification bar was gone before saving, it should not show up upon "Edit as new". > So, if this > lands, all these three are supposed to be fixed. My uninformed intuition is that bug 919286 will need further code to be fixed. > So, the question that remains left is, do you want the notification bar the > next time or just the attachment reminder alert when the user clicks on send > with no notification of any sort of whether the user ever chose to be > reminded of the attachments? The notification bar is just a vehicle trying to invite the user into a choice: - attach now - remind me later - ignore notification (-> remind me later with default of reminder_agressive pref=true) As soon as the user has made that choice, the notification bar is irrelevant (will only come back if you ignore and later type new reminder keywords which you haven't ignored yet, and user can't prevent that for the current message which is the first half of bug 648256). If the user decides "Remind me later" (from notification or menu), thereby dismissing the notification bar and activating the final reminder alert, I think we should respect that and not nag him with another notification bar when re-opening the draft. Iow, per ux-consistency and ux-wysiwyg, we should preserve exactly the same status of UI and behaviour which was there upon saving when re-opening the draft, or "Edit as new". * So if there was no notification bar when saving, don't show one for re-opening or "edit as new". It's OK to just show the reminder alert "out of the blue" when sending because user either actively opted in via "Remind me later", or actively ignored the notification by closing it. In both cases, he chose to not have the notification bar showing, so we should respect and preserve that. * Otoh, if there *was* a notification bar when saving, also show it when re-opening or "edit as new". > with no notification of any sort of whether the user ever chose to be > reminded of the attachments? If this bug does what I envision, the "Remind me later" checkbox menu implemented here will also serve as a real-time status indicator if, after user chose "Remind me later" either from bar or menu, the *opt-in* reminder *alert* will *actually* trigger when sending or not (similar to the alarm button on your old-style alarm clock: switch it on and you'll see the red button part sticking out as an indicator of active alarm. When the red is not seen, you'll not get an alarm, but you can always reactivate it). As an exception, we'll not show the manual reminder as checked when the alert will come through reminder_aggressive pref (which is good behaviour, because reminder_aggressive only triggers when notification is still around, which is too easy to dismiss so it would be confusing to show manualReminder checked whilst user never opted in to be reminded). Alert-status-indicating behaviour of "Remind me later" menu (this bug): - when user checks "Remind me later" from menu > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = > + document.getElementById("attachmentBucket").itemCount; n=current number of attachments at the time of activating reminder if notification bar is shown (implying n=0), *close notifaction bar* (no longer needed as manual reminder has been activated and will persist) (Suyash, that's still missing in function toggleAttachmentReminder(), section: if (showReminder)) we imitate a listener function in cmd_attachFile, cmd_attachPage, and RemoveSelectedAttachment() via cmd_attachFile, cmd_attachPage: if numAttachments > n -> uncheck (to indicate status that we currently won't alert, but the manualReminder still active in background to watch number of attachments) via RemoveSelectedAttachment: if numAttachments < n+1 -> check (auto-reactivate/re-check manual reminder, alert when sending because number of attachments fell below manual threshold of n+1) - when sending if manual reminder is on (which implies numAttachments < n+1 per listener functions) -> alert if numAttachments = 0 AND notification not dismissed AND reminder_aggressive=true -> alert

There's a couple of design choices which we've made here so far: * when user clicks "Remind me later" (from menu or notification bar), we'll ensure that at least n+1 attachments are present when sending, and alert upon sending if not (n being the current number of attachments at the time of activating the reminder); if n+1 attachments are found when sending, we'll NOT alert, so it's a dynamic, conditional reminder. * Generally speaking, at any given time, the "checked" status of "Remind me later" option in the menu corresponds to whether user will see the attachment reminder alert when sending or not. More specifically, status indications are as follows: - checked: User opted in to "Remind me later" AND has not added another attachment since, so the opt-in alert *will* trigger upon sending. To that end, we'll constantly monitor the total number of attachments until user explicitly *unchecks* the manual reminder. - unchecked: - User did *not* opt in to "Remind me later" so the /opt-in/ alert *will NOT* triggger upon sending (however, the /reminder_aggressive/ alert *might* trigger if there are no attachments and the notification has been ignored.) - OR, if he did opt in, he's added another attachment since; so the alert effectively *will NOT* triggger upon sending (but, as one of the main benefits of this bug, you can re-activate the reminder to trigger anyway if you don't add yet another attachment). * whichever status the reminder options are in should be preserved exactly for re-opening a saved draft or "edit as new"

Okay, so I think I am unable to explain what new notification bar is doing.. here are two screenshots showing the scenario. If you still think the behavior isn't as expected/desired. Please let me know. Thanks.

This is the new notification bar that is shown up when the user selected "remind me later" from the menu or the attachment notification bar that had appeared from the attachment specific words in the message body.

Comment on attachment 808515 [details] reminder bar when re-opening draft.png Sir, Can you please provide a feedback on the screen-shot on whether the behavior is as expected/desired or not?

Comment on attachment 808515 [details] reminder bar when re-opening draft.png Suyash, given that you are introducing a new feature which is not part of this bug and has never been discussed here nor fully explained/defined concerning it's interactions with existing and new design per this bug, I think it's premature to ask for ui-review which is hardly possible at this stage, even feedback would be pretty hard for any person not having followed this bug from the beginning. Let's explore, discuss and fully understand / define the workflows first before that. As I tried to narrow down in my comment 41, I have several doubts that this new behaviour will provide a good workflow and behaviour. And certainly it's not what I as reporter had in mind for this bug. But I think either way, we're almost there. My idea for this bug is very simple: * Check "Remind me later" from the menu -> Get reminded about adding another attachment when sending (unless you've already added another attachment by then). So it's like activating a "hard" reminder which allows you to focus on your composition without worrying about attachments at this time, but with the safety of the reminder alert(!) at send time. This very simple idea comes with several benefits over status quo: 1) independent of reminder keywords in body: you can activate reminder even without such words (whereas current mechanism only triggers on words). 2) independent of current number of attachments: you can activate reminder any time even when you already have one or more attachments, to be reminded of adding yet another one before sending (whereas the current notification/ reminder mechanism will only operate when you have no attachments yet). 3) more transparent, less volatile, less clutter: As explained in comment 42, the checkbox status in the menu creates transparency about the actual reminder status: If it's checked, you'll definitely get reminded by alert when sending; if it's unchecked, you'll generally *not* be reminded (whereas with current reminder, you can get into states where you don't know and have no way of verifying if reminder will trigger or not). So the complementary behaviour proposed here is more stable than rather volatile nature of current notification. And after activating "Remind me later" from current notification bar or new menu, you can continue working clutterfree without notification bar, with peace of mind knowing you'll be reminded later. Workflow/behaviour/disadvantages with Suyash new bar: - activate alert reminder by clicking "Remind me later" from notification or new menu (so notification bar is gone, and reminder alarm is set for n attachments). - after saving,closing and reopening of draft, secondary notification bar reappears (why? haven't I dismissed that already? violates ux-consistency & ux-wysiwyg) - user needs to click on "Remind me later" again (!) to return to same stable and clutterfree state of reminder as when saving the draft - checkbox status of "Remind me later" in menu must be unchecked (why? haven't I just checked it?), because we can't have a button "Remind me later" when "Remind me later" is already active; so the alert reminder is also inactive? - what happens if user closes this secondary notification? Is reminder still active (as it was when saving draft) or not? So in short, instead of just persisting the status of reminder across saving and re-opening of draft, Suyash new bar silently swaps a confirmed and stable background reminder (alert when send) with an unconfirmed and volatile secondary notification bar (which user has previously done away with, and which now requires another user intervention). I doubt that the benefit of a visual invitation to add the attachment immediately after re-opening the draft outweighs thouse ux disadvantages. I expect this will also be harder to code, as this bar is different in behaviour and purpose than 1st notification bar. The technical reason why it disappears "so quickly" is probably due to the code of 1st notification bar, which was designed to disappear after user has decided for "Remind me later". I'll try to let this sink in a bit, but at present I think it's just making things more complex and disgressing from the original purpose of this bug. If such disgressions are wanted, I'd much prefer to try them in a separate bug, after finishing and testing the original behaviour proposed in this bug. Having said which, I think all the rest of the patch is very good and unless I've overlooked something (which is possible given the complex internal workings) I think we're almost there, if not already there.

(In reply to Suyash Agarwal (:sshagarwal) away till Oct 5 from comment #44) > Created attachment 808515 [details] > reminder bar when re-opening draft.png > > This is the new notification bar that is shown up when the user selected > "remind me later" from the menu or the attachment notification bar that had > appeared from the attachment specific words in the message body. It's only shown after *re-opening* or "editing as new" of the draft, not immediately after clicking "Remind me later" on menu or first notification bar, right?

Comment on attachment 806054 [details] [diff] [review] Patch v4 Review of attachment 806054 [details] [diff] [review]: ----------------------------------------------------------------- I have finally tried this and I see these problems: 0. After checking "Remind me later" in the menu, no bar is shown. Not sure if that is expected. See my comments below. 1. Saving a draft with having "Remind me later" the draft opened, the reminder quickly flashed by and disappeared. 2. The menu item "Remind me later" was not checked. Upon sending the email, I was not notified of missing attachments. (I determined that before editing again the draft had the "reminder=1" flag set in the source. 3. Sometimes the reminder bar stays shown when reediting a message, sometimes not. 4. The "reminder=1" flag probably should be called more specifically, e.g. attachmentReminder=1. We can have some other reminders in the future. 5. Both the buttons on the reminder bar have U as the accesskey. 6. There is the small close button on the reminder bar with unknown meaning. If the bar is supposed to stay shown for longer periods, I think the button should be removed. And there should be a new button "Never remind me again", which would turn this manual reminding off (equivalent to turning it off from the menu). For my taste I would like the reminder bar to only show up for several seconds after it is activated from the menu and then again when the draft is re-edited. But as I not sure I will ever use this feature and I think Thomas is more interested in it (he is the reporter), more weight should be put to his desired design description. But we should really hear some thoughts from bwinton already. When this patch is getting into C++ and adding flags to message source, there should be already a agreed upon design document so that Suyash's work is not wasted with too many attempts and experiments.

Indeed, this bug got a bit bloated at some point. Reminding for drafts wasn't really part of it to begin with - and we could use bug 919286 to do that, still. But i'll leave that for Suyash to decide.

(In reply to :aceman from comment #48) > Review of attachment 806054 [details] [diff] [review]: > ----------------------------------------------------------------- aceman, thanks for your review. I wish I could do it concise like that ;) > I have finally tried this and I see these problems: Agree with most of these, but some are design problems inherent to this new type of bar as already mentioned in Workflow part of my comment 46. > 6. There is the small close button on the reminder bar with unknown meaning. > If the bar is supposed to stay shown for longer periods, I think the button > should be removed. Unknown meaning of [x] may or may not be a problem, but having a bar which I can't close unless clicking on possibly unwanted button actions like "Remind me later" looks a bigger problem to me. > And there should be a new button "Never remind me again", > which would turn this manual reminding off (equivalent to turning it off > from the menu). +1, but only for the primary (word-triggered) notification as far as I'm concerned. "Never again" sounds too strong and wrong because it's only about current message and can be reactivated, so what about making that a dropdown button on primary notification bar, like this: [Remind me later | v ] +---------------------------------+ |Don't remind me for this message | +---------------------------------+ Same design principle as in FF password save prompt. > For my taste I would like the reminder bar to only show up for several > seconds after it is activated from the menu and then again when the draft is > re-edited. Despite the significant devil in the detail and workflow design (see comment 46), I can see the intention and some potential benefit of showing the secondary notification bar /when re-opening/ the draft as proposed by Suyash, sort of re-confirming the "Remind me later" setting with a visual invitation after re-opening. But I'm entirely failing to see what is the point of showing a notification bar having "Add attachment" and "Remind me later" buttons /just after/ user explicitly clicked on "Remind me later", i.e. we know that he does NOT want to add attachments now, and he does NOT want to be reminded now, but later. Aceman, could you explain the workflow/purpose of instantly showing the bar? If it's only about providing visual feedback/confirmation that "Remind me later" has been successfully enabled from menu, than indeed it should be a simple notification bar with no buttons except a small [x] to close, probably showing up for just a few seconds, with an informative text like +---------------------------------------------------------------------------------------------------+ | You will be reminded of adding another attachment before sending the message. [x]| +---------------------------------------------------------------------------------------------------+ Don't know if that helps in the long run, user can check status of reminder any time from the "Remind me later" checkbox menu in Attach button dropdown per this bug.

(In reply to Thomas D. (away till 23rd Oct) from comment #47) > It's only shown after *re-opening* or "editing as new" of the draft, not > immediately after clicking "Remind me later" on menu or first notification > bar, right? (In reply to :aceman from comment #48) > Comment on attachment 806054 [details] [diff] [review] > Patch v4 > > Review of attachment 806054 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have finally tried this and I see these problems: > 0. After checking "Remind me later" in the menu, no bar is shown. Not sure > if that is expected. See my comments below. (In reply to Thomas D. (away till 23rd Oct) from comment #50) > (In reply to :aceman from comment #48) > > Review of attachment 806054 [details] [diff] [review]: > > ----------------------------------------------------------------- > But I'm entirely failing to see what is the point of showing a notification > bar having "Add attachment" and "Remind me later" buttons /just after/ user > explicitly clicked on "Remind me later", i.e. we know that he does NOT want > to add attachments now, and he does NOT want to be reminded now, but later. > If it's only about providing visual feedback/confirmation that "Remind me > later" has been successfully enabled from menu, than indeed it should be a > simple notification bar with no buttons except a small [x] to close, This bar is explicitly shown when user opens the draft or edits the draft as new. It has no connection of any sort with the current message being composed. Even my screen shot name suggests that. Maybe I wasn't able to explain it better. So, according to the current implementation, nothing will be shown after you click on "Remind Me Later" either from the notification bar or the menu, except the check on the option. The sole intention of this bar is: When you save this message as draft and re-open it, or edit it as new, then you see that bar saying, "Hey! You wanted to be reminded about attachments, (I thought you would have forgot that and would blame me if I reminded you at the time of sending, so I am doing it now), so would you like to add the attachments now, or would like to be reminded later". I like the idea of the third button and will definitely implement it. And for the quick hiding of the bar, I told you I am having this problem and I am unable to figure out the solution. Though the bar persists when you don't have any contact in the To field, otherwise, it hides quickly after showing. Thanks.

(In reply to Magnus Melin from comment #49) > Indeed, this bug got a bit bloated at some point. Reminding for drafts > wasn't really part of it to begin with - and we could use bug 919286 to do > that, still. But i'll leave that for Suyash to decide. I like this idea! Thanks :) Because things seem to be taking time, I have no problem in completing this bug here itself. On opening the draft or editing it as new, you'll have the check on the "Remind me later" option in the menu, and so ofcourse you will get the alert while sending. Though I like the new notification bar I am working on, if you all agree on it, I can put it in the other followup bugs i.e. either bug 833909 or bug 919286. So, shall I just restore the check on opening the draft and get this done?

(In reply to Suyash Agarwal (:sshagarwal) away till Oct 5 from comment #52) > So, shall I just restore the check on opening the draft and get this done? Sounds good to me! Personally i think the other stuff are related good/bad feature requests or just making something in principle simple more complicated than it should be.

(In reply to Magnus Melin from comment #53) > (In reply to Suyash Agarwal (:sshagarwal) away till Oct 5 from comment #52) > > So, shall I just restore the check on opening the draft and get this done? > > Sounds good to me! Personally i think the other stuff are related good/bad > feature requests or just making something in principle simple more > complicated than it should be. +1. Doing the no-whistles version first will also make it much easier to get ui-review for this. Design and benefits as described in first half of comment 46, starting from "My idea for this bug...", ending with "peace of mind knowing you'll be reminded later." If it's hard to add the |Don't remind me for this message| dropdown button, that could/should also be done in another bug.

(In reply to Suyash Agarwal (:sshagarwal) away till Oct 5 from comment #52) > Though I like the new notification bar I am working on, if you all agree on > it, I can put it in the other followup bugs i.e. either bug 833909 or bug > 919286. And please open a new bug for your RFE of having secondary follow-up reminder notification bar when re-opening / editing-as-new of draft because bug 833909 and bug 919286 are technical bugs of the current implementation which need to be fixed regardless of that RFE, so mingling that isn't helpful. I wonder how hard it would be to fix bug 919286 here if we just set the draft's manual reminder flag (introduced by the patch here) when user clicks "Remind me later" from the primary word-triggered notification bar. But perhaps it's still better to fix it separately in bug 919286 anyway, rather than adding more complexity to the already hard-to-read code here.

Okay, so now this is as desired, with nothing at all extra.

There was a nit that needed attention :)

Comment on attachment 809157 [details] [diff] [review] Patch v5.1 Review of attachment 809157 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r=mkmelin with some nits fixed. ::: mail/components/compose/content/MsgComposeCommands.js @@ +1746,5 @@ > label: getComposeBundle().getString("remindLaterButton"), > callback: function (aNotificationBar, aButton) > { > gRemindLater = true; > + document.getElementById("cmd_remindLater").setAttribute("checked", true); i think it's supposed to be "true" (as string), even though this usually works too. same thing in a few other places too. @@ +2130,5 @@ > + gMsgCompose.compFields.attachmentReminder = showReminder; > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = trailing whitespace ::: mail/components/compose/content/messengercompose.xul @@ +105,5 @@ > <command id="cmd_attachFile" oncommand="goDoCommand('cmd_attachFile')"/> > <command id="cmd_attachCloud" oncommand="attachToCloud(event.target.cloudProvider); event.stopPropagation();"/> > <command id="cmd_attachPage" oncommand="goDoCommand('cmd_attachPage')"/> > <command id="cmd_attachVCard" checked="false" oncommand="ToggleAttachVCard(event.target)"/> > + <command id="cmd_remindLater" checked="false" autocheck="false" oncommand="toggleAttachmentReminder()"/> i don't think commands even have a autocheck attribute @@ +722,5 @@ > type="checkbox" > label="&attachVCardCmd.label;" > accesskey="&attachVCardCmd.accesskey;" > command="cmd_attachVCard"/> > + <menuseparator/> add an id for the menuseparator (the other separators are also lacking ids). e.g. id="button-attachPopup_RemindLaterSep" @@ +723,5 @@ > label="&attachVCardCmd.label;" > accesskey="&attachVCardCmd.accesskey;" > command="cmd_attachVCard"/> > + <menuseparator/> > + <menuitem id="remindLaterItem" type="checkbox" this id should also follow the same naming conventions as the other items in the popup. " like id="button-attachPopup_remindLater ::: mailnews/compose/public/nsIMsgCompFields.idl @@ +43,5 @@ > attribute boolean forcePlainText; > attribute boolean useMultipartAlternative; > attribute boolean bodyIsAsciiOnly; > attribute boolean forceMsgEncoding; > + attribute boolean attachmentReminder; add documentation for this, above it add tis: /// Should we remind about missing attachments.

Comment on attachment 809157 [details] [diff] [review] Patch v5.1 After playing around with this for a bit, and thinking through the various scenarios, I'm pretty sure I like it! ui-r=me!

Comment on attachment 809157 [details] [diff] [review] Patch v5.1 Review of attachment 809157 [details] [diff] [review]: ----------------------------------------------------------------- I have run with this and it seems to work reliably this time. The reminder state is stored in the draft properly. Please comment in the patch on how the ("checked" attribute of "cmd_remindLater") is used differently to gManualAttachmentReminder.showReminder (otherwise they would look like duplicates). ::: mail/components/compose/content/MsgComposeCommands.js @@ +393,5 @@ > doCommand: function() { > AttachFile(); > + if (document.getElementById("attachmentBucket").itemCount > > + gManualAttachmentReminder.lastNumberOfAttachments) { > + document.getElementById("cmd_remindLater").setAttribute("checked", false); You have several constructs like this, can you abstract it out into a function that just does the right thing (there does not seem to be any needed context fromt he call places passed in)? Then just call the function from all these places (attachment manipulation). @@ +2125,5 @@ > +function toggleAttachmentReminder() > +{ > + let showReminder = !gManualAttachmentReminder.showReminder; > + gManualAttachmentReminder.showReminder = showReminder; > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); This does not seem right. There are places where you clear "checked" without .showReminder (you will describe the difference in the comment). But the user see the "checked" state here and tries to toggle it. But you toggle based on .showReminder. So it might result in the opposite state than the user wanted. @@ +2126,5 @@ > +{ > + let showReminder = !gManualAttachmentReminder.showReminder; > + gManualAttachmentReminder.showReminder = showReminder; > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > + gMsgCompose.compFields.attachmentReminder = showReminder; Is this needed? You seem to set this again in GenericSendMessage() so the value seems overwritten. @@ +2129,5 @@ > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > + gMsgCompose.compFields.attachmentReminder = showReminder; > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) Should this be just an "else" ? @@ +2131,5 @@ > + if (!showReminder) > + gRemindLater = false; > + if (showReminder) > + gManualAttachmentReminder.lastNumberOfAttachments = > + document.getElementById("attachmentBucket").itemCount; Indent by another 2 spaces.

Fixed the nits.

Comment on attachment 812605 [details] [diff] [review] Patch v6 Review of attachment 812605 [details] [diff] [review]: ----------------------------------------------------------------- Looks better now. But where is the comment about difference of .showReminder and cmd_remindLater checked? ::: mail/components/compose/content/MsgComposeCommands.js @@ +393,5 @@ > + (document.getElementById("attachmentBucket").itemCount <= > + gManualAttachmentReminder.lastNumberOfAttachments)) { > + document.getElementById("cmd_remindLater").setAttribute("checked", "true"); > + } > +} Can you please make this readable by assigning document.getElementById("attachmentBucket").itemCount into a variable and document.getElementById("cmd_remindLater") into another one? End just return from the function early if !gManualAttachmentReminder.showReminder (no need to unnecessarily set state on elements). @@ +1749,5 @@ > callback: function (aNotificationBar, aButton) > { > gRemindLater = true; > + document.getElementById("cmd_remindLater").setAttribute("checked", "true"); > + gManualAttachmentReminder.showReminder = true; Is this needed in the current version? This seems like you affect the manual reminder from a button that shows in the automatic reminder? @@ +2126,5 @@ > + gManualAttachmentReminder.showReminder = showReminder; > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > + gMsgCompose.compFields.attachmentReminder = showReminder; > + if (!showReminder) > + gRemindLater = false; Do you really want to set this gRemindLater variable? Isn't that used for the automatic reminder which you should not affect. Or is it intentional? @@ +2440,5 @@ > // - the button to remind about attachments was clicked, or > // - the aggressive pref is set and the notification was not dismissed > // and the message (still) contains attachment keywords. > + gManualAttachmentReminder.showReminder = > + (document.getElementById("cmd_remindLater").getAttribute("checked") == "true"); Can the user cancel send at this spot? If he does, do we not want to preserve the value of gManualAttachmentReminder.showReminder to stay in the same state he was before the sending attempt? ::: mailnews/compose/public/nsIMsgCompFields.idl @@ +44,5 @@ > attribute boolean useMultipartAlternative; > attribute boolean bodyIsAsciiOnly; > attribute boolean forceMsgEncoding; > + /// Remind about missing attachments. > + attribute boolean attachmentReminder; Please update the comment to mean this is the manual reminder.

Made the changes.

(In reply to :aceman from comment #62) > Comment on attachment 812605 [details] [diff] [review] > Patch v6 > > Review of attachment 812605 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +1749,5 @@ > > callback: function (aNotificationBar, aButton) > > { > > gRemindLater = true; > > + document.getElementById("cmd_remindLater").setAttribute("checked", "true"); > > + gManualAttachmentReminder.showReminder = true; > > Is this needed in the current version? This seems like you affect the manual > reminder from a button that shows in the automatic reminder? Yes, actually what I think is that whether the user chooses "Remind me later" from the keyword bar that appears or from the menu, they are manual choices and so accordingly even if the user clicks on the remind later button from the bar, the check should appear and the user's choice should be duly noted. > @@ +2126,5 @@ > > + gManualAttachmentReminder.showReminder = showReminder; > > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > > + gMsgCompose.compFields.attachmentReminder = showReminder; > > + if (!showReminder) > > + gRemindLater = false; > > Do you really want to set this gRemindLater variable? Isn't that used for > the automatic reminder which you should not affect. Or is it intentional? Yes, this is intentional too as mentioned above. If the user removes the check from the menu, that means he/she doesn't want to be reminded about the attachments in any manner. > @@ +2440,5 @@ > > + gManualAttachmentReminder.showReminder = > > + (document.getElementById("cmd_remindLater").getAttribute("checked") == "true"); > > Can the user cancel send at this spot? If he does, do we not want to > preserve the value of gManualAttachmentReminder.showReminder to stay in the > same state he was before the sending attempt? Interesting point, I have tried to include this behavior in the new version of the patch.

And this isn't a good first bug believe me :)

Comment on attachment 812657 [details] [diff] [review] Patch v6.1 I haven't read the bug but the code seems overengineered to me, I would just cancel the reminder after a successful attachment. >+ // Set the shouldShowReminder according to the manual attachment >+ // reminder checkbox. >+ let shouldShowReminder = >+ (document.getElementById("cmd_remindLater").getAttribute("checked") == "true"); >+ >+ if (((gRemindLater || (getPref("mail.compose.attachment_reminder_aggressive") && > document.getElementById("attachmentNotificationBox").currentNotification)) && >+ ShouldShowAttachmentNotification(false)) || shouldShowReminder) { I haven't checked the logic here because I don't know how Thunderbird's existing attachment reminder logic works. >- if (m_compFields) >- m_compFields->GetAttachVCard(&attachVCard); >+ bool attachmentReminder = false; >+ if (m_compFields) { >+ m_compFields->GetAttachVCard(&attachVCard); >+ m_compFields->GetAttachmentReminder(&attachmentReminder); >+ } This change seems unnecessary.

Yes it has the added value that it watches whether attachments were removed and then triggers the reminder again. Neil, I think Suyash wanted to know whether to put the autocheck attribute on the command or on the menuitem. In reply to neil@parkwaycc.co.uk from comment #66) > >- if (m_compFields) > >- m_compFields->GetAttachVCard(&attachVCard); > >+ bool attachmentReminder = false; > >+ if (m_compFields) { > >+ m_compFields->GetAttachVCard(&attachVCard); > >+ m_compFields->GetAttachmentReminder(&attachmentReminder); > >+ } > This change seems unnecessary. Why. Is is a duplicate of anything? We need to read the reminder state from the saved message draft.

Comment on attachment 812657 [details] [diff] [review] Patch v6.1 Review of attachment 812657 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me now, thanks. ::: mail/components/compose/content/MsgComposeCommands.js @@ +2130,5 @@ > + // whereas, cmd_remindLater's checked attribute will keep changing on the > + // attachment counts as well. > + // The main intention of keeping this difference is when the attachment count > + // decreases, I need a way to know that I have to light up the "Remind Me Later" > + // button and that can only be done by the showReminder attribute. -Suyash Button?

Comment on attachment 812657 [details] [diff] [review] Patch v6.1 Requesting review again as there have been significant changes.

Why were there significant changes needed?

Oh, it was before my feedback :)

Comment on attachment 812657 [details] [diff] [review] Patch v6.1 Review of attachment 812657 [details] [diff] [review]: ----------------------------------------------------------------- Reading this again i think I agree with Neil it is overengineered. We don't really need to keep track of the number of attachments. If you tick the menu to remind you, the tick should get removed once an attachment is added - that's all to it. If you later choose to remove an attachment that doesn't mean the tick should come back IMHO. That will simplify the code a lot too.

Getting back to the basic idea after IRC discussion with mkmelin and bwinton.

Comment on attachment 816080 [details] [diff] [review] Patch v0 again I agree with the new direction of this patch as discussed on IRC, which makes the entire behaviour a lot more transparent, controllable and predictable for the user. In fact, it's very much what I originally had in mind when I filed this bug, because we're finally at the point where manually activating "Remind me later" from notification bar or menu really means exactly that: Remind me! (by showing the alert before sending no matter what, as opposed to "Remind me, but only IF this and that...") I talked to Suyash on TeamViever how to streamline the code, eliminate an ux-mode error in the current patch, make it run faster, and fix ux bug 926056 while we are here. So we can expect a new nice patch from Suyash shortly. :)

Okay so here is the patch with the desired modifications and behavior. But there is an issue that doesn't seem acceptable. You choose to be reminded later about the attachments, you click on the send button and you are reminded about the attachments, you choose "oh! I did!" and then you add the attachment(s), (since we have made the behavior of the reminder that, once enabled, can only be disabled by manually un-checking the option) and now you press the Send button, as the check is still there, you again get the attachment alert (What will you do now! Will you entertain this sort of behavior?)!!! Food for thought :) Thanks.

(In reply to Suyash Agarwal (:sshagarwal) from comment #75) > But there is an issue that doesn't seem acceptable [...] > and now you press the Send button, as the check is still there, you *again* > get the attachment alert (What will you do now! Will you entertain this sort > of behavior?)!!! Good catch! No, we're not here to entertain the user with too many alerts... ;) So after user has been reminded with alert, clicked "Oh, I did [forget to add an attachment]!", and then indeed added an attachment, surely we should not show the same alert again. In theory, we could try to ensure that user actually adds an attachment after clicking "Oh, I did [forget to add an attachment]!". However, then we're back to all the code complexity we've just removed, including old and new ux problems that come with it. I cringe at the details... So let's try to get away with simplicity. What about this: The reminder alert has 3 buttons: _Attachment Reminder_ 3[x] Did you forget to add an attachment? 1[No, send now] 2[Oh, I did!] We should react according to the button clicked by user: 1[No, send now] (or Enter without moving focus) -> Just send (obviously) 2[Oh, I did!] -> De-activate the reminder (incl. remove the checkmark) and let the user take care of his attachments 3[x] (or Escape key pressed) -> don't change anything, i.e. reminder remains active as it was before (obviously) I think 1 and 3 are obvious, so let's look at 2 a bit more to understand what we're doing: 2 [Oh, I did!] -> De-activate the reminder and let the user take care of his attachments User requested to be reminded, and we *did* show the reminder alert (so already our duty is done). Moreover, by clicking [Oh, I did [forget to add an attachment]!], user even *confirms* that he has been successfully reminded (as he confirms having forgotten to add some attachments). If user doesn't add attachments at this point, well then, it's his own choice and we've tried as instructed. To nag beyond this point is too complex (think about closing & reopening drafts), too aggressive and probably more annoying than helpful.

Two analogies which describe the entire simple behaviour of the reminder now: Alarm clock - switch on during the day (activate attachment reminder any time) - will ring in the morning (upon sending) - if you decide to get up before it rings, and don't switch it off, it will still ring (if you add attachments while reminder is active, and you don't deactivate, it'll still alert you) - when it rings, and you push the big red button, alarm is OFF (if you get alerted, and confirm that with [Oh, I did!], you will not be reminded again) - when it rings, and you just hit snooze button to get rid of the noise, it will ring again (if you get alerted, and press ESC without saying yes or no, you'll get reminded again) Composing a real letter with attachments - Prepare envelope and start writing main letter - Add a sticky note "All papers added?" to remind yourself of double-checking before sending (activate attachment reminder) - Add some of the papers (but perhaps not all of them); sticky note will stay unless you actively remove it (reminder will remain active even after adding an attachment unless you explicitly de-activate it) - When you're done adding all papers, you can remove the sticky note (when you've added all attachments, you can de-activate the reminder) - When you ignore the sticky note (removing it from envelope, so you've seen and touched it!) and then send the letter without adding the missing papers - tough luck! (when you get the alert, confirm it with [Oh, I did!], but still don't add your attachments before sending again - it's up to you! we won't force you...)

FTR (for posterity), some points extracted from IRC moznet #maildev UX input from bwinton: Considerations leading to simpler, manually user-controlled reminder behaviour seen in attachment 816261 [details] [diff] [review] (and away from counting attachments and auto-magically de-activating the reminder when you add attachments): http://logbot.glob.com.au/?c=mozilla%23maildev&s=10+Oct+2013&e=13+Oct+2013#c105419 19:55 sshagarwal bwinton: now the proposed fact is, once the user checks the reminder option and adds even a single attachment, the check should go away, and now whatever happens, it shouldn't return unless specifically user re-checks the reminder option 20:00 bwinton sshagarwal: Right, so our theory is that if the user has interacted with the attachment system, then we should clear the option, because they've obviously done whatever it was that they wanted us to remind them about? 20:00 sshagarwal bwinton: and when now they remove the attachment(s), shouldn't we restore the check? 20:01 bwinton I think I'm leaning the other way, though. The user explicitly asked us to remind them later, so we should do that, no matter what they do with the attachments, because sometimes reminding them later and sometimes not seems like a bad idea. 20:01 bwinton So we shouldn't clear the check in the first place. 20:06 mkmelin bwintons proposal wfm too 20:10 sshagarwal bwinton: so, what is the final verdict? :) 20:14 bwinton Don't remove the check unless manually unchecked, because we have no way of telling what the user meant when they checked the box, and guessing wrong is going to annoy them a lot.

Comment on attachment 816261 [details] [diff] [review] Patch v0.1 Review of attachment 816261 [details] [diff] [review]: ----------------------------------------------------------------- -- providing feedback -- Although I haven't yet seen it in action, I'm starting to really like the simple and straightforward behaviour of this patch (thanks to bwinton and mkmelin for returning us to the original simple idea in the spirit of comment 0, see IRC logs referenced in Comment 78). This patch finally merges in code what we had de facto already merged in behaviour of previous patches: The variables gRemindLater ("Remind me later" from notification bar) and "gManualAttachmentReminder" ("Remind me later" from menu) have now been merged into a single variable with consistent behaviour. Which is good for ux-consistency and avoids unnecessary complexity of behaviour and code. It really does not matter from where the reminder is triggered, the behaviour should be the same. ::: mail/components/compose/content/MsgComposeCommands.js @@ +2096,5 @@ > } > > +function toggleAttachmentReminder() > +{ > + nit: empty row should go @@ +2407,5 @@ > return; > } > } > > // Alert the user if Please add Attachment Reminder to the first row of comment so that it's clear upfront what this is all about: // Attachment Reminder: Alert the user if @@ +2411,5 @@ > // Alert the user if > + // - the user requested "Remind me later" from either the notification bar or the menu > + // (alert regardless of the number of attachments already attached: we can't > + // guess the intended number, and guessing wrong will annoy users a lot), OR > + // - the aggressive pref is set, the latest notification is still showing (i.e. it Let's rephrase this slightly because Suyash and I optimized the code :) // - the aggressive pref is set, AND the latest notification is still showing (implying that: message has no attachment yet, message still contains some attachment keywords, and notification was not dismissed) (You'll have to distribute that across several comment lines) ::: mailnews/compose/public/nsIMsgCompFields.idl @@ +43,5 @@ > attribute boolean forcePlainText; > attribute boolean useMultipartAlternative; > attribute boolean bodyIsAsciiOnly; > attribute boolean forceMsgEncoding; > + /// Manually remind about missing attachments. Perhaps this could be: /// Preserve status of manually-activated attachment reminder

(In reply to Thomas D. (away till 23rd Oct) from comment #79) >(thanks to bwinton and > mkmelin for returning us to the original simple idea in the spirit of > comment 0, see IRC logs referenced in Comment 78). Thanks to Neil too :) Actually I am the real culprit, I requested Neil to look into this, he said something, mkmelin agreed and then bwinton too drifted to this thought. So Blame all of them including me!!! :P

Comment on attachment 816261 [details] [diff] [review] Patch v0.1 Review of attachment 816261 [details] [diff] [review]: ----------------------------------------------------------------- Looks like Thomas' nits to the patch are correct ;)

Comment on attachment 816261 [details] [diff] [review] Patch v0.1 Hi Suyash, per your own Comment 75 this patch isn't ready yet for [ui]reviews because while mostly doing the right thing as discussed, it also introduces a new workflow problem which looks unacceptable, where users who activated reminder, get alerted and then DO add an attachment can get into an "alert loop" that would require them to go back and manually switch off the reminder before sending (which is ux-interruption and hence unwanted). I suggested a very simple and straightforward fix for that ux-interruption problem in my comment 76: - when we've actually shown the reminder alert - AND the user confirms having been reminded by clicking "Oh, I did [forget to add some attachments]!" -> we can safely claim that the manual reminder job is done, and deactivate the manual reminder at this point (gManualReminder=false; and checked=false for cmd_remindLater in the menus). Word-based notification/reminder will not be affected by that afasics. So that way, we provide a reasonable exit condition for the manual reminder that avoids the undesired alert loop. So I suggest that we fix that problem first, and address the nits of comment 79 (confirmed by comment 81), to avoid repetitive [ui]review cycles which will unnecessarily increase the burden on reviewers. Especially for Blake, it will be helpful if we can serve him with a fully functional version which eliminates identified problems. I'll be glad to assist if there are any questions.

Okay no problems :)

(In reply to Thomas D. (away till 23rd Oct) from comment #76) > (In reply to Suyash Agarwal (:sshagarwal) from comment #75) > > But there is an issue that doesn't seem acceptable [...] > > and now you press the Send button, as the check is still there, you *again* > > get the attachment alert (What will you do now! Will you entertain this sort > > of behavior?)!!! > > Good catch! No, we're not here to entertain the user with too many alerts... > So let's try to get away with simplicity. What about this: > > The reminder alert has 3 buttons: > > _Attachment Reminder_ 3[x] > Did you forget to add an attachment? > 1[No, send now] 2[Oh, I did!] > > We should react according to the button clicked by user: > > 1[No, send now] (or Enter without moving focus) > -> Just send (obviously) > > 2[Oh, I did!] > -> De-activate the reminder (incl. remove the checkmark) and let the user > take care of his attachments > > 3[x] (or Escape key pressed) > -> don't change anything, i.e. reminder remains active as it was before > (obviously) Suyash and I looked into this and we're facing a problem here because of Bug 345067 :( The code uses confirmEx and due to bug 345067, the return value for [Oh, I did!] and [x]/ESCape is the same: 1. So it's kinda hard/impossible atm to distinguish between the two cases without using wild hacks :( Any comments or ideas regarding my proposed behaviour to avoid the alert loop, and, if my proposal makes sense, how to work around that new problem of button return values?

Made the suggested changes.

(In reply to Suyash Agarwal (:sshagarwal) from comment #85) > Created attachment 818074 [details] [diff] [review] > Patch v0.2 > > Made the suggested changes. Well, that patch does more than just the suggested changes... (and imho the additional UI changes should be explained and explicitly mentioned to make reviewers aware and allow discussion/weighing of pros and cons). To that end, Suyash, can you please add a screenshot of your modified attachment reminder alert? Suyash worked around the reminder alert return value problem (bug 345067) by adding a third button ("Edit email") to the reminder alert, and he also changed the captions of the two existing reminder buttons: +------------------------------------------------+ | Attachment Reminder | +------------------------------------------------+ | | | Did you forget to add an attachment? | | | | [Send anyway] [Add Attachment] [Edit email] | +------------------------------------------------+ The horizontal distribution of these buttons might differ (depending on OS?), needs screenshots. For comparison, these are the buttons we currently have: | [No, Send Now] [Oh, I did!] | +------------------------------------------------+ So practically, Suyash proposal now involves bug 756396 and bug 516957: Bug 756396 - Attachment reminder button text/captions should be more self-explaining Bug 516957 - When attachment reminder pop-ups on Send, clicking "Oh, I did!" should bring up the Attach File(s) dialog box Some first thoughts/comments: 1) Having three buttons only makes sense if they do different things; iow, three buttons require doing bug 516957. To consider: Should "Add Attachment" really just offer the plain vanilla "Attach File(s)" dialogue, or do we need/want to offer a choice (perhaps as dualbutton with dropdown part) to include "Filelink" attachments (and, less importantly, "Attach Web Page")? 2) The 3rd button, [Edit email], if we want that, should probably be [Edit message] 3) Getting Reminder button captions right (bug 756396) is tricky; I made some attempts in that bug but they are partly obsolete due to the extended manual reminder functionality we are adding here, and not yet aware of bug 516957.

Comment on attachment 818074 [details] [diff] [review] Patch v0.2 (incl. modified reminder alert with 3 buttons, see comment 86) Review of attachment 818074 [details] [diff] [review]: ----------------------------------------------------------------- Please let's not make this more difficult than it needs to be.

Comment on attachment 816261 [details] [diff] [review] Patch v0.1 I think this v0.1 patch is pretty much there, with some small modifications. The original idea to clear the "remind me later" check when an attachment was added would appear to solve the problem. And we already have the aggressive pref for those who always want to be asked. I suggest that when the aggressive pref is set, the check is never cleared automatically. Thinking about it, it would seem inconsistent to behave differently with the menu option compared to the button in the reminder bar. They should imo give the same behavior.

(In reply to Thomas D. (away till 23rd Oct) from comment #86) > (In reply to Suyash Agarwal (:sshagarwal) from comment #85) > > Created attachment 818074 [details] [diff] [review] > > Patch v0.2 > Suyash worked around the reminder alert return value problem (bug 345067) by > adding a third button ("Edit email") to the reminder alert, And actually, correct me if I'm wrong, but current patch Attachment 818074 [details] [diff] does NOT work around the problem of ambiguous hadForgotten return values for MS Windows case of closing alert with [x]/[ESC]: [Send anyway] -> BUTTON_POS_0 -> return value: 0 [Add attachment] -> BUTTON_POS_1 -> return value: 1 [x]/[ESC] -> return value: 1 (bug 345067) [Edit email] -> BUTTON_POS_2 -> return value: 2 So both [Add attachment] and [x]/[ESC] will return 1 and deactivate the manual reminder. * [Add attachment] should certainly deactivate manual reminder, see my comment 76. * I'm not sure if [x]/[ESC] should deactivate manual reminder, but it might be acceptable. Choices: a) interpret [x]/[ESC] as "not sure what I want, so just get me out of this alert without touching anything" -> then we should keep status quo, i.e. manual reminder should remain active b) (current patch) interpret [x]/[ESC] as "I don't care about this, let me get rid of this" -> then we could translate that to "deactivate manual reminder" (given that we've actually shown the reminder alert, and user apparently didn't care as he just closed it with [x]/[ESC]) Aceman, mkmelin, bwinton, anyone, what do you think? Can we get away with just deactivating the manual reminder when the alert has been shown, even when user just escapes from the alert via [x]/[ESC] without explicit choice of action? If we go for b), which kinda works around bug 345067 as we successfully implement exit conditions to set gManualAttachmentReminder=false (deactivated), I don't see why we have to include fixes for bug 516957 and bug 756396 into this bug by adding 3rd button and tweaking button captions; that's not required for this bug so it might just add more discussion points which can be dealt with on those other bugs after we land this and see how it works and feels. Again, what do others think? >--- a/mail/components/compose/content/MsgComposeCommands.js >+++ b/mail/components/compose/content/MsgComposeCommands.js > let flags = Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING + >- Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_IS_STRING; >+ Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_IS_STRING + >+ Services.prompt.BUTTON_POS_2 * Services.prompt.BUTTON_TITLE_IS_STRING; > let hadForgotten = Services.prompt.confirmEx(window, > getComposeBundle().getString("attachmentReminderTitle"), > getComposeBundle().getString("attachmentReminderMsg"), > flags, >- getComposeBundle().getString("attachmentReminderFalseAlarm"), >- getComposeBundle().getString("attachmentReminderYesIForgot"), >- null, null, {value:0}); >+ getComposeBundle().getString("attachmentReminderSend"), >+ getComposeBundle().getString("attachmentReminderAdd"), >+ getComposeBundle().getString("attachmentReminderReturn"), >+ null, {value:0}); > if (hadForgotten) >+ { >+ if (hadForgotten == 1) >+ { >+ gManualAttachmentReminder = false; >+ document.getElementById("cmd_remindLater").setAttribute("checked", "false"); >+ } > return; >+ } > }

(In reply to Magnus Melin from comment #88) > Comment on attachment 816261 [details] [diff] [review] > Patch v0.1 > > I think this v0.1 patch is pretty much there, with some small modifications. +1 > The original idea to clear the "remind me later" check when an attachment > was added would appear to solve the problem. Well, no, per "final verdict" request of bwinton (and I tend to agree) we have just decided that "Remind me later" should no longer look at the number of attachments (see IRC log in comment 78), because effectively there's a high potential for annoying users when we just secretly switch off the reminder after user added just one more attachment. And to track "one more attachment" when user activates manual reminder when some attachments are already there, we would need all the "counting-attachments" code back into place which we've just eliminated because mkmelin himself expressed his preference for simpler code without counting attachments :) > And we already have the aggressive pref for those who always want to be > asked. I suggest that when the aggressive pref is set, the check is never > cleared automatically. That's a whole new story again which imo will add complexity and unpredictability in behaviour, and it's not easy because the aggressive pref is not even exposed. I also doubt if that works in line with bwinton's verdict. > Thinking about it, it would seem inconsistent to behave differently with the > menu option compared to the button in the reminder bar. They should imo give > the same behavior. +1. That is why in patch v0.1, we have made the behaviour consistent: [Remind me later] will always behave exactly the same way, no matter if triggered from notification bar or menu option. Workflow for the *manual* reminder: - user deliberately activates [Remind me later] from either notification bar or menu (thus declaring his explicit wish to be reminded later when sending!) - user may or may not add/remove attachments before sending (we don't care, because as bwinton says, we don't know how many attachments will satisfy the user's intention when he activated the reminder; automatically switching off the reminder after just one more attachment is like secretly switching off someone's clock alarm without telling him); user is free to deactivate the reminder when done - when user sends the message (and reminder has not been manually deactivated) -> we will show reminder alert (as explicitly opted-in by user; regardless of number of attachments) Pls note that the *word-based* aggressive reminder behaviour (when you ignore notification bar) is not affected or changed by this in any way: For users who did NOT opt in by activating [Remind me later], they will only see the alert if upon sending, they still have no attachments, reminder keywords found in body, and notification was not dismissed. So indeed (as Magnus says), there's just a tiny little bit which is missing from patch v0.1 (attachment 816261 [details] [diff] [review]): We need an exit condition for the *manual* reminder to avoid a reminder alert loop (send -> alert -> add attachments -> send -> alert again). Proposed solution: When we have shown the reminder alert once, and the alert gets dismissed (by clicking "Oh, I did!" OR [x]/[ESC]) -> deactivate manual reminder (remove checkmark from [Remind me later]). That looks acceptable even for the case of closing the alert with [x]/[ESC] because the reminder alert was actually shown, so if user ignores that and just dismisses the alert, we could assume the manual reminder job is done. As seen in patch v0.2, but without the unnecessary 3rd button, and without changing the button captions. So I think the following will make the behaviour consistent, simple, and predictable, in line with bwinton's idea of comment 78 (and mostly according narratives of comment 77): - use patch v0.1 as basis (attachment 816261 [details] [diff] [review]) - add only the following few lines of code from patch v0.2 (attachment 818074 [details] [diff] [review]): if (hadForgotten) + { + if (hadForgotten == 1) + { + gManualAttachmentReminder = false; + document.getElementById("cmd_remindLater").setAttribute("checked", "false"); + } return; + } After that, I suggest that we all play with that and see how it feels. You'll find that the behaviour is really simple and straightforward, because activating [Remind me later] will mean just that (always and predictably with no surprises): Remind me later (via alert)! Thanks.

Short summary of comment 90: This bug seeks to improve the functionality of the *manual* attachment reminder, aka [Remind me later]: independent of magic words in body, and independent of current number of attachments. Like an alarm clock, the resulting behaviour should be straightforward, predictable, safe, and simple (ON or OFF). 1) We can safely assume that users who deliberately click [Remind me later] from notification or menu actually WANT to be reminded (to prevent them from forgetting to add one or more attachments) 2) As bwinton says, given that we don't know how many and which attachments they want to be reminded about, we should not secretely switch off the reminder automatically just because any one more attachment was added. 3) Instead, before sending, we should just remind those users via alert anyway, per bwinton's guiding verdict (see comment 78): > Don't remove the check unless manually unchecked, because we have no way of telling what the user > meant when they checked the box, and guessing wrong is going to annoy them a lot. 4) As mkmelin says (comment 88), behaviour needs to be consistent for [Remind me later] from notification bar OR menu option. So we need to stop guessing for [Remind me later] from notification bar, too, and just show the alert at send time (without looking at attachments added). 5) As an exception to "TB should never deactivate the manual reminder", we do need an exit condition for the manual reminder *after* showing the alert, to avoid users getting into an alert loop. When user dismisses the alert (via "Oh, I did" or [x]/[ESC]), we can assume the manual reminder job done, hence deactivate it at that point (or just send the message if requested via [No, Send now]). (That's even consistent with notification bar behaviour where after dismissing it via [x] you will not get alerted upon sending). 6) For users who just ignore the word-based notification bar, we are not changing the behaviour; conditions for showing the /unrequested/ "aggressive" alert have not changed (you'll only see that if the notification bar is still visible when sending, which implies: no attachments, magic words in body, and notfication not dismissed).

(In reply to Thomas D. (away till 23rd Oct) from comment #91) > 5) For users who just ignore the word-based notification bar, That's 6), you see counting things always gets us into trouble :)

Thanks Thomas for saving me a lot of words and I extend my apologies for not describing my intention and work in patch v0.2 I had discussion with bwinton about this on IRC, here goes the log: http://logbot.glob.com.au/?c=mozilla%23maildev&s=16+Oct+2013&e=16+Oct+2013#c106720 20:31 sshagarwal bwinton: ping 20:31 bwinton sshagarwal: pong… 20:32 sshagarwal bwinton: hi, for bug 521158 20:32 firebot Bug https://bugzilla.mozilla.org/show_bug.cgi?id=521158 enh, --, ---, syshagarwal, ASSI, Implement menu option for "Remind me later" to add attachment before sending (allow activating and c 20:32 sshagarwal the "Attachment Reminder" dialog that comes at the time of sending the mail if the reminder was set 20:33 sshagarwal offers two possibilities, either we select [oh I did] or we select [no, send now] or we simply close the dialog 20:33 sshagarwal current scenario doesn't remove the check from the reminder option whatever the user chooses on this "attachment reminder" dialog 20:34 sshagarwal so, if the user chooses [oh I did] or closes the dialog (by either the escape key or [x]) and irrespective of whether he/she adds the attachments or not, because the check from the manual reminder isn't cleared manually 20:34 sshagarwal the user will be again prompted 20:35 bwinton Right, so if they've asked to be prompted, and we prompt them, then we probably shouldn't prompt them again… 20:35 bwinton Unless they ask again. 20:35 sshagarwal to avoid this, we came to a solution that if the user clicks on [oh I did] we should clear the check from the "remind me later" option 20:36 sshagarwal the dialog is a confirmEx prompt so, because of bug 345067, the return value of [oh I did] and [x] is same 20:37 sshagarwal so, either we need to write a custom dialog, or fix this bug before proceeding, or 20:37 sshagarwal we can have 3 buttons on the dialog 20:38 sshagarwal [no send now] [close the dialog] [oh I did] 20:38 sshagarwal so that we can differentiate the return values 20:38 bwinton Ugh. I'm not a giant fan of the three buttons, at least, not with the second one labelled the way it is. 20:38 sshagarwal bwinton: this awaits your decision :) 20:39 sshagarwal oh :( 20:39 sshagarwal so what to do then? 20:39 sshagarwal okay, if the user closes the dialog by either the escape key or the cross [x] button, should we send the mail directly as we would do in case of [no send now]? Is this acceptable? 20:39 bwinton Well, what about labelling the second button "Remind me again later", or something like that? 20:40 bwinton We shouldn't take an action (like sending the email) when the user dismisses a dialog, so that's not a great solution. 20:41 sshagarwal okay, so I just need three different return values from a confirmEx dialog, so whatever you suggest will be followed :) 20:41 bwinton Heh. So, can I get a screenshot of the dialog with three buttons, and the middle one saying "Remind me later"? 20:42 sshagarwal okay, just a minute :) 20:43 sshagarwal bwinton: <it is the same screenshot that I have uploaded on Bugzilla> 20:44 bwinton That seems reasonable. I think we could probably come up with some better wording, but for now it's okay. 20:44 sshagarwal okay, thanks :) 20:45 bwinton I kind of like something like "Not now, Thunderbird", to keep in the tone of the other two buttons, but I think that's a little too informal… 20:46 sshagarwal so, we can go with informal messages :) that's one more thing Open Source is known for :) 21:23 sshagarwal bwinton: also the current default value is "Send anyway" should I change it to "Add attachment" ? as this is attachment reminder 21:24 bwinton Well, on the one hand, we shouldn't interrupt the user too much. On the other hand, if they've asked us to interrupt them, then we should. 21:24 sshagarwal so which hand should I go this time? 21:24 bwinton On the third hand, if they hit <escape>, then we don't send the email, so providing a single-key to just send seems like the right thing to do. 21:25 sshagarwal okay, so letting it default to "Send anyway" and do you have a better message for "Edit email" button? that doesn't seem to fit I think 21:27 bwinton "Edit email" was the best we could come up with. 21:27 bwinton "Edit message" might be better… 21:28 sshagarwal oh okay :) 21:28 sshagarwal I thought we were editing email anyway so maybe something like getting back to it would suit 21:29 bwinton Yeah, we thought about that, but couldn't come up with anything decent… 21:30 sshagarwal okay so finalizing this for review 21:30 sshagarwal bwinton: thanks :) So, I think all the queries in the above comments are answered in this conversation. And yes I misplaced the buttons, re-positioning them will fix the problem expected in comment 89. I am just uploading the current patch's screenshot, if you have any other ideas, please let me know. Thanks.

So we'd have two buttons to do the same thing? Reasonable both add attachment and edit message should bring you back to the compose window. I don't like it. (Let's keep bug 516957 separate.)

Okay so this is the agreed version till date. Please let me know if further any changes need to be made. Thanks.

Comment on attachment 818699 [details] [diff] [review] Patch v0.1 reloaded Review of attachment 818699 [details] [diff] [review]: ----------------------------------------------------------------- Great! Suyash is so bravely facing all our challenges ;) This patch provides the expected simple and consistent behaviour described in comment 91 and agreed upon in subsequent IRC discussion involving bwinton and others, as a minimally intrusive change avoiding overload of this bug. So I just have some nits for polishing... (and I'm starting to enjoy real reviewing ;) And one general question: Do we need to verify that preserving manual reminder status in the msg source survives correctly with and without recycled compose windows? mail.compose.max_recycled_windows;1 (default) mail.compose.max_recycled_windows;0 (perhaps we should test this?) ::: mail/components/compose/content/MsgComposeCommands.js @@ +1728,5 @@ > label: getComposeBundle().getString("remindLaterButton"), > callback: function (aNotificationBar, aButton) > { > + document.getElementById("cmd_remindLater").setAttribute("checked", "true"); > + gManualAttachmentReminder = true; Pls swap these two lines for consistency and easier readinig. @@ +2350,5 @@ > var subject = GetMsgSubjectElement().value; > msgCompFields.subject = subject; > Attachments2CompFields(msgCompFields); > + gMsgCompose.compFields.attachmentReminder = > + (document.getElementById("cmd_remindLater").getAttribute("checked") == "true"); At the time we get here, boolean gManualAttachmentReminder variable is already set, and the "boolean" checked attribute is just an UI reflection of that. So I think this could be rewritten to be simpler and run faster (pls verify if it works): gMsgCompose.compFields.attachmentReminder = gManualAttachmentReminder Is this code executed when saving drafts? Is this code also executed when sending? If yes, will the recipient still see those mime fields which we added to the draft source for internal use? (I hope not) How does this behave for "Send later" (Ctrl+Shift+Enter from composition)? @@ +2409,5 @@ > > + // Attachment Reminder: Alert the user if > + // - the user requested "Remind me later" from either the notification bar or the menu > + // (alert regardless of the number of attachments already attached: we can't > + // guess the intended number, and guessing wrong will annoy users a lot), OR Thanks for tweaking the comments! Given that this was one of the main points of discussion, let's make this a bit more precise again for posterity (pls copy & paste to catch all the tweaks! Just swap the two affected comment lines) + // (alert regardless of the number of files already attached: we can't guess for how many + // or which files users want the reminder, and guessing wrong will annoy them a lot), OR @@ +2427,5 @@ > null, null, {value:0}); > if (hadForgotten) > + { > + gManualAttachmentReminder = false; > + document.getElementById("cmd_remindLater").setAttribute("checked", "false"); Great! This gets us out of the alert loop when user clicks on [Oh, I did!] or [x]/[ESC]. However, what happens if user clicks [No, send now], and the mail size is big, and user cancels sending? Manual Attachment Reminder would be still active, which is undesired because user has already decided to send without adding more attachments. To cover that edge case, I think we have to move these two lines forward a bit. Basically, after showing the alert at send time, we just switch gManualAttachmentReminder to false no matter what. Let's also tell posterity why we do that. + // Deactivate manual attachment reminder after showing the alert to avoid alert loop. + // We also deactivate reminder when user ignores alert with [x] or [ESC], as an + // acceptable and intentional workaround for bug 345067. + gManualAttachmentReminder = false; + document.getElementById("cmd_remindLater").setAttribute("checked", "false"); if (hadForgotten) return; ::: mail/components/compose/content/messengercompose.xul @@ +105,5 @@ > <command id="cmd_attachFile" oncommand="goDoCommand('cmd_attachFile')"/> > <command id="cmd_attachCloud" oncommand="attachToCloud(event.target.cloudProvider); event.stopPropagation();"/> > <command id="cmd_attachPage" oncommand="goDoCommand('cmd_attachPage')"/> > <command id="cmd_attachVCard" checked="false" oncommand="ToggleAttachVCard(event.target)"/> > + <command id="cmd_remindLater" checked="false" oncommand="toggleAttachmentReminder()"/> I'm not sure, but I think our style guidelines require a concluding semicolon (Magnus, pls correct me if I'm wrong): oncommand="toggleAttachmentReminder();"/> @@ +450,5 @@ > </menu> > <menuitem label="&attachPageCmd.label;" accesskey="&attachPageCmd.accesskey;" command="cmd_attachPage"/> > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator/> New UI elements should have an ID to enable addons to customize them. We typically add IDs for menuseparators. <menuseparator id="menu_Attach_remindLaterSeparator/> See below for explanation of how this ID was constructed. @@ +451,5 @@ > <menuitem label="&attachPageCmd.label;" accesskey="&attachPageCmd.accesskey;" command="cmd_attachPage"/> > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator/> > + <menuitem id="remindLater_menu" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" I think this ID doesn't provide enough context (remind about what?) and might be misleading because it's not a menu, it's a menuitem. I suggest the following ID: <menuitem id="menu_Attach_remindLater" Thunderbird code is very inconsistent about IDs, but I understand that ideally, IDs for a nested element should be constructed like this: parentID + elementID + [elementType] parentID = "menu_Attach" (This provides context, because we can have the same menuitem in different locations. So here it's File > Attach > menu with id="menu_Attach" and it's important to indicate that.) elementID = "RemindLater" (the name we give this particular element) [elementType] = "MenuItem" (optionally, add element type at the end to avoid ambiguities between commands, keys, menuitems etc; not needed here). Also found at the beginning of IDs, especially for non-nested elements ;) @@ +722,5 @@ > type="checkbox" > label="&attachVCardCmd.label;" > accesskey="&attachVCardCmd.accesskey;" > command="cmd_attachVCard"/> > + <menuseparator/> Pls add an ID here: <menuseparator id="button-attachPopup_remindLaterSeparator/> ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd @@ +28,5 @@ > <!--LOCALIZATION NOTE attachVCardCmd.label Don't translate the term 'vCard' --> > <!ENTITY attachVCardCmd.label "Personal Card (vCard)"> > <!ENTITY attachVCardCmd.accesskey "P"> > +<!ENTITY remindLater.label "Remind Me Later"> > +<!ENTITY remindLater.accesskey "L"> Shouldn't this be the first letter wherever possible, to provide mnemonic value as in "R" for "Remind me later"? R is free, or not? Any famous addons (Lightning essentially) that we'd be breaking here? If not, pls use: <!ENTITY remindLater.accesskey "R"> Using "R" would also be consistent with access key "R" used on [_Remind me later] button on notification bar. They won't conflict because access keys are context sensitive: Alt+R -> [_Remind me later] (on notification bar) Alt+F, t, r -> _Remind me later (on file menu) ::: mailnews/compose/public/nsIMsgCompFields.idl @@ +6,5 @@ > #include "nsISupports.idl" > #include "nsIMsgAttachment.idl" > #include "nsISimpleEnumerator.idl" > > +[scriptable, uuid(4f803e7a-5bd5-4344-9901-7a9ee89be780)] Has it been confirmed that we need to change this uuid for basically just adding a new variable without breaking existing stuff?

Comment on attachment 818699 [details] [diff] [review] Patch v0.1 reloaded Review of attachment 818699 [details] [diff] [review]: ----------------------------------------------------------------- And another nit: Pls make Bug summary *on the patch header* slightly shorter and more informative ;) Bug 521158 - Implement menu option for "Remind me later" to allow checking status and using manual Attachment Reminder independent of keyword magic and number of attachments

Well, usually the checkin message in the patch matches the summary of the bug, where some bug summaries are made 3-4 lines long to provide more information for easier searching (but which doesn't make them easier to read). In such cases I'd usually skip the informative part added in parentheses, which in this case makes it really short but should be sufficient. If the patch name was more descriptive than "Patch v0.1 reloaded" you could add it after the shortened non-() bug summary. Let's also keep in mind that people can always go back to the bug report as necessary to get the full story. So, bottom-line, the current commit message mostly matches the bug title and as such is ok. If you want to change it as suggested in comment #98, which isn't shorter anyway, then also please change the bug title respectively to keep things consistent.

(In reply to rsx11m from comment #99) I won't fight about the checkin message... ;) Suffice to say that I think I've digged myself deep enough into the internals of this bug that I'd claim to know what's important to know about it. It's more than just implementing a menu option for an already existing feature, we've extended and improved the entire behaviour of "Remind me later" aka manual Attachment Reminder (vs. word-based attachment reminder notification bar which appears automatically). My impression was that long checkin messages are unwanted, whereas longer bug summaries are sometimes required on bugs as you explained. So I've seen many cases where checkin message was entirely different from bug summary, often rewritten and not just omission of detail. In this case, I'd claim the details form an important part of the bug. > suggested in comment #98, which isn't shorter anyway Fwiw, the checkin message I suggested in comment 99 is exactly 7 characters shorter than the current checkin message, which isn't much, but it's still "slightly" shorter as I said, in spite of containing /more/ information... ;)

(In reply to Thomas D. (away till 23rd Oct) from comment #100) > (In reply to rsx11m from comment #99) > > suggested in comment #98, which isn't shorter anyway > Fwiw, the checkin message I suggested in comment 99 is exactly 7 characters shorter than the current typo: comment 98 Pls avoid changing the summary of this bug as it describes pretty well what this bug is about. I should know because I filed it and contributed a lot to getting this where we are now, in close cooperation with everyone involved...

(In reply to Thomas D. (away till 23rd Oct) from comment #100) > My impression was that long checkin messages are unwanted, whereas longer > bug summaries are sometimes required on bugs as you explained. So I've seen > many cases where checkin message was entirely different from bug summary, > often rewritten and not just omission of detail. In this case, I'd claim the > details form an important part of the bug. AFAIK, a bug summary is a description of the problem or of the enhancement (the WHAT). The checkin message is a description of the solution (the HOW) or of the code change. Sometimes they may be the same (as in this bug) other times completely different (e.g. bug: TB is crashing when sending mail; checkin: add NULL check of arguments in nsIMsgComposer::onSendMail to prevent a crash).

(In reply to :aceman from comment #102) > (In reply to Thomas D. (away till 23rd Oct) from comment #100) > AFAIK, a bug summary is a description of the problem or of the enhancement > (the WHAT). The checkin message is a description of the solution (the HOW) > or of the code change. Sometimes they may be the same (as in this bug) I'm perfectly fine with using the full bug summary as checkin message. Current checkin message omits some of bug summary.

I'm writing up documentation for this feature: https://support.mozillamessaging.com/en-US/kb/attachment-reminder/history#preview

Comment on attachment 818699 [details] [diff] [review] Patch v0.1 reloaded Review of attachment 818699 [details] [diff] [review]: ----------------------------------------------------------------- This seems to work as the guys want now ;) To the mail.compose.max_recycled_windows problem, I'd say that if it is 0, all the widgets should be properly initialized. So no carrying over of the state from one composition to another. I have tested that for the default value of 1, no carrying over is done. If first message has the reminder set, the next composition does not have it (until set). Carrying of the reminder state via the message source also works fine.

Comment on attachment 818699 [details] [diff] [review] Patch v0.1 reloaded Review of attachment 818699 [details] [diff] [review]: ----------------------------------------------------------------- Looks like nsMsgCompose.cpp bitrotted. Could you also include Thomas's comments in the new patch? ::: mailnews/compose/public/nsIMsgCompFields.idl @@ +6,5 @@ > #include "nsISupports.idl" > #include "nsIMsgAttachment.idl" > #include "nsISimpleEnumerator.idl" > > +[scriptable, uuid(4f803e7a-5bd5-4344-9901-7a9ee89be780)] Someone correct me if i'm, but it may not strictly speaking be necessary to rev the uuid if you add a variable. But I don't think it hurts either, and best to update anyway to be entirely sure.

Okay so I have made almost all the changes as suggested. And yes, I have changed the uuid as some said it is needed and others were not sure. And ThomasD, thanks for almost making this perfect but please don't suggest me the exact things to be done unless I am completely lost, it sort of kills my thinking and programming part :) Thanks.

(In reply to Suyash Agarwal (:sshagarwal) from comment #107) > please don't suggest > me the exact things to be done unless I am completely lost, it sort of kills > my thinking and programming part :) Sorry, this doesn't sound like what I wanted to say, so rephrasing: I wanted to thank you for giving this bug a concrete direction so that we don't get lost in different directions and this bug takes longer to fix. I also wanted to thank you for guiding me so well on this :) I just wanted to give myself a chance to try the things before you reveal the line no. to me :) I again sincerely apologize if my previous comment conveys some unwanted/incorrect message. Thanks :)

Nit: There's an inconsistency in the id's I suggested, sorry: We have (correctly imo, consistent with historically grown surroundings): + <menuseparator id="button-attachPopup_remindLaterSeparator"/> + <menuitem id="button-attachPopup_remindLaterItem" + <menuseparator id="menu_Attach_remindLaterSeparator"/> The ID of this one doesn't match that pattern, and imo we should be a bit more consistent between the two identical menuitems we introduce: + <menuitem id="menu_Attach_remindLater" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" Suyash, pls think about it and make a suggestion ;)

> Suyash, pls think about it and make a suggestion ;) :D I didn't mean that. > + <menuseparator id="menu_Attach_remindLaterSeparator"/> > > The ID of this one doesn't match that pattern, and imo we should be a bit > more consistent between the two identical menuitems we introduce: > > + <menuitem id="menu_Attach_remindLater" type="checkbox" > label="&remindLater.label;" accesskey="&remindLater.accesskey;" Either we remove the id as the surrounding menu items don't have it, or we can do something like this: + <menuseparator id="menu_AttachRemindLaterSeparator"/> + <menuitem id="menu_AttachRemindLater" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" + command="cmd_remindLater"/> As is with the new message and contact menu item in the File menu.

Comment on attachment 819431 [details] [diff] [review] Patch v0.3 Review of attachment 819431 [details] [diff] [review]: ----------------------------------------------------------------- Thank you Suyash, this looks very good! :) I found a minor nit which drove me to have another deep look at this, and, inspired by your earlier code here and elsewhere, I kept experimenting until things started to fall into place nicely with a few tweaks in code... Thanks to Archaeopteryx who helped my debug my test.xul... ;) f+ = me with those improvements (if I got this right and others agree) FTR: I did not look at the C++ code. ::: mail/components/compose/content/MsgComposeCommands.js @@ +1728,5 @@ > label: getComposeBundle().getString("remindLaterButton"), > callback: function (aNotificationBar, aButton) > { > + gManualAttachmentReminder = true; > + document.getElementById("cmd_remindLater").setAttribute("checked", "true"); This is when we click the "Remind me later" *Button* from notification bar. IF toggling gMsgCompose.compFields.attachmentReminder is required/wanted in the toggleAttachmentReminder() function (probably yes), then it is needed here, too: gMsgCompose.compFields.attachmentReminder = true And, thinking about it a bit more, we could then just replace all of these 3 lines with just calling toggleAttachmentReminder(true) if we tweak that function a little to accept optional boolean parameter. @@ +1988,5 @@ > document.getElementById("cmd_attachVCard") > .setAttribute("checked", gMsgCompose.compFields.attachVCard); > + document.getElementById("cmd_remindLater") > + .setAttribute("checked", gMsgCompose.compFields.attachmentReminder); > + gManualAttachmentReminder = gMsgCompose.compFields.attachmentReminder; For better reading, and perhaps minimally better performance, I'd swap this line around with the previous two, and set the command's checked attribute to gManualAttachmentReminder (rather than going into compFields twice). I suppose/hope it has been verified that when we get here via ComposeStartup, .compFields.attachmentReminder will always already have been initialized before! @@ +2094,5 @@ > > gAutoSaveKickedIn = false; > } > > +function toggleAttachmentReminder() Suyash, default parameters which you used e.g. in bug 900541 would come in handy here, too, for maximum flexibility, re-usability, and elegance of code :) (gManualAttachmentReminder has been initialized early on, and we keep it updated here, so it's always available): function toggleAttachmentReminder(showReminder = !gManualAttachmentReminder) @@ +2097,5 @@ > > +function toggleAttachmentReminder() > +{ > + let showReminder = (document.getElementById("cmd_remindLater") > + .getAttribute("checked") == "false"); These two lines no longer needed, now in default parameter. @@ +2100,5 @@ > + let showReminder = (document.getElementById("cmd_remindLater") > + .getAttribute("checked") == "false"); > + gManualAttachmentReminder = showReminder; > + document.getElementById("cmd_remindLater").setAttribute("checked", showReminder); > + gMsgCompose.compFields.attachmentReminder = showReminder; I suspect that toggling compFields.attachmentReminder is not technically required here because we set that again in GenericSendMessage() function, but otoh I'd always prefer to set correct values on everything as early as possible in case somebody looks at those values, so I think it's good :) We could consider removing that line from GenericSendMessage... @@ +2350,5 @@ > var subject = GetMsgSubjectElement().value; > msgCompFields.subject = subject; > Attachments2CompFields(msgCompFields); > + gMsgCompose.compFields > + .attachmentReminder = gManualAttachmentReminder; Pls simplify this line as seen in the previous lines. Setting .attachmentReminder here might not technically be necessary IF we always toggle .attachmentReminder immediately (in our toggle function) whenever we toggle the manual reminder, but it doesn't seem to hurt here either so it might be ok. If in doubt, I'd remove it here (to speed up sending which is slow anyway) and keep it in toggleAttachmentReminder(). @@ +2427,5 @@ > null, null, {value:0}); > + // Deactivate manual attachment reminder after showing the alert to avoid alert loop. > + // We also deactivate reminder when user ignores alert with [x] or [ESC]. > + gManualAttachmentReminder = false; > + document.getElementById("cmd_remindLater").setAttribute("checked", "false"); Again, if it's required/wanted to toggle compFields.attachmentReminder in our toggleAttachmentReminder() function, then we need to toggle it here, too. And fortunately, we can streamline these 3 away by exploiting the new boolean flexibility of toggleAttachmentReminder(showreminder...) and call that with false parameter here. ::: mail/components/compose/content/messengercompose.xul @@ +451,5 @@ > <menuitem label="&attachPageCmd.label;" accesskey="&attachPageCmd.accesskey;" command="cmd_attachPage"/> > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator id="menu_Attach_remindLaterSeparator"/> > + <menuitem id="menu_Attach_remindLater" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" Needs id change per comment 109, for internal naming consistency between the two menuitems we add: <menuitem id="menu_Attach_remindLaterItem"### I'd keep the double underscores for clarity and consistency, but we'll never be fully consistent as ID patterns differ all over Thunderbird.

For starting points in code, see patch in Bug 526998 where we implemented F2 keyboard shortcut.

(In reply to Thomas D. (away till 23rd Oct) from comment #112) > For starting points in code, see patch in Bug 526998 where we implemented F2 > keyboard shortcut. Sorry, that comment ran into the wrong bug, intended for Bug 929037.

Okay so I have implemented a few of the suggestions. I can't make toggleAttachmentReminder() usable everywhere because it is called from the messengercompose.xul file, so I don't know how to pass variable argument(s) from there. I have the setting of the message headers flag only in the GenericSendMessage as that covers everything. If I am supposed to make any further changes or what I assumed above isn't appropriate, please let me know. Thanks.

Comment on attachment 821503 [details] [diff] [review] Patch v0.8 Review of attachment 821503 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +1729,5 @@ > callback: function (aNotificationBar, aButton) > { > + gManualAttachmentReminder = true; > + document.getElementById("cmd_remindLater") > + .setAttribute("checked", "true"); So you have now merged the manual reminder in the menu with the "remind me later" button click when a keyword is found? Interesting. So then do you still need the gManualAttachmentReminder variable? @@ +2428,5 @@ > null, null, {value:0}); > + // Deactivate manual attachment reminder after showing the alert to avoid alert loop. > + // We also deactivate reminder when user ignores alert with [x] or [ESC]. > + gManualAttachmentReminder = false; > + document.getElementById("cmd_remindLater").setAttribute("checked", "false"); You seem to always set gManualAttachmentReminder and document.getElementById("cmd_remindLater").setAttribute("checked") to the same value. Can we use toggleAttachmentReminder() for it? Let it take a boolean argument. If that argument is not set, use the "toggle" logic. If it is set to some value, use that value to set the variable and menuitem state. (I simply do not want to hardcode the element ID to so many places, when we can have a common accessor function. ::: mail/components/compose/content/messengercompose.xul @@ +452,5 @@ > <menuseparator/> > <menuitem type="checkbox" label="&attachVCardCmd.label;" accesskey="&attachVCardCmd.accesskey;" command="cmd_attachVCard"/> > + <menuseparator id="menu_Attach_RemindLaterSeparator"/> > + <menuitem id="menu_Attach_RemindLaterItem" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" > + command="cmd_remindLater"/> Where did we loose "autocheck" ? @@ +727,5 @@ > + <menuitem id="button-attachPopup_remindLaterItem" > + type="checkbox" > + label="&remindLater.label;" > + accesskey="&remindLater.accesskey;" > + command="cmd_remindLater"/> Where did we loose "autocheck" ?

(In reply to :aceman from comment #115) > Comment on attachment 821503 [details] [diff] [review] > Patch v0.8 > Review of attachment 821503 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/components/compose/content/MsgComposeCommands.js > @@ +1729,5 @@ > > callback: function (aNotificationBar, aButton) > > { > > + gManualAttachmentReminder = true; > > + document.getElementById("cmd_remindLater") > > + .setAttribute("checked", "true"); > > So you have now merged the manual reminder in the menu with the "remind me > later" button click when a keyword is found? Interesting. Yes, we merged because it's required for behavioural ux-consistency as explained in my comment 91, 4). > So then do you > still need the gManualAttachmentReminder variable? Yes, for simpler, better readable, and faster code. Without the variable, we'd have to call... document.getElementById("cmd_remindLater").getAttribute("checked") ...wherever we now have the variable. Correct me if I'm wrong, but not only does that look awful and is much harder to read, but I'd also assume that getElement...getAttribute performs slower than just reusing the global variable (and I believe it gets called a lot in the word-checking routines). > @@ +2428,5 @@ > > null, null, {value:0}); > > + // Deactivate manual attachment reminder after showing the alert to avoid alert loop. > > + // We also deactivate reminder when user ignores alert with [x] or [ESC]. > > + gManualAttachmentReminder = false; > > + document.getElementById("cmd_remindLater").setAttribute("checked", "false"); > > You seem to always set gManualAttachmentReminder and > document.getElementById("cmd_remindLater").setAttribute("checked") to the > same value. Can we use toggleAttachmentReminder() for it? Let it take a > boolean argument. If that argument is not set, use the "toggle" logic. If it > is set to some value, use that value to set the variable and menuitem state. > (I simply do not want to hardcode the element ID to so many places, when we > can have a common accessor function. That's exactly what I already proposed and explained in detail in my review of comment 111. > > + <menuitem id="menu_Attach_RemindLaterItem" type="checkbox" label="&remindLater.label;" accesskey="&remindLater.accesskey;" > > + command="cmd_remindLater"/> > > Where did we loose "autocheck" ? Probably because of Magnus comment 58, where he misleadingly assumed that > i don't think commands even have a autocheck attribute Looking at https://developer.mozilla.org/en-US/docs/XUL/command#Attributes, indeed there's no /autocheck/ attribute on the list, but then there's no /checked/ attribute either and yet we are already using that. The truth is that you can use *any* attribute on commands for the purpose of *attribute forwarding*, as documented here: https://developer.mozilla.org/en-US/docs/XUL/Tutorial/Broadcasters_and_Observers#Command_Attribute_Forwarding Judging from autocheck documentation... https://developer.mozilla.org/en-US/docs/XUL/Attribute/autoCheck ...we'd probably want to set it to false. But my impression is that it's not needed because we made the menuitem observe cmd_RemindLater and if I observed my test.xul correctly, as soon as you have a command handler in place it will no longer autocheck (somewhat contrary to above documentation claim that it defaults to true). So I think both ways will work, with or without autocheck=false. (And fwiw, we can't rely on autocheck=true because there are situations where we change the checkmark from outside the menuitem/command. So we actually control the checkmark from code always, which looks like a safe thing to do).

ya, autocheck went long ago :) So, here are two variants, one with the gManualAttachmentReminder and the other without. Tell me which one do you like :) This is variant 1 (without the variable)

This is variant 2 with the gManualAttachmentReminder.

(In reply to Thomas D. (away till 23rd Oct) from comment #116) > But my impression is that it's not needed because we made the menuitem > observe cmd_RemindLater and if I observed my test.xul correctly, as soon as > you have a command handler in place it will no longer autocheck (somewhat > contrary to above documentation claim that it defaults to true). > So I think both ways will work, with or without autocheck=false. > (And fwiw, we can't rely on autocheck=true because there are situations > where we change the checkmark from outside the menuitem/command. So we > actually control the checkmark from code always, which looks like a safe > thing to do). I mean autocheck=false and on the "menuitem"s not "command".

Comment on attachment 821553 [details] [diff] [review] Patch variant2 Review of attachment 821553 [details] [diff] [review]: ----------------------------------------------------------------- OK, this one is optically nicer, I just fear that the variable and the menuitem checkbox may get desynchronized by something. ::: mail/components/compose/content/MsgComposeCommands.js @@ +122,5 @@ > gSendDefaultCharset = null; > gCharsetTitle = null; > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > gHideMenus = false; > + gManualAttachmentReminder = false; toggleAttachmentReminder(false) to be sure? @@ +2091,5 @@ > > gAutoSaveKickedIn = false; > } > > +function toggleAttachmentReminder(param) Better name, e.g. "aState" and document the function and argument in a comment above with the usual format. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +1249,5 @@ > > bool attachVCard = false; > m_compFields->GetAttachVCard(&attachVCard); > + bool attachmentReminder = false; > + m_compFields->GetAttachmentReminder(&attachmentReminder); Where is this variable used again? Do I miss something?

Comment on attachment 821547 [details] [diff] [review] patch variant1 (without gManualAttachmentReminder variable) Per aceman's comment 120, and my comment 116, we really want the keep the variable for readability, versatility, and performance, so this patch variant is obsolete.

Comment on attachment 821553 [details] [diff] [review] Patch variant2 Review of attachment 821553 [details] [diff] [review]: ----------------------------------------------------------------- We're getting there... ;) This patch still needs to address one or two review comments to simplify the code per my comment 111. I'll add them again here. FTR: I did NOT review the c++ parts of this patch. ::: mail/components/compose/content/MsgComposeCommands.js @@ +122,5 @@ > gSendDefaultCharset = null; > gCharsetTitle = null; > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > gHideMenus = false; > + gManualAttachmentReminder = false; Not needed, we already do that a bit later in composeStartup function, where we correctly get aState from compFields: > toggleAttachmentReminder(gMsgCompose.compFields.attachmentReminder); @@ +1985,5 @@ > document.getElementById("dsnMenu") > .setAttribute("checked", gMsgCompose.compFields.DSN); > document.getElementById("cmd_attachVCard") > .setAttribute("checked", gMsgCompose.compFields.attachVCard); > + toggleAttachmentReminder(gMsgCompose.compFields.attachmentReminder); I understand this code runs for these 2 scenarios: 1) virgin composition (no draft (auto-)saved yet, so no saved user-set value for .attachmentReminder yet), or 2) re-opened composition (from physical draft msg, having user-set value for .attachmentReminder in source which we use to persist reminder status) I was told by Suyash that for virgin composition scenario 1), by the time we get here, gMsgCompose.compFields.attachmentReminder is already set to false by default. That's good, although I still don't fully understand how (maybe someone can enlighten me). @@ +2091,5 @@ > > gAutoSaveKickedIn = false; > } > > +function toggleAttachmentReminder(param) +1. Something didn't think of, thanks Aceman. @@ +2099,5 @@ > + showReminder = param; > + } else { > + showReminder = (document.getElementById("cmd_remindLater") > + .getAttribute("checked") == "false"); > + } As explained in my previous review (comment 111), all of this complexity (let showreminder and the entire if/else structure) is not needed as we can just condense the whole function into only 2 lines, like this, using default parameter: function toggleAttachmentReminder(aState = !gManualAttachmentReminder) { gManualAttachmentReminder = aState; document.getElementById("cmd_remindLater").setAttribute("checked", aState); } @@ +2353,5 @@ > var subject = GetMsgSubjectElement().value; > msgCompFields.subject = subject; > Attachments2CompFields(msgCompFields); > + gMsgCompose.compFields > + .attachmentReminder = gManualAttachmentReminder; msgCompFields.attachmentReminder = ...

Scrap my previous review, I fell for a design flaw in splinter review. When I composed the review, I quoted some of Aceman's review, and the quotes were actually shown in the review preview, but they get lost when publishing. I'll add the missing bits here: (In reply to Thomas D. (away till 23rd Oct) from comment #122) > Comment on attachment 821553 [details] [diff] [review] > Patch variant2 > > Review of attachment 821553 [details] [diff] [review]: > ----------------------------------------------------------------- > > We're getting there... ;) > > This patch still needs to address one or two review comments to simplify the > code per my comment 111. > I'll add them again here. > > FTR: I did NOT review the c++ parts of this patch. > > ::: mail/components/compose/content/MsgComposeCommands.js > @@ +122,5 @@ > > gSendDefaultCharset = null; > > gCharsetTitle = null; > > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > > gHideMenus = false; > > + gManualAttachmentReminder = false; > > (In reply to :aceman from comment #120) > > toggleAttachmentReminder(false) to be sure? > > Not needed, we already do that a bit later in composeStartup function, where > we correctly get aState from compFields: > > > toggleAttachmentReminder(gMsgCompose.compFields.attachmentReminder); > > @@ +1985,5 @@ > > document.getElementById("dsnMenu") > > .setAttribute("checked", gMsgCompose.compFields.DSN); > > document.getElementById("cmd_attachVCard") > > .setAttribute("checked", gMsgCompose.compFields.attachVCard); > > + toggleAttachmentReminder(gMsgCompose.compFields.attachmentReminder); > > I understand this code runs for these 2 scenarios: > 1) virgin composition (no draft (auto-)saved yet, so no saved user-set value > for .attachmentReminder yet), or > 2) re-opened composition (from physical draft msg, having user-set value for > .attachmentReminder in source which we use to persist reminder status) > > I was told by Suyash that for virgin composition scenario 1), by the time we > get here, gMsgCompose.compFields.attachmentReminder is already set to false > by default. That's good, although I still don't fully understand how (maybe > someone can enlighten me). > > @@ +2091,5 @@ > > > > gAutoSaveKickedIn = false; > > } > > > > +function toggleAttachmentReminder(param) > > > aceman: Better name, e.g. "aState" and document the function and argument in a > > comment above with the usual format. > > +1. Something didn't think of, thanks Aceman. > > @@ +2099,5 @@ > > + showReminder = param; > > + } else { > > + showReminder = (document.getElementById("cmd_remindLater") > > + .getAttribute("checked") == "false"); > > + } > > As explained in my previous review (comment 111), all of this complexity > (let showreminder and the entire if/else structure) is not needed as we can > just condense the whole function into only 2 lines, like this, using default > parameter: > > function toggleAttachmentReminder(aState = !gManualAttachmentReminder) > { > gManualAttachmentReminder = aState; > document.getElementById("cmd_remindLater").setAttribute("checked", aState); > } > > @@ +2353,5 @@ > > var subject = GetMsgSubjectElement().value; > > msgCompFields.subject = subject; > > Attachments2CompFields(msgCompFields); > > + gMsgCompose.compFields > > + .attachmentReminder = gManualAttachmentReminder; > > msgCompFields.attachmentReminder = ...

(In reply to :aceman from comment #120) > Comment on attachment 821553 [details] [diff] [review] > Patch variant2 > > +function toggleAttachmentReminder(param) > Better name, e.g. "aState" and document the function and argument in a > comment above with the usual format. Aceman, I wasn't sure how to indicate in the function comment that the argument is optional. Is this acceptable? Is there a style guide that describes this? /** * Toggles or sets the status of manual Attachment Reminder, i.e. whether user will * get the "Attachment Reminder" alert before sending or not. Toggles checkmark on * "Remind me later" menuitem and internal gManualAttachmentReminder flag accordingly. * * @param aState (optional) true = activate reminder. false = deactivate reminder. * (omitted) = toggle reminder state */ function toggleAttachmentReminder(aState)

(In reply to Thomas D. (away till 23rd Oct) from comment #124) > function toggleAttachmentReminder(aState) Oops. Of course, that should be: function toggleAttachmentReminder(aState = !gManualAttachmentReminder)

(In reply to Thomas D. (away till 23rd Oct) from comment #125) > (In reply to Thomas D. (away till 23rd Oct) from comment #124) > > function toggleAttachmentReminder(aState) > > Oops. Of course, that should be: > > function toggleAttachmentReminder(aState = !gManualAttachmentReminder) I am not sure this works (is the default value allowed to be non-constant?), but I haven't tried it. But even if it does not work, we could shorten it to let showReminder = (aState != undefined) ? aState : !gManualAttachmentReminder

(In reply to Thomas D. (away till 23rd Oct) from comment #124) > Aceman, I wasn't sure how to indicate in the function comment that the > argument is optional. Is this acceptable? Is there a style guide that > describes this? > > /** > * Toggles or sets the status of manual Attachment Reminder, i.e. whether > user will > * get the "Attachment Reminder" alert before sending or not. Toggles > checkmark on > * "Remind me later" menuitem and internal gManualAttachmentReminder flag > accordingly. > * > * @param aState (optional) true = activate reminder. false = deactivate > reminder. > * (omitted) = toggle reminder state > */ > function toggleAttachmentReminder(aState) Yes this looks very good. I am not sure where to put the "optional" keyword but the comment is readable as is. mkmelin will tell if the format is right.

(In reply to :aceman from comment #126) Aceman, thanks for rapid response :) > > (In reply to Thomas D. from comment #124) > > function toggleAttachmentReminder(aState = !gManualAttachmentReminder) > > I am not sure this works (is the default value allowed to be non-constant?), > but I haven't tried it. But even if it does not work, we could shorten it to > let showReminder = (aState != undefined) ? aState : > !gManualAttachmentReminder That's not shorter, it's additional code which is neither required nor helpful in any way (readability, performance, etc.). I have tested my condensed function (see below) in live XUL, so I know that it works as expected without any problems. Setting default parameter like that [1] explicitly covers the undefined case. We always pass non-constant variables as parameters into functions, so no problem to set a non-constant as default value for the parameter here, it's just a newer and better notation. I don't see why we should make the code more complex than necessary. (In reply to Thomas D. from comment #123) > As explained in my previous review (comment 111), all of this complexity > (let showreminder and the entire if/else structure) is not needed as we can > just condense the whole function into only 2 lines, like this, using default > parameter: > > function toggleAttachmentReminder(aState = !gManualAttachmentReminder) > { > gManualAttachmentReminder = aState; > document.getElementById("cmd_remindLater").setAttribute("checked", aState); > } [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/default_parameters

> function toggleAttachmentReminder(aState = !gManualAttachmentReminder) From the documentation (1): > The default argument gets evaluated at call time, so unlike e.g. in Python, a new Object > is created each time the function is called. This even applies to functions and variables. (1) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/default_parameters

Ok then, thanks for the pointer. We just knew it would not work in C so we were not sure it works in JS. So then we can use it.

Magnus, is this the right way of documenting *optional* function arguments in function comment? Any style guide for that? /* ... * @param aState (optional) true = activate reminder. false = deactivate reminder. * (omitted) = toggle reminder state */ function toggleAttachmentReminder(aState = !gManualAttachmentReminder)

Comment on attachment 822381 [details] [diff] [review] Patch v0.9 Review of attachment 822381 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +122,5 @@ > gSendDefaultCharset = null; > gCharsetTitle = null; > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > gHideMenus = false; > + gManualAttachmentReminder = false; Me and Thomas both commented on this line (even if in conflicting ways). And it is still here unchanged. So what is the decision? My reasoning to keep this here (and change to toggleAttachmentReminder()) is that the function is about initializing variables. It does not matter that we set it later on in ComposeStartup(). There will still be a small time window where the variable is not initialized. Yes, it may be a bit slower but more robust. Who knows if after later changes some code will not look at the variable in the small window? Just keep the intention of the InitializeGlobalVariables function. @@ +2094,5 @@ > > +/** > + * Toggles or sets the status of manual Attachment Reminder, i.e. whether > + * the user will get the "Attachment Reminder" alert before sending or not. > + * Toggles checkmark on "Remind me later" menuitem and internal Trailing space. @@ +2097,5 @@ > + * the user will get the "Attachment Reminder" alert before sending or not. > + * Toggles checkmark on "Remind me later" menuitem and internal > + * gManualAttachmentReminder flag accordingly. > + * @param aState (optional) true = activate reminder, false = deactivate > + * reminder, (omitted) = toggle reminder state. Why did you not format the @param line as Thomas suggested? Also, there should be a blank comment line (" *") before @param line. See around the file how function documentation comments are formatted. ::: mailnews/compose/src/nsMsgCompose.cpp @@ +1249,5 @@ > > bool attachVCard = false; > m_compFields->GetAttachVCard(&attachVCard); > + bool attachmentReminder = false; > + m_compFields->GetAttachmentReminder(&attachmentReminder); I had a question here. Where is the answer?

Comment on attachment 822381 [details] [diff] [review] Patch v0.9 Review of attachment 822381 [details] [diff] [review]: ----------------------------------------------------------------- Perfect! Ready to roll... Can't wait to see this in action... f+ = me with 2 whitespace nits and 1 other nit fixed FTR: I don't know enough C++ to check that, but it also looks good :) ::: mail/components/compose/content/MsgComposeCommands.js @@ +2094,5 @@ > > +/** > + * Toggles or sets the status of manual Attachment Reminder, i.e. whether > + * the user will get the "Attachment Reminder" alert before sending or not. > + * Toggles checkmark on "Remind me later" menuitem and internal trailing whitespace @@ +2097,5 @@ > + * the user will get the "Attachment Reminder" alert before sending or not. > + * Toggles checkmark on "Remind me later" menuitem and internal > + * gManualAttachmentReminder flag accordingly. > + * @param aState (optional) true = activate reminder, false = deactivate > + * reminder, (omitted) = toggle reminder state. Pls distribute and indent these lines as follows, for improved readability (few more characters line length in comments is less important than readability): * @param aState (optional) true = activate reminder. false = deactivate reminder. * (omitted) = toggle reminder state @@ +2104,5 @@ > +{ > + gManualAttachmentReminder = aState; > + document.getElementById("cmd_remindLater") > + .setAttribute("checked", aState); > +} > function toggleAttachmentReminder(aState = !gManualAttachmentReminder) beautiful, isn't it? ::: mailnews/compose/src/nsMsgCompUtils.cpp @@ +457,5 @@ > PUSH_STRING("DSN=1"); > else > PUSH_STRING("DSN=0"); > PUSH_STRING("; "); > + PUSH_STRING("uuencode=0; "); I think we should be consistent with previous lines here, pls keep line 461 and just add PUSH_STRING("; ");

(In reply to :aceman from comment #120) > ::: mail/components/compose/content/MsgComposeCommands.js > @@ +122,5 @@ > > gSendDefaultCharset = null; > > gCharsetTitle = null; > > gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager); > > gHideMenus = false; > > + gManualAttachmentReminder = false; > > toggleAttachmentReminder(false) to be sure? I don't think that is needed, we already are setting checkbox=false from the xul file also calling the function maybe an overhead (that's what I think) if that isn't the case please let me know, I'll put the call here. > ::: mailnews/compose/src/nsMsgCompose.cpp > @@ +1249,5 @@ > > > > bool attachVCard = false; > > m_compFields->GetAttachVCard(&attachVCard); > > + bool attachmentReminder = false; > > + m_compFields->GetAttachmentReminder(&attachmentReminder); > > Where is this variable used again? Do I miss something? I don't know about that, maybe this call is useless, I just did the behavior similar to that of attachVCard.

(In reply to Suyash Agarwal (:sshagarwal) from comment #135) > (In reply to :aceman from comment #120) > > ::: mail/components/compose/content/MsgComposeCommands.js > > @@ +122,5 @@ > > > + gManualAttachmentReminder = false; > > > > toggleAttachmentReminder(false) to be sure? > I don't think that is needed, we already are setting checkbox=false from the > xul file also calling the function maybe an overhead (that's what I think) > if that isn't the case please let me know, I'll put the call here. Are you sure the checkbox=false is reread from the XUL file when the compose window is recycled and shown again for a new message? I am not, but mkmelin will know more. > > ::: mailnews/compose/src/nsMsgCompose.cpp > > @@ +1249,5 @@ > > > > > > bool attachVCard = false; > > > m_compFields->GetAttachVCard(&attachVCard); > > > + bool attachmentReminder = false; > > > + m_compFields->GetAttachmentReminder(&attachmentReminder); > > > > Where is this variable used again? Do I miss something? > I don't know about that, maybe this call is useless, I just did the behavior > similar to that of attachVCard. With the exception that attachVCard value is used later on, while yours isn't...

(In reply to :aceman from comment #136) > > > toggleAttachmentReminder(false) to be sure? > > I don't think that is needed, we already are setting checkbox=false from the > > xul file also calling the function maybe an overhead (that's what I think) > > if that isn't the case please let me know, I'll put the call here. > Are you sure the checkbox=false is reread from the XUL file when the compose > window is recycled and shown again for a new message? I am not, but mkmelin > will know more. Maybe, but it is being recycled for me. > > > > + m_compFields->GetAttachmentReminder(&attachmentReminder); > > > Where is this variable used again? Do I miss something? > > I don't know about that, maybe this call is useless, I just did the behavior > > similar to that of attachVCard. > With the exception that attachVCard value is used later on, while yours > isn't... Yes, it isn't required so I have removed it. (In reply to Thomas D. from comment #134) Sorry, I forgot those changes, made now.

Comment on attachment 822403 [details] [diff] [review] Patch Review of attachment 822403 [details] [diff] [review]: ----------------------------------------------------------------- I like how the patch is still shrinking :)

(In reply to :aceman from comment #138) > Comment on attachment 822403 [details] [diff] [review] > I like how the patch is still shrinking :) lol... me too :) But if we shrink it more than that, it will start to disappear ;)

(In reply to Suyash Agarwal (:sshagarwal) from comment #137) > Created attachment 822403 [details] [diff] [review] > Patch > > (In reply to :aceman from comment #136) > > > > toggleAttachmentReminder(false) to be sure? > > > I don't think that is needed, we already are setting checkbox=false from the > > > xul file also calling the function maybe an overhead (that's what I think) > > > if that isn't the case please let me know, I'll put the call here. > > Are you sure the checkbox=false is reread from the XUL file when the compose > > window is recycled and shown again for a new message? I am not, but mkmelin > > will know more. > Maybe, but it is being recycled for me. It's not even necessary to read from XUL because we still explicitly set the checkmark in composeStartup (based on user-defined value from gMsgCompose.compFields.attachmentReminder btw), so I don't see how this could go wrong. But what about this: Speaking of compFields, although Suyash has almost convinced me by saying "but we have gManualAttachmentReminder which always has the correct value, so they can read that if they want", I still feel uneasy that we don't immediately update gMsgCompose.compFields.attachmentReminder also whenever we toggle the reminder state, for the same reasons as aceman said: We never know which code might look at gMsgCompose.compFields.attachmentReminder at any given time, so it should be always up to date, and not only later when saving. So I'd prefer this line should go back into our toggleAttachmentReminder function (as in ToggleAttachVCard function!) - aceman, would you agree? > gMsgCompose.compFields.attachmentReminder = aState; If anything, I'd rather skip that line when in genericSendMsg when saving the draft if we're sure it's already set, but I'm neutral to keep it there to be more robust.

FYI: This is the flow of gManualAttachmentReminder variable, also showing where we set the checkmark initially: L72 (MsgComposeCommands.js header): var gManualAttachmentReminder; L126 (InitializeGlobalVariables): gManualAttachmentReminder = false; L1989 (ComposeStartup): toggleAttachmentReminder(gMsgCompose.compFields.attachmentReminder); which will set (in toggleAttachmentReminder, during ComposeStartup): L2105 gManualAttachmentReminder = aState; (i.e. now = compFields.attachmentReminder) L2106 document.getElementById("cmd_remindLater").setAttribute("checked", aState); [L2108] gMsgCompose.compFields.attachmentReminder = aState; (if re-introduced per my comment below)

(In reply to Thomas D. from comment #140) > But what about this: > So I'd prefer this line should go back into our > toggleAttachmentReminder function (as in ToggleAttachVCard function!) - > aceman, would you agree? > > > gMsgCompose.compFields.attachmentReminder = aState; > > If anything, I'd rather skip that line when in genericSendMsg when saving > the draft if we're sure it's already set, but I'm neutral to keep it there > to be more robust. I would do the same what we do for other gMsgCompose.compFields.* attributes. See if we set them in genericSendMsg() or whereever their associated feature is toggled. I just opt for consistency so that all variables and attributes are set on the same place.

(In reply to :aceman from comment #142) > I would do the same what we do for other gMsgCompose.compFields.* > attributes. See if we set them in genericSendMsg() or whereever their > associated feature is toggled. I just opt for consistency so that all > variables and attributes are set on the same place. The recipient(s), subject, attachment(s) etc. are being stored in the message header in this GenericSendMessage() function. So, I think we should place this reminder header too here itself.

(In reply to Suyash Agarwal (:sshagarwal) from comment #143) > The recipient(s), subject, attachment(s) etc. are being stored in the > message header in this GenericSendMessage() function. So, I think we should > place this reminder header too here itself. There is setting of the message header during the toggle as in ToggleReturnReceipt() but I still think this is a different case as attachment reminder is worthwhile only during the saving of the draft and if someone needs to check the status, gManualAttachmentReminder has the value so there is no point in increasing the overhead in the toggle function. But if you think this is needed during every toggle, please let me know. needinfo? mkmelin, aceman

(In reply to Suyash Agarwal (:sshagarwal) from comment #144) > (In reply to Suyash Agarwal (:sshagarwal) from comment #143) > > The recipient(s), subject, attachment(s) etc. are being stored in the > > message header in this GenericSendMessage() function. So, I think we should > > place this reminder header too here itself. > > There is setting of the message header during the toggle as in > ToggleReturnReceipt() but I still think this is a different case as > attachment reminder is worthwhile only during the saving of the draft and if > someone needs to check the status, gManualAttachmentReminder has the value > so there is no point in increasing the overhead in the toggle function. But > if you think this is needed during every toggle, please let me know. > > needinfo? mkmelin, aceman ToggleReturnReceipt(), ToggleDSN(), ToggleAttachVCard() all toggle their gMsgCompose.compFields immediately when toggled. Suyash will not be there to tell coders that they mustn't use gMsgCompose.compFields because they should use gManualAttachmentReminder instead. "Overhead" is minimal given that in most cases, users will only toggle the reminder *once* for every message. As aceman says, pls lets be consistent with how other variables work.

(In reply to Thomas D. from comment #145) > ToggleReturnReceipt(), ToggleDSN(), ToggleAttachVCard() all toggle their > gMsgCompose.compFields immediately when toggled. Looking at the structural/functional similarity between these functions and our toggleAttachmentReminder function... - all are *small* helper functions - all change compFields to preserve their status in msg source - all toggle checkboxes in the respective menus ...I think our toggleAttachmentReminder function should be near these friends in the code. So unless there are better reasons not to (speak now or be forever silent), can we pls move toggleAttachmentReminder function to this line (above ToggleReturnReceipt function): http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2982 (In current patch, we're hiding it somewhat randomly between a rather unsimilar bigger function - ComposeStartup - and a rather unrelated bigger variable - var gMsgEditorCreationObserver -, starting from this line: http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2094)

Okay, making the suggested change(s), I think the job is done now and I hope there are no further suggestions to be incorporated :) Still, if any suggestions are left, please let me know. Thanks.

Comment on attachment 822864 [details] [diff] [review] Final Patch Review of attachment 822864 [details] [diff] [review]: ----------------------------------------------------------------- Perfect! :)

(In reply to Thomas D. from comment #131) > Magnus, is this the right way of documenting *optional* function arguments > in function comment? > Any style guide for that? > > /* ... > * @param aState (optional) true = activate reminder. false = deactivate > reminder. > * (omitted) = toggle reminder state > */ > function toggleAttachmentReminder(aState = !gManualAttachmentReminder) yes, that looks like the correct way to do it.

Comment on attachment 822864 [details] [diff] [review] Final Patch Review of attachment 822864 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +3147,5 @@ > + * gManualAttachmentReminder flag accordingly. > + * > + * @param aState (optional) true = activate reminder. > + * false = deactivate reminder. > + * (omitted) = toggle reminder state. though (omitted) shoudl probably read (default) ::: mailnews/compose/public/nsIMsgCompFields.idl @@ +43,5 @@ > attribute boolean forcePlainText; > attribute boolean useMultipartAlternative; > attribute boolean bodyIsAsciiOnly; > attribute boolean forceMsgEncoding; > + /// Preserve status of manually-activated attachment reminder. Remove "Preserve"

Comment on attachment 822864 [details] [diff] [review] Final Patch Review of attachment 822864 [details] [diff] [review]: ----------------------------------------------------------------- A final nit (i hope!) ;) ::: mail/components/compose/content/MsgComposeCommands.js @@ +2457,5 @@ > Recipients2CompFields(msgCompFields); > var subject = GetMsgSubjectElement().value; > msgCompFields.subject = subject; > Attachments2CompFields(msgCompFields); > + msgCompFields.attachmentReminder = gManualAttachmentReminder; Suyash, given that compFields.attachmentReminder will now always be set correctly and updated instantly from inside the toggleAttachmentReminder() function (which is the preferred location per aceman's and my comments, to keep it up to date instantly, in analogy to similar toggle functions), is there any purpose or benefit of setting the same value here *again* (unlike attachVCard, ReturnReceipt, DSN), or are we just wasting a bit of performance in GenericSendMessage which already performs a lot of stuff? If it's just duplication, pls remove this line (and we can assume Magnus agrees with this if he doesn't complain ;) (It's true that some other compFields like subject, recipients and attachment are updated *only* here in GenericSendMessage, but our .attachmentReminder is more similar to those other fields like AttachVCard which update immediately (in their own toggle functions), and we are not here to change that, there might even be a reason...).

(In reply to Thomas D. from comment #145) > (In reply to Suyash Agarwal (:sshagarwal) from comment #144) > > I still think this is a different case as > > attachment reminder is worthwhile only during the saving of the draft and if > > someone needs to check the status, gManualAttachmentReminder has the value > > so there is no point in increasing the overhead in the toggle function. > > ToggleReturnReceipt(), ToggleDSN(), ToggleAttachVCard() all toggle their > gMsgCompose.compFields immediately when toggled. Suyash will not be there to > tell coders that they mustn't use gMsgCompose.compFields because they should > use gManualAttachmentReminder instead. > "Overhead" is minimal given that in most cases, users will only toggle the > reminder *once* for every message. (In reply to Thomas D. from comment #151) > Comment on attachment 822864 [details] [diff] [review] > Final Patch > > + msgCompFields.attachmentReminder = gManualAttachmentReminder; > > Suyash, given that compFields.attachmentReminder will now always be set > correctly and updated instantly from inside the toggleAttachmentReminder() > there any purpose or benefit of setting the same value here *again* (unlike > attachVCard, ReturnReceipt, DSN), or are we just wasting a bit of > performance in GenericSendMessage which already performs a lot of stuff? Now these are two conflicting statements :) And obviously this line isn't wrong both on logical as well as performance grounds. So believe me, allow me to leave this as is :) The programmers will also have a track of all the headers being finalized here.

(In reply to Suyash Agarwal (:sshagarwal) from comment #152) > (In reply to Thomas D. from comment #151) > > Comment on attachment 822864 [details] [diff] [review] > > Final Patch > > > + msgCompFields.attachmentReminder = gManualAttachmentReminder; > > > > Suyash, given that compFields.attachmentReminder will now always be set > > correctly and updated instantly from inside the toggleAttachmentReminder() > > there any purpose or benefit of setting the same value here *again* (unlike > > attachVCard, ReturnReceipt, DSN), or are we just wasting a bit of > > performance in GenericSendMessage which already performs a lot of stuff? > > Now these are two conflicting statements :) > And obviously this line isn't wrong both on logical as well as performance > grounds. So believe me, allow me to leave this as is :) The programmers will > also have a track of all the headers being finalized here. I didn't buy that, so we agreed to remove this line from genericSendMsg (because we really keep compFields.attachmentReminder always and instantly in sync with gManualAttachmentReminder in the toggleAttachmentReminder function, even for the default value at the beginning, via calling toggleAttachmentReminder in ComposeStartup). But I agree with Suyash' point that it would be nice for coders to keep track of things in genericSendMsg, and it's confusing because some compFields are updated immediately in toggle functions (like attachVCard etc., and attachmentReminder), while others are not and only get updated in genericSendMsg (like subject, recipients and attachments). So I think we should add a short comment in genericSendMsg instead (where we remove the line), to point that out for posterity: 2338 msgCompFields.subject = subject; 2339 Attachments2CompFields(msgCompFields); 2340 // Some other msgCompFields have already been updated instantly in their respective // toggle functions, e.g. ToggleReturnReceipt(), ToggleDSN(), ToggleAttachVCard(), // and toggleAttachmentReminder().

Comment on attachment 822864 [details] [diff] [review] Final Patch Blake, this is now ready and awaits your final blessing :) (In reply to :bwinton on IRC, http://logbot.glob.com.au/?c=mozilla%23maildev&s=10+Oct+2013&e=13+Oct+2013#c105435) > Don't remove the check unless manually unchecked, because we have no way of telling what the user > meant when they checked the box, and guessing wrong is going to annoy them a lot. Have a look at this beautifully efficient patch, which implements your nice tweak of removing some covert hence unexpected automagic behaviour, so as to make this a really straightforward manual attachment reminder in analogy to the "alarm clock" principle (either ON or OFF), which now does exactly what it says on the box, "Remind me later" (via alert, without IFs and BUTs or guesswork-counting of attachments). (In reply to bwinton on IRC, http://logbot.glob.com.au/?c=mozilla%23maildev&s=10+Oct+2013&e=28+Oct+2013#c107168) > ThomasD: That comment [comment 91] is okay by me, and seems to sum up the desired behaviour. The behaviour of this patch is (still) exactly as described and explained in comment 91. That's on top of the core idea of this bug to make "Remind me later" (aka Manual Attachment Reminder) available at any time during composition, independent of magic attachment keywords and the number of attachments already attached (which already has your ui-r+ in comment 59). (For bonus points, the patch even comes with performance gains in genericSendMessage() function because we removed duplicated code cycle of ShouldShowAttachmentNotification(), currently line 2401 of MsgComposeCommands.js[1], which unnecessarily traversed the whole message body again in the final conditions testing for the reminder alert). Suyash (patch author) bravely faced the challenge of navigating all the different expectations and ideas that were cooperatively developed and refined in the course of this bug, a job well done! Thanks to aceman and mkmelin, too! [1] http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js?rev=723f9a787538#2401

(In reply to Thomas D. from comment #154) > Comment on attachment 822864 [details] [diff] [review] > Final Patch > > (For bonus points, the patch even comes with performance gains in > genericSendMessage() function because we removed duplicated code cycle of > ShouldShowAttachmentNotification(), currently line 2401 of > MsgComposeCommands.js[1], which unnecessarily traversed the whole message > body again in the final conditions testing for the reminder alert). Oh, and we also make "Remind me later" aka Manual Attachment Reminder survive closing and re-opening of drafts, as we fix the following bugs with our patch here: Bug 919286 - Attachment reminder's "Remind me later" not persisted when saving, closing and re-opening/editing draft: No alert dialogue when sending Bug 926056 - Attachment reminder: User's explicit "Remind me later" request from notification not honoured if attachment keywords no longer in body: No alert dialogue when sending

Carrying over review from mkmelin.

Comment on attachment 823763 [details] [diff] [review] Final Patch + suggested nits Review of attachment 823763 [details] [diff] [review]: ----------------------------------------------------------------- Great :) ::: mail/components/compose/content/messengercompose.xul @@ +738,5 @@ > + <menuitem id="button-attachPopup_remindLaterItem" > + type="checkbox" > + label="&remindLater.label;" > + accesskey="&remindLater.accesskey;" > + command="cmd_remindLater"/> Have you checked all the individual menus that the accesskey "R" is free to use in all of them?

Comment on attachment 823763 [details] [diff] [review] Final Patch + suggested nits Perfect & polished! :) Forwarding r=mkmelin from comment 150. I'll re-add bwinton's ui-r? flag to add the right pointers on his queue in next comment.

Comment on attachment 823763 [details] [diff] [review] Final Patch + suggested nits Blake, this well-polished patch is now ready and realizes the additional UX input you provided and discussed with us on IRC. We're all eager and excited to see this land. For your ui-review, pls refer to comment 154 and comment 155 which summarize your previous ui-r and subsequent UX input and what we've done here accordingly. (In reply to Blake Winton (:bwinton) from comment #59) > After playing around with this for a bit, and thinking through the various > scenarios, I'm pretty sure I like it! ui-r=me! r=mkmelin (comment 150) f=aceman, ThomasD

(In reply to :aceman from comment #157) > ::: mail/components/compose/content/messengercompose.xul > @@ +738,5 @@ > > + <menuitem id="button-attachPopup_remindLaterItem" > > + type="checkbox" > > + label="&remindLater.label;" > > + accesskey="&remindLater.accesskey;" > > + command="cmd_remindLater"/> > > Have you checked all the individual menus that the accesskey "R" is free to > use in all of them? Though there is no use in File menu, there are two usages in Edit menu, one in Options menu and one for headers. Can I use this? :)

(In reply to Suyash Agarwal (:sshagarwal) from comment #160) > (In reply to :aceman from comment #157) > > ::: mail/components/compose/content/messengercompose.xul > > @@ +738,5 @@ > > > + <menuitem id="button-attachPopup_remindLaterItem" > > > + type="checkbox" > > > + label="&remindLater.label;" > > > + accesskey="&remindLater.accesskey;" > > > + command="cmd_remindLater"/> > > > > Have you checked all the individual menus that the accesskey "R" is free to > > use in all of them? > > Though there is no use in File menu, there are two usages in Edit menu, one > in Options menu and one for headers. > > Can I use this? :) First, we need a more accurate understanding of accesskeys vs. shortcut keys... https://developer.mozilla.org/en-US/docs/XUL_Accesskey_FAQ_and_Policies (perhaps not completely up to date, and different customs might apply) Access keys inside the menus will only compete inside their own menu or submenu. So inside File menu and inside Edit menu are different stories, it doesn't matter. Access keys don't usually compete with shortcut keys, because we reserve Alt+... for access keys (shortcut keys usually with Ctrl). It's Alt+... for things exposed in focused window (like main menus, or buttons in main interface), but e.g. inside the menus, it's just the access key itself which will trigger the action. So it's Alt+F, t, F to attach a file. So in Edit menu, r for redo does certainly not compete with Ctrl+R, which is a shortcut key and only works in compose window, not in the menus. But two access keys which are on the same level will cause problems. Sometimes they will just focus the elements alternatingly, sometimes they don't work at all. See next comment.

(In reply to Suyash Agarwal (:sshagarwal) from comment #160) > (In reply to :aceman from comment #157) > > Have you checked all the individual menus that the accesskey "R" is free to > > use in all of them? > Can I use this? :) Actually no (so my recommendation was wrong), because all of us overlooked this one: Bug 550848 - Attachment reminder's "Remind me later" button and "From" field have the same access key (Alt+R) (keyboard shortcut)

(In reply to Thomas D. from comment #162) > Bug 550848 - Attachment reminder's "Remind me later" button and "From" field > have the same access key (Alt+R) (keyboard shortcut) So R is already taken by From:, and we can't really change that because F is File, O is Format, and M is Attachments (in attachment pane header). So we need to revert accesskey back to L, which is the only reasonable solution I can see. L for Later as in "Attach...Later" actually makes a pretty good mnemonic, and it does not seem taken by anything else. So we'll also fix bug 550848 while we are here.

(In reply to Thomas D. from comment #163) > So we'll also fix bug 550848 while we are here. So to be consistent, in addition to changing "our" access key for "Remind me later" menus to L, we also need to change the access key on the "Remind me later" button on notification bar: http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1847 > accessKey : getComposeBundle().getString("remindLaterButton.accessskey"), Let's fix the spelling here, three s is one too much. http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties#329 > remindLaterButton.accessskey=R Pls fix the spelling here, too, and change access key to L. Sorry for immediate prescriptive solutions, but I really want to get this bug done now, sooner rather than later.

Carrying over review from mkmelin and feedback from aceman and Thomas D. (Quoting Thomas D. from comment #159) > Comment on attachment 823763 [details] [diff] [review] > Final Patch + suggested nits > > Blake, this well-polished patch is now ready and realizes the additional UX > input you provided and discussed with us on IRC. We're all eager and excited > to see this land. > > For your ui-review, pls refer to comment 154 and comment 155 which summarize > your previous ui-r and subsequent UX input and what we've done here > accordingly. > > (In reply to Blake Winton (:bwinton) from comment #59) > > After playing around with this for a bit, and thinking through the various > > scenarios, I'm pretty sure I like it! ui-r=me! Though File menu's reminder accesskey has no conflicts, still changing it to 'L' for consistency. Thanks :)

(In reply to Suyash Agarwal (:sshagarwal) from comment #165) > Created attachment 824847 [details] [diff] [review] > Final Patch + more suggested nits remindLaterButton.accessskey (triple sss) still hasn't been fully fixed (1 spot), and we should correct the highly confusing misspelling of addAttachmentButton.accessskey (with triple sss), too (2 spots). I'll contribute some localization notes on these while we are here, but not today...

Carrying over review from mkmelin and feedback from aceman and Thomas D. (Quoting Thomas D. from comment #159) > Comment on attachment 823763 [details] [diff] [review] > Final Patch + suggested nits > > Blake, this well-polished patch is now ready and realizes the additional UX > input you provided and discussed with us on IRC. We're all eager and excited > to see this land. > > For your ui-review, pls refer to comment 154 and comment 155 which summarize > your previous ui-r and subsequent UX input and what we've done here > accordingly. > > (In reply to Blake Winton (:bwinton) from comment #59) > > After playing around with this for a bit, and thinking through the various > > scenarios, I'm pretty sure I like it! ui-r=me! Fixed the slipped in mistakes. Thanks :)

Comment on attachment 825270 [details] [diff] [review] Final Patch + regression Fix Okay, I think I like it! The one thing I couldn't check that I want to make sure works, is if we hit the "remind later" button, then save it as a draft. When we re-open the draft, and hit send, it should show the reminder. So, ui-r=me with that working. :) Thanks, Blake.

(In reply to Blake Winton (:bwinton) from comment #168) > Comment on attachment 825270 [details] [diff] [review] > If we hit the "remind later" button, then save it as a draft. > When we re-open the draft, and hit send, it should show the reminder. > > So, ui-r=me with that working. :) Ya, that's a part of the feature, thanks :)

Congratulations to Suyash for making the patch land after the heroic effort to go through the ~25 revisions of it! :) Somebody should now go through the dependent bugs 345067 550848 919286 926056 and ask the reporters to try the next nightly with this patch. It looks like some of the bugs could have been fixed by the patch here. Thomas, please mark this bug verified when you test the feature in tomorrow's nightly.

(In reply to :aceman from comment #171) > Congratulations to Suyash for making the patch land after the heroic effort > to go through the ~25 revisions of it! :) +1. Well done, Suyash, rapid response times, with learning curves for many of us as we refined this together. The net result is a beautifully efficient patch (see my thanks & praise in comment 154) which significantly streamlines, expands and exposes the previously covert and erratic attachment reminder feature. We've also fixed the following bugs in the same sweep: bug 550848, bug 919286, and bug 926056 (see comment 155). I especially like that attachment reminder status is now preserved in draft source, which allows for interesting usecases like saving a draft with active attachment reminder as a template... > Thomas, please mark this bug verified when you test the feature in > tomorrow's nightly. Done :) Behaves exactly as expected per comment 91, yippeeh! Finally!!! Thanks to everyone involved for their patient and constructive contributions... :)