244455 - Help prevent forgetting inclusion of attachments

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Christophe Porteneuve]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

(There's been an RFE in Mail/News for over 4 years, with bug <a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=35601">#35601</a>, that went
dead for over 2 years and has just recently got a couple additional comments.

Apparently, nobody's acted on it. Since Thunderbird is a different project
anyway--I don't believe Mail/News gets merged with TB at any point, unlike
Browser and Firefox--I open this bug here)

Tens of thousands of times a day, people start writing emails with the intention
of including attachments. They go "Hereattached is blah blah". By the time the
writing is done, they click the Send button. And then belatedly realize they
forgot to include the attachments.

So they have to go through the silly motions of sending *another* email, usually
along the lines of "d'oh! Sorry about this. Here it is.", with the attachments
included.

KMail has recently addressed this issue in a rather smart manner, by providing a
user-configurable dictionary (which could actually augment some l10n-maintained
basic dictionary) of terms to look for in the body text when the user attempts
to send the email.

If any of those flag up and no attachment is present, a dialog box comes up
saying KMail believes the user may have forgotten attaching files. The dialog
lets the user confirm immediate sending or sit back and add attachments.

This is, in my view, the best approach. However, it could be complemented by
Matthew Thomas' suggestion in <a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=35601">#35601</a> of putting
the Attach button right next to the Send button. I don't use buttons, I'm a
keyboard guy, but still, this would surely help some people.

This does not seem like a hard task, although it does require some additional
GUI in the preferences dialog's Attachment section (but not a complicated one!).
It should be coded before 1.0 is out! 

Note that ideally, it should be possible to use several dictionaries (for
instance, I email people in French, English and German. In French I'll typically
use terms such as "ci-joint", "ci-jointe", "ci-joints", "pièce jointe", "en
attachement", with possible starter caps. In English, "hereattached",
"attachment", etc.

Reproducible: Always
Steps to Reproduce:
No specific steps: just writing an email and sending it without attachments! It
works well, it's just not good enough! :-)

Comment 1

[Mike Cowperthwaite]

*** Bug 270138 has been marked as a duplicate of this bug. ***

Comment 2

[Matthias Jordan]

Just to copy over my remarks from my unintentionally created duplicate bug
report/feature request:

Why not additionally use a Bayesian filter to find out if the user probably
wanted to include an attachment just before sending the message? Training on
false negatives could occur post-mortem on saved outbound messages in Sent. 

Comment 3

[Mike Cowperthwaite]

*** Bug 282935 has been marked as a duplicate of this bug. ***

Comment 4

[Jo Hermans]

*** Bug 311856 has been marked as a duplicate of this bug. ***

Comment 5

[tyl2]

Obviously, the dialog doesn't have to appear when the user has attached a file.

Btw, this bug's summary could be "Remind user to include attachment when message
text contains words such as "attached."

Comment 6

[Jo Hermans]

*** Bug 339700 has been marked as a duplicate of this bug. ***

Comment 7

[David McClure]

There is an extension called withAttach that addresses this issue in some regard:

https://addons.mozilla.org/thunderbird/2199/

This extension adds a menu option (and a toolbar button) to compose an email "with attachments".

Also adds context menu options and toolbar buttons to "reply with attachment" and "reply all with attachment".


If you try to send the email without attachments, you are warned about it and asked to confirm your action.

Comment 8

[Jo Hermans]

*** Bug 351684 has been marked as a duplicate of this bug. ***

Comment 12

[Chinmay Patel]

I am willing to take this bug and I have started working in this bug. So far, I have found an extension which does exactly the same thing as required. Here is the extension https://addons.mozilla.org/en-US/thunderbird/addon/5759. 
Tell me if our requirements are something different than this extension.

Comment 13

[Jim Frame]

The extension seems to work fine, but it includes a "Please Donate" button and associated text makes the UI confusing.  If you can replicate the functionality without the donation stuff, I'd be happy.

Thanks.

Comment 14

[Magnus Melin [:mkmelin]]

Assigning to you then, Chinmay. 

clarkbw might be able to figure out what kind of UI we'd want for this.

Comment 15

[Chinmay Patel]

I have e-mailed clarkbw asking if he would like to share his ideas about the UI for the bug. At this point of time, I am just trying to make it as simple as possible. After some suggestions, I will make it better depending on what is needed.

Comment 16

[Chinmay Patel]

Attachment: Prompt Box

Here is the snapshot of the screen when confirm box has popped up. The box is reminding the user to attach a document if the keyword has found.

Comment 17

[Jim Frame]

You might want to consider rewording the prompt box text to something like the following:

"Thunderbird thinks you may be missing an attachment.  Press 'OK' to continue without attachments, or press 'Cancel' to return to the Compose window."

(Question:  the OK button text uses all caps.  Is it common to refer to an OK button using sentence case (i.e., 'Ok')?

Comment 18

[David Humphrey (:humph)]

> (Question:  the OK button text uses all caps.  Is it common to refer to an OK
> button using sentence case (i.e., 'Ok')?

Why not give them something more than just OK, perhaps, "Send Anyway" or "Send without Attachment" and "Cancel"?

Comment 19

[Chinmay Patel]

Attachment: Done: If the user enters the keyword, thunderbird will remind him to attach the file.

Comment 20

[Chinmay Patel]

Attachment: Only basic requirement is done.

Done: If the user enters the keyword, thunderbird will remind him to attach the file.

Remaining to be done: 
1. Table should be created at the time of the installation not runtime.
2. More default keywords to be added.
3. Localization of the feature.
4. Add GUI for preferences and allow user to add more keywords.

I need a large number of "attachment keywords" to be added to the table. I also need you to tell me if I am going on the wrong direction or our requirement is something different. Comments, suggestion and/or criticism are welcomed.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 347201 [details] [diff] [review]
Only basic requirement is done.

Just a quick drive by.

Using sqlite for this is way too much. All you need to do is add a localizablepref of comma separated words.

>+      var bucket = document.getElementById("attachmentBucket");

Move to close where you're using it.

>+
>+        if(bucket.itemCount == 0)
>+        {
>+          var keywordFound = false;
>+          var file = Components.classes["@mozilla.org/file/directory_service;1"]
>+                       .getService(Components.interfaces.nsIProperties)
>+                       .get("ProfD", Components.interfaces.nsIFile);

Please align as .classes
                .getService
                .get

... here and elsewhere.

You also have a lot of tabs which you should remove, we use spaces only in mozilla code. (Also remove trailing and other unnecessary whitespace from the lines you're adding/touching. Also keep lines to ~ 80 chars.

The prompt text could be much simpler. If you have to explain what the ok and cancel buttons do, don't use them but use verbs instead

--------------------------------------------
|
|  Did you forget to add an attachment?
|
|                [Oh, I did!] [No, Send Now]
--------------------------------------------

... or something :)

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Some quick comments as well.

Preferences:
The main pref will likely need to go in the Composer tab of the Thunderbird prefs.  We can use a checkbox to turn it on, with an adv. options button to open up the dialog.

+--------------------------------------------------------------------------+
|  ...                                                                     |
|  [X] Check for missing attachments                      ( Advanced... )  |
|  ...                                                                     |
+--------------------------------------------------------------------------+

It would be nice to use a list of terms instead of the comma separated values.  This would require ( + Add ), ( = Edit ), and ( - Remove ) buttons to manage the list.  However the comma separated text field is going to be much easier for someone to mistakenly break.

Warning Label:
I'm not sure if this is included in your patch, but warning labels aren't really good user experience.  To do an inline suggest we should be friendlier than a warning as well as being obviously actionable.  I did a quick design for the inline helper ( http://clarkbw.net/designs/clippy-helper/inline/ ) to the compose window.  This would likely end up adding an additional experience element where if a person canceled the helper we don't show the dialog.

Dialog:
Avoid the ( OK ) / ( Cancel ) as others have pointed out.  It is much better to use one line of text for the dialog and verbs in the buttons.  

I think our tone for the text should be friendly and helpful, not cold and precise; similarly for our button verbs.

"Did you forget to add an attachment?"

(( Add Attachment ))  ( No, Send Without )

Bonus Points could grab a snippet of text that we matched to indicate the need for this dialog.

"Did you forget to add an attachment?"
  "/...in the *attachment* you'll find.../"

(( Add Attachment ))  ( No, Send Without )

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to comment #22)
> the list.  However the comma separated text field is going to be much easier
> for someone to mistakenly break.

Just to clarify, whatever the UI looks like, you can still store the value of it as as comma separated text.

Comment 24

[Chinmay Patel]

Attachment: Mozilla Preferences and Spacing

Finally, I have made it work with preferences. I don't know why, It took me so long to understand Mozilla preference System. But, it's done now. Tell me if I have misinterpret you guys and there is something else which should be done to store CSV string.

Plus, please check if there are still Tabs in place of Spaces. 

I will address remaining issues in next patch.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 348395 [details] [diff] [review]
Mozilla Preferences and Spacing

I think basically it looks pretty good. Just a few minor comments:

>+// The Attachment Reminder Keywords
>+pref("mail.MsgCompose.attachmentKeywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties");

Usually prefs are mostly lowercase, i think something like mail.attachment_keywords would be better.

>+        if(bucket.itemCount == 0)
>+          {

Align the bracket.

>+            var keywordFound = false;
>+            var prefsBranch = Components.classes["@mozilla.org/preferences-service;1"].
>+                                        getService(Components.interfaces.nsIPrefService).getBranch(null);

Easier to use (and please align the dots for long lines, dot on new line):

var prefs = Components.classes["@mozilla.org/preferences-service;1"]
                      .getService(Components.interfaces.nsIPrefBranch);


>+            var len = keywordsArray.length;
>+            for(var i = 0; i < len  && keywordFound == false; i++)

You don't need the intermediary len, and we usually don't compare directly to false, but just check if it's falsy like |i < keywordsArray.length && !keywordFound|

>+              {
>+              var re = new RegExp(keywordsArray[i], "i");
>+              var OK = re.exec(mailData);
>+              if (OK)
>+                {
>+                  keywordFound = true;
>+                }
>+              }

... which will let you use keywordFound = re.exec(mailData) and no OK var. BTW, if the if clause only have one line, skip the brackets. 


Oh, and yes, you still have one tab, and at least two extra spaces;)

Comment 26

[Chinmay Patel]

Attachment: User can insert, edit and delete keywords.

Hopefully, I have addressed all the issues pointed out by Mr. Melin.

As, I described in my previous patch, this patch allows user to Insert, Edit and Delete keywords. 

However, I cannot figure it out that where should I to put a checkbox in Tools-->Options-->Attachments so user can turn this feature ON/OFF? If I try to put it there, it messes up the whole Options window.

Notify me if you find any mistakes in the code or UI or anywhere else. I would be happy to solve this issues.

Comment 27

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 351152 [details] [diff] [review]
User can insert, edit and delete keywords.

This looks pretty good so far, however the preferences need a little more work.

The pref needs to be in the Composition tab.  See the beginning of Comment #22 for the description.

The keyword chooser you did is great, we just need to move it to a dialog that appears when ( Advanced ... ) is clicked.

Comment 28

[Jose Sa]

Question: How are different languages being handled. If a user writes emails in several languages, then different keywords can be used to detect the missing attach. I guess that the simple way would be to add all words in all (used) languages.

Comment 29

[Chinmay Patel]

(In reply to comment #28)
Different languages are not handled yet! Making it work for all languages is my next goal. I just need to replace hard coded strings with user preference strings. This feature will let other developers convert this patch in different languages.

(In reply to comment #27)
Ya, that makes more sense. I have exams in next week. Hence, It may take couple of weeks. But, I will be back for sure :).

Thanks for all your support guys !!!

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 351152 [details] [diff] [review]
User can insert, edit and delete keywords.

Don't back out bug 207527 ;)

... and I don't see my earlier comments addressed (or only one of them).

Before submitting the next patch, take a look on the patch with an editor that shows you whitespace characters too. You have a lot of trailing/other extra white space, + some tabs.

>+mail.attachment_keywords=Attach,PFA,PDF,Doc,Document,Slide,Presentation,File,zip,rar,see

I think this list needs some work. I would expect it to contain
"Attachment", "Attached","file" - and possibly some more...

But certainly not "see" or "presentation". Dunno what "PFA" is supposed to be.

>\ No newline at end of file

Fix this, x2

Comment 31

[Chinmay Patel]

Attachment: Moved the option to Compose Tab + A New Window is Created.

Hey guys,

(In reply to comment #27)
First of all, I would like to sorry for taking such a long time to finish up this task. 
In the current patch, I have transfered the attachment reminder option in the compose menu as asked by Bryan W Clark (:clarkbw) in Comment#27. Plus, I have also moved the keyword chooser in a whole new window handled by an "advance" button.

(In reply to comment #30)
In addition with adding this window, I have tried my best to eliminate all unintentional white spaces throughout the patch. If they are still there, let me know so that I can eliminate them.
I know that I need a bit more keywords in the keyword list. However, this is just a demo list. Although I will finalize the keyword list in the final Patch, feel free to suggest some good words or any other resources.

I think, I have addressed all the required issues. Let me know, if I am missing any of the issues.

One more time, I would like to thank you for all your support getting into the open source community.

Thanks ... :)

Comment 32

[Bryan Clark (DevTools PM) [:clarkbw]]

Excellent!

I was so excited I built this in right away so I have a couple quick comments from my side of things.

* In the preferences the button should use the word "Advanced...", not Advance
* The pref should be on by default, at least mine wasn't on at first
* I'm not seeing a list of words in my advanced window - this could be a problem with my build and I'll check that
* I also wasn't able to insert new words into the list

But otherwise the changes from before are great, I think we're almost there.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 357914 [details] [diff] [review]
Moved the option to Compose Tab + A New Window is Created.

Thanks for working on it. However, please read my previous review comments. So far none of them have been addressed.

This patch doesn't apply to the latest hg. It also seems you forgot any words as default pref.

Some (more) things to fix, besides a whole lot of whitespace. Please check the generated patch in an editor that can display them.

>+//pref("mail.attachment_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties")
>\ No newline at end of file

^^^

>+        /*
>+         *Functionality Remaining to be added :
>+         *      1. Make a "Bottom Bar" which will remind the user to add attachment when user types KEYWORDS. (Need ideas to make it)
>+         */

Please get rid of this before the next patch.

>+        {
>+         
>+          var bundle = document.getElementById("bundle_composeMsgs");
>+          var keywordFound = false;
>+          var prefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefService).getBranch(null);
>+          var keywordsInCsv = prefsBranch.getComplexValue("mail.attachment_keywords",
>+                                      Components.interfaces.nsIPrefLocalizedString).data; 
>+          var keywordsArray = keywordsInCsv.split(",");

See earlier review. Comment #25.

>+
>+          var contentFrame = document.getElementById('content-frame');
>+          var mailBody = contentFrame.contentDocument.getElementsByTagName('body')[0];
>+          var mailBodyNode = mailBody.cloneNode(true);
>+          var mailData = mailBodyNode.innerHTML;
>+          for(var i=0; i<keywordsArray.length && !keywordFound; i++)
>+            {
>+            var re = new RegExp(keywordsArray[i], "i");
>+            var OK = re.exec(mailData);
>+            if (OK)
>+                keywordFound = true;
>+            }

This too. x2
>+# The Original Code is the Thunderbird Preferences System.
>+#
>+# The Initial Developer of the Original Code is
>+# Ben Goodger.
>+# Portions created by the Initial Developer are Copyright (C) 2005
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Scott MacGregor <[e-mail]>

Get a clean license boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/

>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+
>+var gAttachmentOptionsDialog = {
>+  
>+  mKeywordListBox:  null,
>+  mprefsBranch: null,
>+
>+  init: function ()
>+  {
>+    this.mKeywordListBox = document.getElementById('keywordList');
>+    this.buildKeywordList();
>+  },
>+  
>+  buildKeywordList: function()
>+  {
>+    mprefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                             .getService(Components.interfaces.nsIPrefService).getBranch(null);

Again, see comment 25.

>+    var keywordsInCsvNSI = mprefsBranch.getComplexValue("mail.attachment_keywords",

Strange var name? What's NSI???

>+    var input = {value: "Keyword"};
>+    var bool  = gPromptService.prompt(
>+                                 window,
>+                                 "Add Keyword",
>+                                 "Enter a Keyword",
>+                                 input,
>+                                 null,
>+                                 {value: 0});

Missing localizations, and please don't name the var "bool" ;)

>+    if(bool){
>+      var keywordsInCsvNSI = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                           Components.interfaces.nsIPrefLocalizedString); 
>+      keywordsInCsvNSI.data += "," + input.value;
>+      mprefsBranch.setComplexValue("mail.attachment_keywords",
>+                                 Components.interfaces.nsISupportsString, keywordsInCsvNSI);
>+      this.mKeywordListBox.appendItem(input.value, input.value);
>+    }
>+  },
>+  
>+  editKeyWord: function ()
>+  {
>+    var index = this.mKeywordListBox.selectedIndex;

No need for the tmp var here, just use it directly. Same thing later on.

>+    if (index >= 0)
>+    {
>+      var keywordElToEdit = this.mKeywordListBox.getItemAtIndex(index);
>+      gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);


Align at dots, comment 25.

>+      var input = {value: keywordElToEdit.getAttribute("value")};
>+      var bool  = gPromptService.prompt(
>+                                   window,
>+                                   "Edit Keyword",
>+                                   "Edit the Keyword",
>+                                   input,
>+                                   null,
>+                                   {value: 0});
>+      if(bool)
>+      {
>+        this.mKeywordListBox.removeItemAt(index);
>+        var  oldValue= keywordElToEdit.getAttribute("value");
>+        var keywordsInCsvNSI = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                            Components.interfaces.nsIPrefLocalizedString); 
>+    keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(oldValue , input.value);
>+    mprefsBranch.setComplexValue("mail.attachment_keywords",
>+                                Components.interfaces.nsISupportsString, keywordsInCsvNSI);

Strange indention, and alignment.

>+        this.mKeywordListBox.appendItem(input.value, input.value);
>+      }
>+    }
>+  },
>+  
>+  removeKeyWord: function ()
>+  {
>+    var index = this.mKeywordListBox.selectedIndex;
>+    if (index >= 0)
>+    {
>+      var keywordElToEdit = this.mKeywordListBox.getItemAtIndex(index);
>+      this.mKeywordListBox.removeItemAt(index);
>+      var  oldValue= keywordElToEdit.getAttribute("value");

Extra space after var

>+      var keywordsInCsvNSI = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                          Components.interfaces.nsIPrefLocalizedString); 
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(oldValue , "");
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(",," , ",");
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(/^,/, "");
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(/,$/, "");

Ugly, here a tmp var would be in order


>+          <hbox align="center">
>+            <checkbox id="attachment_reminder_label" flex="1" label="&attachmentReminder.label;"
>+                  preference="mail.compose.attachmet_reminder_checkbox"

Align preference with id, keep lines at 80 chars.
>+pref("spellchecker.dictionary", "@AB_CD@");
>\ No newline at end of file

Please fix ^^^


>+## Attachment Reminder
>+mail.attachment_keywords=Attach,PFA,PDF,Doc,Document,Slide,Presentation,File,zip,rar,see

I'd suggest just "attachment, attached" to start with.

Comment 34

[Magnus Melin [:mkmelin]]

> It also seems you forgot any words as default pref.

I noticed later, that wasn't the case. Sorry.

Comment 35

[Ben Bucksch (:BenB)]

Personally, I think this implementation as dialog box as proposed here, is like the "MS paperclip" that just annoys people.

I agree with the problem and basic idea, so I'd make the Send button an animated, jumping paperclip when the mouse gets near it.

From a security perspective, I am worried about throwing off a dialog box just because I wrote "I am quite attached to this girl". It's well-known that this just makes people ignore all dialogs of all types.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

BenB: If you think something inline like ( http://clarkbw.net/designs/clippy-helper/inline/clippy-helper-0.png ) is better.  Do you think you could help with some stubbed out code for Chinmay to play with?

Comment 37

[Ben Bucksch (:BenB)]

clarkbw, I personally don't think the info bar is that great a solution here, but definitely better than a dialog.
I don't have time to work on this, as I'm on the autoconfig bug (and even put paid work on the side for that).
I think my suggestion wouldn't be all that hard:
1. Create an animated "make attachment" version of the send button.
2. If you find the trigger word (you'll need to find it while the user is editing, just like inline spellchecking, that may be hard), you set an needattachment="true" attribute on the send button.
3. When the user mouses near the Send button, or simpler, on mouse over, you set another mouseisnear="true" attribute. CSS defines for that case (both attributes true) that the send button changes to the animated version.

Comment 38

[Phil Ringnalda (:philor)]

(In reply to comment #37)
> 3. When the user mouses near the Send button, or simpler, on mouse over

I suspect that people who send with ctrl+return, alt+f+d, or clicking the menuitem, also need a bit of a reminder.

Comment 39

[Ben Bucksch (:BenB)]

Do "force feedback" keyboards exist yet? Sorry, I'm silly.

Comment 40

[Chinmay Patel]

Attachment: patch-5

This bug is ready to find its place in Thunderbird ... :)

Errors resolved in the :
1. No more ">\ No newline at end of file"
2. No more White Spaces problems. I have almost eliminated all the unwanted white-spaces from the code (Hopefully).
3. I have given my best to indent the code as per my knowledge.
4. Got a clean boilerplate licence for my 2 new files. attachmentoptions.xul, attachmentoptions.js. Moreover, I found out that attachmentoptions.dtd doesn't need any kind of boilerplate licensing. If it does need a license, let me know the where to get it.
5. Taken care of localization throughout the code. I have converted 2 pointed out localization problem from the last code. Inform me, if I am missing any other.
6. Changed the variable name "bool" to "response".
7. Now, code is using "this.mKeywordListBox.selectedIndex" directly in place of any temp varables.
8. Replaced 

>+      var keywordsInCsvNSI = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                          Components.interfaces.nsIPrefLocalizedString); 
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(oldValue , "");
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(",," , ",");
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(/^,/, "");
>+      keywordsInCsvNSI.data = keywordsInCsvNSI.data.replace(/,$/, "");

with 

+      var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",

+                                                      Components.interfaces.nsIPrefLocalizedString);

+      keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , "")

+                                             .replace(",," , ",")

+                                             .replace(/^,/, "")

+                                             .replace(/,$/, "");

9. Replaced 

>+          <hbox align="center">
>+            <checkbox id="attachment_reminder_label" flex="1" label="&attachmentReminder.label;"
>+                  preference="mail.compose.attachmet_reminder_checkbox"

with 

+          <hbox align="center">

+            <checkbox id="attachment_reminder_label" 
+                      flex="1" 

+                      label="&attachmentReminder.label;"

+                      preference="mail.compose.attachmet_reminder_checkbox"

+                      accesskey="&attachmentReminder.accesskey;"/>

10. As Bryan Clark suggested, I have added the feature that whenever the user clicks on "Attach files" in the "Attachment Reminder" prompt box, thunderbird will open the Attachmet Oprtions Box. 

Finally, this bug is almost done. Although there is nothing left to do be done in the bug, let me know if I am missing anything. I would like to thank you one more time for all the support and patience that you guys have shown.

Comment 41

[Ben Bucksch (:BenB)]

+mail.attachment_keywords=attachment,attached,PFA,PDF,Doc,Document,Slide,Presentation,File,zip,rar,see

"attachment" is the only one you can use, at best. All the rest will cause way too many false positives, in normal English text ("attached", "see"), when sending URLs instead of attachments (which we should *encourage* instead of attachments for big files) etc..

Comment 42

[Chinmay Patel]

in reply to comment#41

I appreciate your help. However, this is not the finalized list. I am working on that list. If you would like to contribute to the list feel free to go to the page https://developer.mozilla.org/User:Chinmay/Attachment_Keyword_List: and edit the keyword list. 
Thanks.

Comment 43

[Ben Bucksch (:BenB)]

Ah, OK, thanks. Done, with a suggestion how to avoid URLs and still trigger on file endings.
(Comment 37 still stands, though.)

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 360490 [details] [diff] [review]
patch-5

After enabling it, I tried to send but just got a 

Error: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getComplexValue]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://messenger/content/preferences/attachmentoptions.js :: anonymous :: line 57"  data: no]
Source File: chrome://global/content/bindings/preferences.xml
Line: 738

Comment 45

[Chinmay Patel]

Attachment: patch-5 replacement

First of all, I would like to apologize for my mistake. In the previous patch, I forget to add 

>pref("mail.attachment_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties");

in mail/locales/en-US/all-l10n.js. As far as my knowledge is concerned, this line links the preference "mail.attachment_keywords" with it's value in "composeMsgs.properties" file. It will also create a default value for the preference. I have checked the patch by creating a new tree in a new computer. And the good thing is, it is working :). 

Moreover, read the comment#40 for the changes I have made in the patch. 
 
Thank you very much.

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 360668 [details] [diff] [review]
patch-5 replacement

First of all, the attachmentoptions.dtd,xul,js aren't included in the patch so it doesn't build. 

>diff --git a/mail/components/compose/content/MsgComposeCommands.js b/mail/components/compose/content/MsgComposeCommands.js
>--- a/mail/components/compose/content/MsgComposeCommands.js
>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -1663,16 +1663,63 @@ function GenericSendMessage( msgType )
>               msgCompFields.subject = result.value;
>               var subjectInputElem = GetMsgSubjectElement();
>               subjectInputElem.value = result.value;
>             }
>             else
>               return;
>           }
>         }
>+
>+        /*Atatchment Reminder*/

Misspelled, and use a "// " comment please. 

>+        var bucket = document.getElementById("attachmentBucket");
>+        var warn = getPref("mail.compose.attachmet_reminder_checkbox");
>+        if (warn && !bucket.itemCount)
>+        {
>+          var bundle = document.getElementById("bundle_composeMsgs");
>+          var keywordFound = false;
>+          var prefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefService)
>+                                      .getBranch(null);

See comment 25.

>+          var keywordsInCsv = prefsBranch.getComplexValue("mail.attachment_keywords",
>+                                             Components.interfaces.nsIPrefLocalizedString).data; 
>+          var keywordsArray = keywordsInCsv.split(",");
>+
>+          var contentFrame = document.getElementById('content-frame');
>+          var mailBody = contentFrame.contentDocument.getElementsByTagName('body')[0];
>+          var mailBodyNode = mailBody.cloneNode(true);
>+          var mailData = mailBodyNode.innerHTML;
>+          for (var i = 0; i < keywordsArray.length && !keywordFound; i++)
>+            {
>+            var re = new RegExp(keywordsArray[i], "i");
>+            var OK = re.exec(mailData);
>+            if (OK)
>+                keywordFound = true;
>+            }

See comment 25.

>+
>+          if (keywordFound)
>+          {
>+            var check = {value: false};
>+            var flags = gPromptService.BUTTON_POS_0 * gPromptService.BUTTON_TITLE_IS_STRING +
>+                        gPromptService.BUTTON_POS_1 * gPromptService.BUTTON_TITLE_IS_STRING; 
>+            var button = gPromptService.confirmEx(null,
>+                                                  bundle.getString("attachmentReminderTitle"),
>+                                                  bundle.getString("attachmentReminderMsg"),
>+                                                  flags,
>+                                                  bundle.getString("attachmentReminderOptionsYes"),
>+                                                  bundle.getString("attachmentReminderOptionsNo"),
>+                                                  null,
>+                                                  null,
>+                                                  check);
>+            if (!button){

Nit: add one space after )


>+## Attachment Reminder
>+mail.attachment_keywords=attachment,attached,PFA,PDF,Doc,Document,Slide,Presentation,File,zip,rar,

Please reduce the list as discussed earlier in this bug.

Comment 47

[Magnus Melin [:mkmelin]]

Comment on attachment 360490 [details] [diff] [review]
patch-5

>+ * The Original Code is the Thunderbird Preferences System.

is mozilla.org code, or something.

>+ *
>+ * The Initial Developer of the Original Code is
>+ * Chinmay Deepakbhai Patel.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+var gAttachmentOptionsDialog = {
>+
>+  mKeywordListBox: null,
>+  mprefsBranch: null,
>+  bundle: null,
>+
>+  init: function ()
>+  {
>+    this.mKeywordListBox = document.getElementById('keywordList');
>+    this.buildKeywordList();
>+    this.bundle = document.getElementById("bundlePreferences");

Be consistent with ' and ", please.

>+  },
>+
>+  buildKeywordList: function()
>+  {
>+    mprefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                             .getService(Components.interfaces.nsIPrefService)
>+                             .getBranch(null);

See comment 25.

>+    var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                    Components.interfaces.nsIPrefLocalizedString); 
>+    var keywordsInCsv = keywordsInCsv.data;
>+    var keywordsInArr = keywordsInCsv.split(",");
>+    for (var i = 0; i < keywordsInArr.length; i++)
>+      {

Misaligned.

>+      this.mKeywordListBox.appendItem(keywordsInArr[i], keywordsInArr[i]);
>+      }
>+    this.mKeywordListBox.selectedIndex = 0;
>+  },
>+
>+  addKeyWord: function ()
>+  {
>+    gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                               .getService(Components.interfaces.nsIPromptService);

Hm, either the gPromptService is available already, or you probably shouldn't assign it at this point.

>+    var input = {value: "Keyword"};

Unlocalized.

>+    var response  = gPromptService.prompt(

Extra spaces.

>+                                 window,
>+                                 this.bundle.getString("addDialogTitle"),
>+                                 this.bundle.getString("addText"),
>+                                 input,
>+                                 null,
>+                                 {value: 0});
>+    if (response){

Space after )

>+      var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                      Components.interfaces.nsIPrefLocalizedString);
>+      keywordsInCsv.data += "," + input.value;
>+      mprefsBranch.setComplexValue("mail.attachment_keywords",
>+                                  Components.interfaces.nsISupportsString, keywordsInCsv);
>+      this.mKeywordListBox.appendItem(input.value, input.value);
>+    }
>+  },
>+
>+  editKeyWord: function ()
>+  {
>+    if (this.mKeywordListBox.selectedIndex >= 0)
>+    {
>+      var keywordElToEdit = this.mKeywordListBox.getItemAtIndex(this.mKeywordListBox.selectedIndex);
>+      gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                                 .getService(Components.interfaces.nsIPromptService);

Same here wrt gPromptService

>+      var input = {value: keywordElToEdit.getAttribute("value")};
>+      var response  = gPromptService.prompt(
>+                                            window,
>+                                            this.bundle.getString("editDialogTitle"),
>+                                            this.bundle.getString("editText"),
>+                                            input,
>+                                            null,
>+                                            {value: 0});
>+      if (response)
>+      {
>+        this.mKeywordListBox.removeItemAt(this.mKeywordListBox.selectedIndex);
>+        var oldValue= keywordElToEdit.getAttribute("value");
>+        var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                        Components.interfaces.nsIPrefLocalizedString);
>+        keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , input.value);
>+        mprefsBranch.setComplexValue("mail.attachment_keywords",
>+                                    Components.interfaces.nsISupportsString, keywordsInCsv);

Misaligned. And please fold the last argument to the next row

>+        this.mKeywordListBox.appendItem(input.value, input.value);
>+      }
>+    }
>+  },
>+  
>+  removeKeyWord: function ()
>+  {
>+    if (this.mKeywordListBox.selectedIndex >= 0)
>+    {
>+      var keywordElToEdit = this.mKeywordListBox.getItemAtIndex(this.mKeywordListBox.selectedIndex);
>+      this.mKeywordListBox.removeItemAt(this.mKeywordListBox.selectedIndex);
>+      var oldValue= keywordElToEdit.getAttribute("value");
>+      var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                      Components.interfaces.nsIPrefLocalizedString);
>+      keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , "")
>+                                             .replace(",," , ",")

Need to be a regexp, and global.

>+                                             .replace(/^,/, "")
>+                                             .replace(/,$/, "");

.replace(/^,|,$/g, "");


>
>+# Portions created by the Initial Developer are Copyright (C) 2___
>+# the Initial Developer. All Rights Reserved.

2009

>+#
>+# Contributor(s):
>+#
>+# Alternatively, the contents of this file may be used under the terms of
>+# either the GNU General Public License Version 2 or later (the "GPL"), or
>+# the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+# in which case the provisions of the GPL or the LGPL are applicable instead
>+# of those above. If you wish to allow use of your version of this file only
>+# under the terms of either the GPL or the LGPL, and not to allow others to
>+# use your version of this file under the terms of the MPL, indicate your
>+# decision by deleting the provisions above and replace them with the notice
>+# and other provisions required by the GPL or the LGPL. If you do not delete
>+# the provisions above, a recipient may use your version of this file under
>+# the terms of any one of the MPL, the GPL or the LGPL.
>+#
>+# ***** END LICENSE BLOCK *****
>+
>+<?xml-stylesheet href="chrome://global/skin/"?>
>+#ifdef XP_MACOSX
>+<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
>+#endif
>+
>+<!DOCTYPE prefwindow [
>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
>+<!ENTITY % sendOptionsDTD SYSTEM "chrome://messenger/locale/preferences/attachmentoptions.dtd" >
>+%sendOptionsDTD;
>+]>
>+
>+<prefwindow id="attachmentOptionsDialog" type="child"
>+            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            dlgbuttons="accept,cancel"
>+            title="&dialog.title;">
>+
>+  <prefpane id="attachmentOptionsDialogPane"  onpaneload="gAttachmentOptionsDialog.init();">
>+
>+    <script type="application/x-javascript" src="chrome://messenger/content/preferences/attachmentoptions.js"/>
>+    <stringbundle id="bundlePreferences" src="chrome://messenger/locale/preferences/preferences.properties"/>
>+
>+    <groupbox>
>+      <caption label="&attacReminder.label;"/>
>+      <label control="keywordList">&attachKeywordText.label;</label>
>+      <hbox>
>+        <listbox id="keywordList" flex="1" rows="5" ondblclick="gAttachmentOptionsDialog.editKeyWord();"/>
>+        <vbox>
>+          <button label="&addKeywordButton.label;" accesskey="&addKeywordButton.accesskey;"
>+                  oncommand="gAttachmentOptionsDialog.addKeyWord();"/>
>+          <button label="&editKeywordButton.label;" accesskey="&editKeywordButton.accesskey;"
>+                  oncommand="gAttachmentOptionsDialog.editKeyWord();"/>
>+          <button label="&removeKeywordButton.label;" accesskey="&removeKeywordButton.accesskey;"
>+                  oncommand="gAttachmentOptionsDialog.removeKeyWord();"/>
>+        </vbox>
>+      </hbox>
>+    </groupbox>
>+  </prefpane>
>+</prefwindow>
>+
>diff --git a/mail/components/preferences/compose.js b/mail/components/preferences/compose.js
>--- a/mail/components/preferences/compose.js
>+++ b/mail/components/preferences/compose.js
>@@ -76,16 +76,21 @@ var gComposePane = {
>       var preference = document.getElementById("mail.preferences.compose.selectedTabIndex");
>       preference.valueFromPreferences = document.getElementById("composePrefs").selectedIndex;
>     }
>   },
> 
>   sendOptionsDialog: function()
>   {
>     document.documentElement.openSubDialog("chrome://messenger/content/preferences/sendoptions.xul","", null);
>+  },
>+
>+  attachmentOptionsDialog: function()
>+  {
>+    document.documentElement.openSubDialog("chrome://messenger/content/preferences/attachmentoptions.xul","", null);
>   },
> 
>   htmlComposeDialog: function()
>   {
>     document.documentElement.openSubDialog("chrome://messenger/content/preferences/htmlcompose.xul","", null);
>   },
> 
>   enableElement: function(aElement, aEnable)
>diff --git a/mail/components/preferences/compose.xul b/mail/components/preferences/compose.xul
>--- a/mail/components/preferences/compose.xul
>+++ b/mail/components/preferences/compose.xul
>@@ -74,16 +74,19 @@
>                   name="pref.ldap.disable_button.edit_directories" type="bool"/>
>       <preference id="mail.collect_email_address_outgoing" name="mail.collect_email_address_outgoing" type="bool"/>
>       <preference id="mail.collect_addressbook" name="mail.collect_addressbook" type="string"/>
>       <preference id="spellchecker.dictionary"  name="spellchecker.dictionary"   type="unichar"/>
>       <preference id="msgcompose.font_face"      name="msgcompose.font_face" type="string"/>
>       <preference id="msgcompose.font_size"      name="msgcompose.font_size" type="string"/>
>       <preference id="msgcompose.text_color"     name="msgcompose.text_color" type="string"/>
>       <preference id="msgcompose.background_color" name="msgcompose.background_color" type="string"/>      
>+      <preference id="mail.compose.attachmet_reminder_checkbox"
>+                  name="mail.compose.attachmet_reminder_checkbox"
>+                  type="bool"/>

Misspelled, and should drop the _checkbox, match the pref name

Comment 48

[Chinmay Patel]

Attachment: patch6.

In reply of:
> Misspelled, and use a "// " comment please. 
// +        //Attachment Reminder

In Reply of: 
> See comment 25.
1. 
+          var prefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
+                                      .getService(Components.interfaces.nsIPrefBranch);

2.
+            var re = new RegExp(keywordsArray[i], "i");
+            keywordFound = re.exec(mailData);

Comment 49

[Chinmay Patel]

****************************************
In Reply of:
> if (!button){
> Nit: add one space after )

+            if (!button) {
+              AttachFile();


****************************************
In Reply of:
> is mozilla.org code, or something.

+ * The Original Code is mozilla.org code.


****************************************
In Reply of:
> Be consistent with ' and ", please.

I have changed all ' to ". 
+    this.mKeywordListBox = document.getElementById("keywordList");
+    this.buildKeywordList();


****************************************
In Reply of:
>+    for (var i = 0; i < keywordsInArr.length; i++)
>+      {
>Misaligned.

+    for (var i = 1; i < keywordsInArr.length; i++)
+    {
+      this.mKeywordListBox.appendItem(keywordsInArr[i], keywordsInArr[i]);
+    }


****************************************
In Reply of:
>+    gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                               .getService(Components.interfaces.nsIPromptService);
>Hm, either the gPromptService is available already, or you probably shouldn't assign it at this point.

I have made gPromptService global at the top and assigned the value in init(),

+var gAttachmentOptionsDialog = {
+
+  mKeywordListBox: null,
+  mprefsBranch: null,
+  bundle: null,
+  gPromptService: null,
+
+  init: function ()
+  {
+    this.mKeywordListBox = document.getElementById("keywordList");
+    this.buildKeywordList();
+    this.bundle = document.getElementById("bundlePreferences");    
+    gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
+                               .getService(Components.interfaces.nsIPromptService);    
+  },


****************************************
In Reply of:
>+    var input = {value: "Keyword"};
>Unlocalized.

+    var input = {value: this.bundle.getString("defaultKeyword")};


****************************************
In Reply of:
>+    var response  = gPromptService.prompt(
>Extra spaces.

+      var response = gPromptService.prompt(


****************************************
In Reply of:
>+    if (response){
>Space after )

+      if (response) {


****************************************
In Reply of:
>+      if (response)
>+      {
>+        this.mKeywordListBox.removeItemAt(this.mKeywordListBox.selectedIndex);
>+        var oldValue= keywordElToEdit.getAttribute("value");
>+        var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                        Components.interfaces.nsIPrefLocalizedString);
>+        keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , input.value);
>+        mprefsBranch.setComplexValue("mail.attachment_keywords",
>+                                    Components.interfaces.nsISupportsString, keywordsInCsv);
>Misaligned. And please fold the last argument to the next row

+    if (this.mKeywordListBox.selectedIndex >= 0) {
+      var keywordToEdit = this.mKeywordListBox.getItemAtIndex(this.mKeywordListBox.selectedIndex);
+      this.mKeywordListBox.removeItemAt(this.mKeywordListBox.selectedIndex);
+      var oldValue= keywordToEdit.getAttribute("value");
+      var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
+                                                      Components.interfaces.nsIPrefLocalizedString);
+      oldValue = "," + oldValue;
+      keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , "");
+      mprefsBranch.setComplexValue("mail.attachment_keywords",
+                                  Components.interfaces.nsISupportsString, 
+                                  keywordsInCsv);
+    }

****************************************
In Reply of:
>+      keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , "")
>+                                             .replace(",," , ",")
>Need to be a regexp, and global.
>+                                             .replace(/^,/, "")
>+                                             .replace(/,$/, "");
>.replace(/^,|,$/g, "");


I have changed the whole concept for storing the words. Although I am storing the keywords in a csv, I am adding a "," in the begining. 
+## Attachment Reminder
+mail.attachment_keywords=,attachment,resume,cover letter,certificate,white paper,ecard,letterhead,enclosure

So, everytime I remove a keyword, I will just have to remove ",keyword". 
+      oldValue = "," + oldValue;
+      keywordsInCsv.data = keywordsInCsv.data.replace(oldValue , "");

Plus, at the time of adding the keyword , I have to add ",keyword". 
+      keywordsInCsv.data += "," + input.value;


The problem with that was my keyword list ws showing an extra blank line at the first place. So, I am going to make the list from the 2nd(array starts from 0) position of the array. 
+    for (var i = 1; i < keywordsInArr.length; i++)
+    {

I have checked it and it worked. Let me know if you find any problem with that.

****************************************
In Reply of:
>+# Portions created by the Initial Developer are Copyright (C) 2___
>+# the Initial Developer. All Rights Reserved.
> 2009

+# Portions created by the Initial Developer are Copyright (C) 2009

****************************************
In Reply of:
+      <preference id="mail.compose.attachmet_reminder_checkbox"
>+                  name="mail.compose.attachmet_reminder_checkbox"
>+                  type="bool"/>
> Misspelled, and should drop the _checkbox, match the pref name

+      <preference id="mail.compose.attachment_reminder"
+                  name="mail.compose.attachment_reminder"
+                  type="bool"/>

(: ******************* DONE ******************* :)

Hopefully, I have done enough work this time. Let me know, if there are other errors remaining...

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 363638 [details] [diff] [review]
patch6.

This is starting to look a lot better, thx!

Some nits and comments:

>diff --git a/mail/components/compose/content/MsgComposeCommands.js b/mail/components/compose/content/MsgComposeCommands.js
>--- a/mail/components/compose/content/MsgComposeCommands.js
>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -1676,16 +1676,60 @@ function GenericSendMessage( msgType )
>               msgCompFields.subject = result.value;
>               var subjectInputElem = GetMsgSubjectElement();
>               subjectInputElem.value = result.value;
>             }
>             else
>               return;
>           }
>         }
>+
>+        //Attachment Reminder

Space before Attachment

>+        var bucket = document.getElementById("attachmentBucket");
>+        var warn = getPref("mail.compose.attachment_reminder");
>+        if (warn && !bucket.itemCount)
>+        {
>+          var bundle = document.getElementById("bundle_composeMsgs");
>+          var keywordFound = false;
>+          var prefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                                      .getService(Components.interfaces.nsIPrefBranch);
>+          var keywordsInCsv = prefsBranch.getComplexValue("mail.attachment_keywords",
>+                                             Components.interfaces.nsIPrefLocalizedString).data; 
>+          var keywordsArray = keywordsInCsv.split(",");
>+
>+          var contentFrame = document.getElementById('content-frame');
>+          var mailBody = contentFrame.contentDocument.getElementsByTagName('body')[0];
>+          var mailBodyNode = mailBody.cloneNode(true);
>+          var mailData = mailBodyNode.innerHTML;
>+          for (var i = 0; i < keywordsArray.length && !keywordFound; i ++)

Extra space before ++

>+            var re = new RegExp(keywordsArray[i], "i");
>+            keywordFound = re.exec(mailData);
>+          }
>+
>+          if (keywordFound)
>+          {
>+            var check = {value: false};
>+            var flags = gPromptService.BUTTON_POS_0 * gPromptService.BUTTON_TITLE_IS_STRING +
>+                        gPromptService.BUTTON_POS_1 * gPromptService.BUTTON_TITLE_IS_STRING; 
>+            var button = gPromptService.confirmEx(null,
>+                                                  bundle.getString("attachmentReminderTitle"),
>+                                                  bundle.getString("attachmentReminderMsg"),
>+                                                  flags,
>+                                                  bundle.getString("attachmentReminderOptionsYes"),
>+                                                  bundle.getString("attachmentReminderOptionsNo"),
>+                                                  null,
>+                                                  null,
>+                                                  check);
>+            if (!button) {
>+              AttachFile();
>+              return;
>+            }

I think these buttons need to change places (yes to no, and vice versa). I keep hitting the wrong button (or the close x button) when wanting to go back.

I think it's too much text on the buttons now - and I think I like something like

 [Oh, I did!] [No, Send Now]

... better. Might be just me, but having a button "Attach Files" is too much of a choice. All I want to do is go back and look at the mail again. Instinctively I hit the close button on the dialog, and the mail was sent out.

>+# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*-

offset 2 would be better.

>+var gAttachmentOptionsDialog = {
>+
>+  mKeywordListBox: null,
>+  mprefsBranch: null,
>+  bundle: null,
>+  gPromptService: null,

g variables are not to be used as member variables.

Since this is JavaScript, the m prefix also is quite redundant, since you have to access them with this. anyway.

... the "member" gPromptService isn't even used for anything now. 

>+
>+  init: function ()
>+  {
>+    this.mKeywordListBox = document.getElementById("keywordList");
>+    this.buildKeywordList();
>+    this.bundle = document.getElementById("bundlePreferences");    
>+    gPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                               .getService(Components.interfaces.nsIPromptService);    
>+  },

You should probably initalize the mprefsBranch in init?


>+      var oldValue= keywordToEdit.getAttribute("value");

Missing space.

>+  attachmentOptionsDialog: function()
>+  {
>+    document.documentElement.openSubDialog("chrome://messenger/content/preferences/attachmentoptions.xul","", 

Please add a space after commas

>+pref("mail.attachment_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties");

Also add the reminder pref to be on by default. But you're adding it to the wrong file. (Not all-l10n.js, should probaly be in all-thunderbird.js
>@@ -0,0 +1,10 @@
>+<!ENTITY dialog.title                  "Attachment Options">

Attachment Reminder Options

>+<!ENTITY attacReminder.label           "Attachment Reminder">
>+<!ENTITY attachKeywordText.label       "Thunderbird will look for these keywords in your email body.">

Should use &brandShortName; instead of thunderbird.

"&brandShortName; will warn you about missing attachments if the you're about to send an e-mail containing one of these keywords".

>+<!ENTITY addKeywordButton.label        "Insert">

"Add" would be consistent with other places.

BTW, can you make the Delete key delete the currently selected item?

>+#### Attachment Reminder
>+addDialogTitle=Add Keyword
>+addText=Enter a Keyword

with a ":" on the end, same for the other similar cases.

Re the commas: can't you just always iterate over the list items and generate the list instead? 

Another thing I noticed - if I remove all keywords, I'm always warned.

Comment 51

[Nomis101]

I have a question about
> mail.attachment_keywords=,attachment,resume,cover...
Are the keywords case sensitive, so does it matter if I write "attachment" or "Attachment"? And for what reason is the comma before the first keyword, is this essential?

Comment 52

[Chinmay Patel]

In Reply of: Comment #51
Yes these keywords are Case Sensitive. Plus, I have removed the comma at the beginning and accepted the method of creating the array by iterating through the list every time.

Comment 54

[Chinmay Patel]

Attachment: Patch 7

************************************************************
In Reply Of:
>+        //Attachment Reminder
> Space before Attachment

+// Attachment Reminder


************************************************************
In Reply Of:
>+          for (var i = 0; i < keywordsArray.length && !keywordFound; i ++)
>Extra space before ++

+          for (var i = 0; i < keywordsArray.length && !keywordFound; i++)

************************************************************
In Reply Of:

>I think these buttons need to change places (yes to no, and vice versa). I keep
>hitting the wrong button (or the close x button) when wanting to go back.
>
>I think it's too much text on the buttons now - and I think I like something
>like
>
> [Oh, I did!] [No, Send Now]
>
>... better. Might be just me, but having a button "Attach Files" is too much of
>a choice. All I want to do is go back and look at the mail again. Instinctively
>I hit the close button on the dialog, and the mail was sent out.


+          if (keywordFound)
+          {
+            var check = {value: false};
+            var flags = gPromptService.BUTTON_POS_0 * gPromptService.BUTTON_TITLE_IS_STRING +
+                        gPromptService.BUTTON_POS_1 * gPromptService.BUTTON_TITLE_IS_STRING; 
+            var button = gPromptService.confirmEx(null,
+                                                  bundle.getString("attachmentReminderTitle"),
+                                                  bundle.getString("attachmentReminderMsg"),
+                                                  flags,
+                                                  bundle.getString("attachmentReminderOptionsNo"),
+                                                  bundle.getString("attachmentReminderOptionsYes"),
+                                                  null,
+                                                  null,
+                                                  check);
+            if (button) {
+              return;
+            }

Comment 55

[Chinmay Patel]

************************************************************
In Reply of:
>+# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>offset 2 would be better.

+# -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
in both files.

************************************************************
In Reply Of:
>+var gAttachmentOptionsDialog = {
>+
>+  mKeywordListBox: null,
>+  mprefsBranch: null,
>+  bundle: null,
>+  gPromptService: null,
>g variables are not to be used as member variables.
>
>Since this is JavaScript, the m prefix also is quite redundant, since you have
>to access them with this. anyway.
>... the "member" gPromptService isn't even used for anything now. 

> You should probably initalize the mprefsBranch in init?

+
+  mKeywordListBox: null,
+  mprefsBranch: null,
+  bundle: null,
+  mPromptService: null,
+
+  init: function ()
+  {
+    mprefsBranch = Components.classes["@mozilla.org/preferences-service;1"]
+                             .getService(Components.interfaces.nsIPrefBranch);
+    mPromptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
+                               .getService(Components.interfaces.nsIPromptService);
+    this.mKeywordListBox = document.getElementById("keywordList");
+    this.buildKeywordList();
+    this.bundle = document.getElementById("bundlePreferences");    
+  },

************************************************************
In Reply Of:
>+      var oldValue= keywordToEdit.getAttribute("value");
>Missing space.

I have taken out the whole line. So, don't have to worry about it!

************************************************************
In Reply Of:
>+  attachmentOptionsDialog: function()
>+  {
>+    document.documentElement.openSubDialog("chrome://messenger/content/preferences/attachmentoptions.xul","", 
>Please add a space after commas

+    document.documentElement.openSubDialog("chrome://messenger/content/preferences/attachmentoptions.xul", "", null);

************************************************************
In Reply of:
>+pref("mail.attachment_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties");
>
>Also add the reminder pref to be on by default. But you're adding it to the
wrong file. (Not all-l10n.js, should probaly be in all-thunderbird.js

+++ b/mail/app/profile/all-thunderbird.js
@@ -428,8 +428,11 @@ pref("mail.winsearch.logging.dump", fals
 #else
 #ifdef XP_MACOSX
 // Should we output warnings and errors to the "error console"?
 pref("mail.spotlight.logging.console", false);
 // Should we output all output levels to stdout via dump?
 pref("mail.spotlight.logging.dump", false);
 #endif
 #endif
+
+// Attachment Reminder
+pref("mail.attachment_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties");


************************************************************
In Reply of:
>@@ -0,0 +1,10 @@
>+<!ENTITY dialog.title                  "Attachment Options">
>Attachment Reminder Options

+++ b/mail/locales/en-US/chrome/messenger/preferences/attachmentoptions.dtd
@@ -0,0 +1,9 @@
+<!ENTITY dialog.title                   "Attachment Reminder Options">


************************************************************
In Reply of:
>+<!ENTITY attacReminder.label           "Attachment Reminder">
>+<!ENTITY attachKeywordText.label       "Thunderbird will look for these keywords in your email body.">
>
>Should use &brandShortName; instead of thunderbird.
>
>"&brandShortName; will warn you about missing attachments if the you're about
>to send an e-mail containing one of these keywords".

+<!ENTITY attachKeywordText.label       "&brandShortName; will warn you about missing attachments if the you're about to send an e-mail containing one of these keywords">


************************************************************
In Reply of:
>+<!ENTITY addKeywordButton.label        "Insert">
>
>"Add" would be consistent with other places.
>
>BTW, can you make the Delete key delete the currently selected item?

It deletes the selected item. But, it doesn't select any default items after deleting an item. Do we need to do that?


************************************************************
In Reply of:
>+#### Attachment Reminder
>+addDialogTitle=Add Keyword
>+addText=Enter a Keyword
>
>with a ":" on the end, same for the other similar cases.

I didn't get this part properly. As far as I understood the problem,
+## Attachment Reminder
+mail.attachment_keywords=attachment,resume,cover letter,certificate,white paper,ecard,letterhead,enclosure
+attachmentReminderTitle=Attachment Reminder
+attachmentReminderMsg=Did you forget to add an attachment?
+attachmentReminderOptionsYes=Oh, I did!
+attachmentReminderOptionsNo=No, Send Now:
                      &
+#### Attachment Reminder
+addDialogTitle=Add Keyword
+addText=Enter a Keyword
+editDialogTitle=Edit Keyword
+editText=Edit the Keyword
+defaultKeyword=Keyword:

************************************************************
In Reply of:
>Re the commas: can't you just always iterate over the list items and generate
>the list instead? 

+  createKeywordList: function ()
+  {
+    var i = 0;
+    var keywordList = "";
+    for(; i < this.mKeywordListBox.getRowCount()-1; i++){
+      keywordList += this.mKeywordListBox.getItemAtIndex(i).getAttribute("value") + ",";
+    }
+    if (this.mKeywordListBox.getRowCount())
+      keywordList += this.mKeywordListBox.getItemAtIndex(i).getAttribute("value");
+    var str = Components.classes["@mozilla.org/supports-string;1"]
+                        .createInstance(Components.interfaces.nsISupportsString);
+    str.data = keywordList;
+    mprefsBranch.setComplexValue("mail.attachment_keywords",
+                                Components.interfaces.nsISupportsString, 
+                                str);
+  }


************************************************************
In Reply of:
Another thing I noticed - if I remove all keywords, I'm always warned.

As per this new logic, this won't happen anymore.

************************************************************
 
 Now, I am dying to get review+. He he ...

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 365678 [details] [diff] [review]
Patch 7

We're getting there! Still a few things to improve though.

>+            if (button) {
>+              return;
>+            }

Nit: don't need the  { } here. And Maybe a better name than "button" ;)


>+ * The Initial Developer of the Original Code is
>+ * Chinmay Deepakbhai Patel.

You should usually include your email too ( you <youremail> )

>+  buildKeywordList: function()
>+  {
>+    var i = 0;
>+    var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                    Components.interfaces.nsIPrefLocalizedString); 
>+    var keywordsInCsv = keywordsInCsv.data;
>+    var keywordsInArr = keywordsInCsv.split(",");
>+    for (; i < keywordsInArr.length; i++)

Please have the i = 0 inside the for loop (declaring it earlier, or not, in js you don't have to). Same thing later.


>+      var response = mPromptService.prompt(
>+                                           window,

Move window up a row.


>+++ b/mail/components/preferences/attachmentoptions.xul

camelCase filenames like attachmentOptions.xul would be nicer. Rename to attachmentReminder.xul?

>+<?xml-stylesheet href="chrome://global/skin/"?>
>+#ifdef XP_MACOSX
>+<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
>+#endif

Is this needed?

>+<!DOCTYPE prefwindow [
>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
>+<!ENTITY % sendOptionsDTD SYSTEM "chrome://messenger/locale/preferences/attachmentoptions.dtd" >
>+%sendOptionsDTD;
>+]>
>+
>+<prefwindow id="attachmentOptionsDialog" type="child"
>+            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            dlgbuttons="accept,cancel"
>+            title="&dialog.title;">
>+
>+  <prefpane id="attachmentOptionsDialogPane"  onpaneload="gAttachmentOptionsDialog.init();">
>+
>+    <script type="application/x-javascript" src="chrome://messenger/content/preferences/attachmentoptions.js"/>
>+    <stringbundle id="bundlePreferences" src="chrome://messenger/locale/preferences/preferences.properties"/>
>+
>+    <groupbox>
>+      <caption label="&attacReminder.label;"/>

Typo, should be attach.


>+## Attachment Reminder
>+mail.attachment_keywords=attachment,resume,cover letter,certificate,white 

Please change the pref name to mail.compose.attachment_reminder_keywords so it's easier to find when looking for the other pref.

paper,ecard,letterhead,enclosure
>+attachmentReminderTitle=Attachment Reminder
>+attachmentReminderMsg=Did you forget to add an attachment?
>+attachmentReminderOptionsYes=Oh, I did!
>+attachmentReminderOptionsNo=No, Send Now:

No ':' at the end please.

>+<!ENTITY attachKeywordText.label       "&brandShortName; will warn you about missing attachments if the you're about to send an e-mail containing one of these keywords">

Add a dot at the end.

>+<!ENTITY attachmentOptions.label               "Advancedâ¦">
>+<!ENTITY attachmentOptions.accesskey           "A">

A is taken already.

>+#### Attachment Reminder
>+addDialogTitle=Add Keyword
>+addText=Enter a Keyword

I meant it should be addText=Keyword:

>+defaultKeyword=Keyword:

Should be empty

Comment 57

[Chinmay Patel]

Attachment: Patch 8

Thanks for spending time on me ...

***********************************************
In Reply Of:

>+            if (button) {
>+              return;
>+            }
>Nit: don't need the  { } here. And Maybe a better name than "button" ;)


+            if (success)
+              return;


***********************************************
In Reply Of:
>+ * The Initial Developer of the Original Code is
>+ * Chinmay Deepakbhai Patel.
>You should usually include your email too ( you <youremail> )

+ * Chinmay Deepakbhai Patel<[e-mail]>.
&
+# Chinmay Deepakbhai Patel<[e-mail]>.

***********************************************

Comment 58

[Chinmay Patel]

In Reply Of:
>+  buildKeywordList: function()
>+  {
>+    var i = 0;
>+    var keywordsInCsv = mprefsBranch.getComplexValue("mail.attachment_keywords",
>+                                                    Components.interfaces.nsIPrefLocalizedString); 
>+    var keywordsInCsv = keywordsInCsv.data;
>+    var keywordsInArr = keywordsInCsv.split(",");
>+    for (; i < keywordsInArr.length; i++)
>Please have the i = 0 inside the for loop (declaring it earlier, or not, in js
>you don't have to). Same thing later.

+    for(var i = 0; i < this.mKeywordListBox.getRowCount()-1; i++){

***********************************************
In Reply Of:
>+      var response = mPromptService.prompt(
>+                                           window,
>Move window up a row.

+    var response = mPromptService.prompt(window,

***********************************************
In Reply Of:
>+++ b/mail/components/preferences/attachmentoptions.xul
>camelCase filenames like attachmentOptions.xul would be nicer. Rename to
>attachmentReminder.xul?c

I have renamed the files as attachmentReminder.xul, attachmentReminder.js, and attachmentReminder.dtd. 
However, if I look at the other files in the code, they are not camelCase. So, is it a good idea to have a difference in the style of the filename?

***********************************************
In Reply Of:
>+<?xml-stylesheet href="chrome://global/skin/"?>
>+#ifdef XP_MACOSX
>+<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
>+#endif
>Is this needed?

Yes, we need this. I took it out and ran it. It broke the whole window style. 

***********************************************
In Reply Of:
>+<!DOCTYPE prefwindow [
>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
>+<!ENTITY % sendOptionsDTD SYSTEM "chrome://messenger/locale/preferences/attachmentoptions.dtd" >
>+%sendOptionsDTD;
>+]>
>+
>+<prefwindow id="attachmentOptionsDialog" type="child"
>+            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            dlgbuttons="accept,cancel"
>+            title="&dialog.title;">
>+
>+  <prefpane id="attachmentOptionsDialogPane"  onpaneload="gAttachmentOptionsDialog.init();">
>+
>+    <script type="application/x-javascript" src="chrome://messenger/content/preferences/attachmentoptions.js"/>
>+    <stringbundle id="bundlePreferences" src="chrome://messenger/locale/preferences/preferences.properties"/>
>+
>+    <groupbox>
>+      <caption label="&attacReminder.label;"/>
>Typo, should be attach.

+      <caption label="&attachReminder.label;"/>

***********************************************
In Reply Of:
>+## Attachment Reminder
>+mail.attachment_keywords=attachment,resume,cover letter,certificate,white 
>Please change the pref name to mail.compose.attachment_reminder_keywords so
>it's easier to find when looking for the other pref.

+## Attachment Reminder
+mail.compose.attachment_reminder_keywords=attachment,resume,cover letter,certificate,white paper,ecard,letterhead,enclosure

+// Attachment Reminder
+pref("mail.compose.attachment_reminder_keywords", "chrome://messenger/locale/messengercompose/composeMsgs.properties");

and everywhelse it is needed.

***********************************************
In Reply Of:
>+attachmentReminderTitle=Attachment Reminder
>+attachmentReminderMsg=Did you forget to add an attachment?
>+attachmentReminderOptionsYes=Oh, I did!
>+attachmentReminderOptionsNo=No, Send Now:
>No ':' at the end please.

+attachmentReminderOptionsNo=No, Send Now

***********************************************
In Reply Of:
>+<!ENTITY attachKeywordText.label       "&brandShortName; will warn you about missing attachments if the you're about to send an e-mail containing one of these keywords">
>Add a dot at the end.

+<!ENTITY attachKeywordText.label        "&brandShortName; will warn you about missing attachments if the you're about to send an e-mail containing one of these keywords.">


***********************************************
In Reply Of:
>+<!ENTITY attachmentOptions.label               "Advancedâ?¦">
>+<!ENTITY attachmentOptions.accesskey           "A">
>A is taken already.

I have taken 'D' as the accesskey because it is the second letter in the 'A.D.V.A.N.C.E.D' word.
+<!ENTITY attachmentOptions.accesskey           "D">


***********************************************
In Reply Of:
>+#### Attachment Reminder
>+addDialogTitle=Add Keyword
>+addText=Enter a Keyword
>I meant it should be addText=Keyword:

+addDialogTitle=Add Keyword
+addText=Keyword:
+editDialogTitle=Edit Keyword
+editText=Keyword:

I am using both addText and editText, two seperate variables, because we can change it in future, if reuired.

***********************************************
In Reply Of:
>+defaultKeyword=Keyword:
>Should be empty

+defaultKeyword=
 I am just keeping a blank value for the sake of future expansion.

**************************************************

Comment 59

[Magnus Melin [:mkmelin]]

Comment on attachment 367227 [details] [diff] [review]
Patch 8

Sorry for the delay. 

The patch has bitrotted a bit.

One obviously wrong thing I notice:

> +  addKeyWord: function ()caption label="&attacReminder.label;

Also add a space between your name and the <email>

Comment 60

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 367227 [details] [diff] [review]
Patch 8

removing myself from this, the patch seems to have rotted according to other comments.  I'll try it out again when it applies cleanly.

Comment 61

[Markus Multrus]

grrr!!!!! I don't get it: First, a newbie provides a patch. Secondly, instead of motivating him and getting the patch through as fast as possible, the reviewers need months to complain about totally senseless whitespace issues. Instead of helping Chinmay directly, every whitespace issue is responded to in a separate comment, always with delay. And now, the persons, responsible for all the delay, complain about the bitrot... I mean: that's really not very productive, and not very motivating for new community members... :-(

Comment 62

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #61)
> grrr!!!!!

I completely understand the frustration.  I hope my comment above didn't come across as brash, I just can't get it the patch to apply.

Markus, if you have any time to work with Chinmay on this patch I think a collective of even just a few people could get it done; especially taking care of the white space issues.

Comment 63

[David Humphrey (:humph)]

(In reply to comment #61)
> grrr!!!!! I don't get it: First, a newbie provides a patch.

Hey Markus, I'm Chinmay's prof--he's doing this work as part of a course on open source.  I wanted to connect with you to say that I understand your frustration, but also that this isn't headed for ruin.

> of motivating him and getting the patch through as fast as possible, the
> reviewers need months to complain about totally senseless whitespace issues.
> Instead of helping Chinmay directly, every whitespace issue is responded to in
> a separate comment, always with delay. And now, the persons, responsible for
> all the delay, complain about the bitrot... I mean: that's really not very
> productive, and not very motivating for new community members... :-(

A couple things to note:

* Chinmay has gone from knowing nothing about working on Mozilla to being able to contribute code here and in a few other bugs.  I'm really pleased with how far he's come, and I think he's happy too.

* Mozilla is pretty gracious with us, letting students come in and work on things at all: I know from first-hand experience that working with someone new means substantial investment of time, much more than a patch like this would normally entail.

* Chinmay is learning as much about process as he is trying to get something done.  This bug, while taking longer than it might if it weren't his first, is showing him what the review process is all about, the need for attention to detail, the fact that code development is iterative with feedback built in, and that others (like you!) care about contributions a great deal.  He's also seeing how the project gets managed (e.g., Bryan removing the ui review until it applies cleanly, so he can manage his review queue).

I'm pleased with his progress, and will encourage (and require :) him to finish this work before the term ends.  When he's done, he'll be able to say that he really understands what it's like to work on a large community open source project, and make an important contribution.  That's good for everyone.

Another thing that is great to see in this bug, are the presence of comments like this one:

"This is starting to look a lot better, thx!"

That helps encourage new contributors a lot, when reviews can often be "don't do this" over and over again.

At any rate, this is coming along, on track to get finished in the next few weeks, and a great introduction to what Mozilla is like.  Thanks for being on the look-out for stop-energy in bugzilla: I see it a lot too, but I don't think it's happening here.

Comment 64

[Chinmay Patel]

Attachment: Patch 9

I guess, this is getting too hot, now. Let's finish it off. Here is the latest HG patch addressing all the previous comments.


Note: I have tested this patch by applying it to another tree and it has worked pretty well. Let me know, if the patch still doesn't apply cleanly.

Comment 65

[Magnus Melin [:mkmelin]]

Comment on attachment 369619 [details] [diff] [review]
Patch 9

Sorry this doesn't build. You need to resolve the conflicts, now they are included in the patch

> *   content/messenger/preferences/sendoptions.js
>+<<<<<<< local
>+*   content/messenger/preferences/attachmentReminder.xul
>+*   content/messenger/preferences/attachmentReminder.js
>+*   content/messenger/preferences/privacy.xul
>+*   content/messenger/preferences/privacy.js
>+=======
> *   content/messenger/preferences/security.xul
> *   content/messenger/preferences/security.js
>+>>>>>>> other

>   locale/@AB_CD@/messenger/preferences/sendoptions.dtd                  (%chrome/messenger/preferences/sendoptions.dtd)
>+<<<<<<< local
>+  locale/@AB_CD@/messenger/preferences/attachmentReminder.dtd           (%chrome/messenger/preferences/attachmentReminder.dtd)
>+  locale/@AB_CD@/messenger/preferences/privacy.dtd                      (%chrome/messenger/preferences/privacy.dtd)
>+=======
>   locale/@AB_CD@/messenger/preferences/security.dtd                     (%chrome/messenger/preferences/security.dtd)
>+>>>>>>> other
>   locale/@AB_CD@/messenger/preferences/junkLog.dtd                      (%chrome/messenger

Comment 66

[Chinmay Patel]

Attachment: Patch 10

Hopefully, this is exactly what I was asked for.

Comment 67

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v11

Chinmay: I played around with the patch, and fixed some minor style/whitespace/convention issue ... and some bugs. Can you try this out, and if it works for you, ask clarkbw to ui-r it?

 o loo wide keyword dialog
 o got rid of the "header" in the "Attachment Options" dialog
 o too many words on default list
 o localization note about the word list
 o add mail.compose.attachment_reminder to mail/app/profile/all-thunderbird.js , default to true
 o removed from attachmentReminder.xul
#ifdef XP_MACOSX
<?xml-stylesheet href="chrome://messenger/skin/preferences/preferences.css"?>
#endif
 o was allowd to add empty keyword
 o remove empty defaultKeyword from properties file
 o attachmentReminder.accesskey accesskey was already taken (and fixed case sensitivity for attachmentOptions.accesskey)
 o move the pref up in the options dialog (now it was in the "HTML" section, kind of)
 o if you removed all keywords, it always reminded...
 o renamed m- variables to not have "m", wan't quite doing the right thing without "this" for some of the vars (as per prevous review comment)
 o don't check if the word is in the quoted text (replies) - only works for html mode though
 o keyword dialog: should save only on ok, cancel should cancel

Seems to work well for me, and I'd r+ it, but since I poked around so much my self, would be good if you have some other reviewer take at least a quick look at it too.

Comment 68

[Chinmay Patel]

Attachment: Patch 11

First of all, thank you very much for working along with me.

I tried the patch and everything is good. 
Especially,I like the idea of not checking the keywords, if the word is in the quoted text.

Herewith, I am asking for ui review to the Bryan Clark.

Comment 69

[Magnus Melin [:mkmelin]]

Comment on attachment 373155 [details] [diff] [review]
Patch 11

From my pov this is ok, yeah.

Comment 70

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 373155 [details] [diff] [review]
Patch 11

tried it out just now and it looks great!  Nice work

Comment 71

[Magnus Melin [:mkmelin]]

Comment on attachment 373155 [details] [diff] [review]
Patch 11

Mark, can you take at least a quick look at this as I poked the code a little too much for me to feel comfortable giving it a complete review.

Comment 72

[Mark Banner (:standard8)]

Comment on attachment 373155 [details] [diff] [review]
Patch 11


>+          for (var i = 0; i < blockquotes.length; i++)
>+          {
>+            blockquotes[i].parentNode.removeChild(blockquotes[i]);
>+          }
>+          var mailData = mailBodyNode.innerHTML;
>+          var keywordFound;
>+          for (var i = 0; i < keywordsArray.length && !keywordFound; i++)
>+          {
>+            var re = new RegExp(keywordsArray[i], "i");
>+            keywordFound = re.exec(mailData);
>+          }

These loops should be changed to "for (let i = 0...." to restrict the scope of i to the loop otherwise you're essentially redeclaring the variable.

>+#### Attachment Reminder
>+attchmentReminderAddDialogTitle=Add Keyword
>+attchmentReminderAddText=Keyword:
>+attchmentReminderEditDialogTitle=Edit Keyword
>+attchmentReminderEditText=Keyword:

Any reason for the incorrect spelling or just a mistake?

r=Standard8 with the for loops and spelling fixed (unless there's a reason, in which case I'd like to know and review).

Comment 73

[Magnus Melin [:mkmelin]]

I've landed this patch, with comment 72 addressed.

changeset:   2595:d6977a2fb36c
http://hg.mozilla.org/comm-central/rev/d6977a2fb36c

->FIXED

Comment 74

[Steffen Wilberg]

Cool stuff, thanks!

Comment 75

[u60234]

Comment on attachment 373155 [details] [diff] [review]
Patch 11

>new file mode 100644
>--- /dev/null
>+++ b/mail/locales/en-US/chrome/messenger/preferences/attachmentReminder.dtd
>@@ -0,0 +1,8 @@

>+<!ENTITY attachKeywordText.label          "&brandShortName; will warn you about missing attachments if the you're about to send an e-mail containing one of these keywords.">

Nit: There is a superfluous "the" in the middle of that sentence.

Comment 76

[Magnus Melin [:mkmelin]]

Thx Hasse - i've checked in a fix for that

changeset:   2598:973cc9ed8b3f
http://hg.mozilla.org/comm-central/rev/973cc9ed8b3f

Comment 77

[Nomis101]

I found out today that this feature doesn't work for me (3.1a1pre) with an installed Enigmail. After deactivating Enigmail this feature workes perfect. So if anybody get into the same problem, than try to deactivate Enigmail first.

Comment 78

[IU]

This feature needs refinement.  It should trigger only for keywords in what is composed.  Apparently it is also triggering both for keywords in the messages responded to (reply/forward) as well as keywords appearing in the HTML code.  Discussion here: http://forums.mozillazine.org/viewtopic.php?p=6458935#p6458935

Comment 79

[Joe Sabash [:JoeS1]]

It seems any hit on "doc" in the image source triggers the keyword found path.
Since all replies to messages in a Winxp profiles contain the text:
mailbox:///C|/Documents%20and%20Settings/....
This is a showstopper for this feature.
It's not just the mailbox uri that's affected.
If I create a folder like c:\doc, and insert an image that results in:
file:///C:/doc/1h.gif as an image src,
That too triggers the false detection.

Comment 80

[Ben Bucksch (:BenB)]

> It seems any hit on "doc" in the image source triggers

From patch 11:

+mail.compose.attachment_reminder_keywords=attachment,attached,.doc,.pdf,resume,cover letter

It would trigger on ".doc", not on merely "doc".

I still think that list should not contain "attached" and "resume", as these are normal language words ("I resumed the computer", "I was so attached to this girl..." - TB: "Did you forget to add an attachment?"). Also, you can only trigger on ".pdf", if you have code to not trigger on URLs, as .pdf very often appears in URLs, and we do *not* want to encourage people to attach PDFs which can just as well be linked, rather the opposite.

Please remove everything apart from "attachment" and "cover letter", until URLs don't trigger it.

Comment 81

[Joe Sabash [:JoeS1]]

(In reply to comment #80)
> > It seems any hit on "doc" in the image source triggers
> 
> From patch 11:
> 
> +mail.compose.attachment_reminder_keywords=attachment,attached,.doc,.pdf,resume,cover
> letter
> 
> It would trigger on ".doc", not on merely "doc".

shoulda, woulda, coulda but in fact does trigger on "doc" and not ".doc"
 
> I still think that list should not contain "attached" and "resume", as these
> are normal language words ("I resumed the computer", "I was so attached to this
> girl..." - TB: "Did you forget to add an attachment?"). Also, you can only
> trigger on ".pdf", if you have code to not trigger on URLs, as .pdf very often
> appears in URLs, and we do *not* want to encourage people to attach PDFs which
> can just as well be linked, rather the opposite.
> 
> Please remove everything apart from "attachment" and "cover letter", until URLs
> don't trigger it.

I agree that that keywords are too broad, but then again they are editable in the pref.
"enclosure" is one that should be there IMO but again, prefs are editable.

Comment 82

[Phil Ringnalda (:philor)]

It splits on commas, and then creates a regexp from the pieces, including the ".doc" piece that matches any character followed by doc.

But, and I can't believe this is the first time you've heard this, if a bug is fixed but made a mistake in the fix, file a new bug and mark it as blocking the fixed bug, don't reopen; if a bug is fixed but you don't like the way it was fixed, file a new bug...

Comment 83

[Joe Sabash [:JoeS1]]

Notwithstanding the fact that I did not reopen this bug.
And irregardless of the fact that sometimes "new" bugs aren't really necessary,
I filed bug 492485

Comment 84

[Ben Bucksch (:BenB)]

philor, when a *serious* mistake happened in a bugfix, and it's known that this is cause by a *recent* checkin, the relevant bug is reopened, not a new bug filed.
But I filed bug 492555 now.

Comment 85

[Phil Ringnalda (:philor)]

No.

If the fix doesn't at all do what it intended (if this patch _never_ warned for any of the strings it tries to match), it's reopened. That's the single case where this bug should just be reopened.

If the fix does something wrong, you file a new bug blocking it, and if it's something serious enough and hard enough to fix (which is not true for either of these problems) to back it out, then it's backed out, the new bug is "fixed by backout" and the original is reopened.

Comment 86

[Ben Bucksch (:BenB)]

Yes. If a fix is so bad that it needs to be backed out, or needs a followup patch to avoid being backed out, the bug is reopened. I saw this many times over the years. I believe that's the case here, although that point is arguable. There's no point in arguing, though, that's why I just filed a new bug.

Comment 87

[[e-mail]]

I just started using this. Very nice work. One comment I have is that the icons in the buttons are backwards (I think). 

It asks the question: "Did you forget to add an attachment?" 

And it has two buttons: "Oh, I did!" and "No, Send Now."

From my perspective, the positive answer to the question should have the check mark. In other words, Did you forget the attachment? Yes, I did. Check. No I didn't, red sign.

Seems confusing in either case though.

Comment 88

[[e-mail]]

Attachment: Example of the buttons seeming backwards.