Closed Bug 944643 Opened 10 years ago Closed 10 years ago

Automatic attachment reminder should check for attachment keywords in message subject, too

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 30.0

People

(Reporter: thomas8, Assigned: sshagarwal)

References

()

Details

(Whiteboard: [gs][good first bug])

Attachments

(2 files, 10 obsolete files)

6.14 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
7.76 KB, patch
sshagarwal
: review+
sshagarwal
: ui-review+
sshagarwal
: feedback+
Details | Diff | Splinter Review
Attachment reminder's notification bar should also show up when user types attachment keywords into the *subject* field. Some users will just type "The documents" or "Scans as requested" into subject and might still forget to add their attachments in the rush if there's no reminder. I think that's a really nice idea (seen here: https://getsatisfaction.com/mozilla_messaging/topics/i_forgot_the_attachment_please_remind_me).

STR

1 compose new msg
2 type "Requested documents attached" into subject
3 send

Actual result

no attachment reminder notification bar
no attachment reminder alert at send time

Expected result

attachment reminder notification bar should show up
if notification not dismissed, and conditions still apply (as cleaned up per bug 521158), users should also see the alert as usual
Whiteboard: [gs]
See http://hg.mozilla.org/comm-central/file/a60a4c8f9a83/mail/components/compose/content/MsgComposeCommands.js#l1875 for which parts of the message are sent to the parser to check for keywords. Should be easy to add subject field in addition to body.
(In reply to :aceman from comment #1)
> See
> http://hg.mozilla.org/comm-central/file/a60a4c8f9a83/mail/components/compose/
> content/MsgComposeCommands.js#l1875 for which parts of the message are sent
> to the parser to check for keywords. Should be easy to add subject field in
> addition to body.

I take that as feedback+ for the idea ;)

This is where the maildata which gets ultimately searched (after some more tweaking) gets set (and where subject could be added):
http://hg.mozilla.org/comm-central/file/a60a4c8f9a83/mail/components/compose/content/MsgComposeCommands.js#l1914
Whiteboard: [gs] → [gs][good first bug][has code starting point]
Yes, it seems useful to me. There are people that only fill in the subject :)
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Attachment #8344156 - Flags: feedback?(bugzilla2007)
Attachment #8344156 - Flags: feedback?(acelists)
Comment on attachment 8344156 [details] [diff] [review]
Patch v1

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

Lovely!

But I'm not sure about the need for existing and proposed listeners. Maybe we can just remove them.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2005,4 @@
>      document.documentElement.setAttribute("screenY", screen.availTop);
>    }
>  
>    var identityList = document.getElementById("msgIdentity");

While we are here, let's change this to "let", and move this orphaned line down to be right above current line 2008 starting with
> if (identityList)

@@ +2007,5 @@
>  
>    var identityList = document.getElementById("msgIdentity");
>  
>    document.addEventListener("keypress", awDocumentKeyPress, true);
>    var contentFrame = document.getElementById("content-frame");

let

@@ +2010,5 @@
>    document.addEventListener("keypress", awDocumentKeyPress, true);
>    var contentFrame = document.getElementById("content-frame");
>    contentFrame.addEventListener("click", CheckForAttachmentNotification, true);
> +  let subjectField = document.getElementById("msgSubject");
> +  subjectField.addEventListener("click", CheckForAttachmentNotification, true);

I understand this is consistent with the click event listener for content frame, but I don't understand the purpose of that listener:

Looking at gAttachmentNotifier and its functions, CheckForAttachmentNotification will be called every 500 milliseconds based on a permanent timer [1].

So what's the point of having an extra listener just for the "click" event (but not for any other events that change text)? From my reading of the code, we could remove that click listener even for contentFrame, and it wouldn't change the behaviour at all. Pls correct me if I'm wrong, and pls enlighten me on the potential purposes of these listeners in addition to timer-controlled CheckForAttachmentNotification.

[1] http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4542

@@ +2011,5 @@
>    var contentFrame = document.getElementById("content-frame");
>    contentFrame.addEventListener("click", CheckForAttachmentNotification, true);
> +  let subjectField = document.getElementById("msgSubject");
> +  subjectField.addEventListener("click", CheckForAttachmentNotification, true);
> +  subjectField.addEventListener("keypress", CheckForAttachmentNotification, true);

dito. The timer-based cycles should catch this (because timer calls CheckForAttachmentNotification every 500 milliseconds, which seems reasonable enough for human inputs, and after this patch, we also check subject from inside CheckForAttachmentNotification).

If we really need these listeners (probably not), then it should be a listener for something like "change" event if such exists (event which fires every time the text changes, regardless if text was changed via click, keypress, copy & paste, drag & drop etc.). But since contentFrame doesn't have a keypress listener (afasics), why should subject need one?
Attachment #8344156 - Flags: feedback?(bugzilla2007) → feedback+
Comment on attachment 8344156 [details] [diff] [review]
Patch v1

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

The change looks right.
However there is a bug. If you write a keyword into the subject, then close the window (not saving or sending the message).
When you try to compose another message, the attachment notification is shown even when the windows it empty yet. Only after click into subject or body the notification goes away. This does not happen if the keyword is in the body. So there is some problem with this patch. Maybe when the window is recycled, it is first opened with the old contents of the fields, then you run attachment checker and only after that the fields are reinitialized to blank?

And you can add a test for this easily, into some of the existing ones :)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2010,5 @@
>    document.addEventListener("keypress", awDocumentKeyPress, true);
>    var contentFrame = document.getElementById("content-frame");
>    contentFrame.addEventListener("click", CheckForAttachmentNotification, true);
> +  let subjectField = document.getElementById("msgSubject");
> +  subjectField.addEventListener("click", CheckForAttachmentNotification, true);

The click listener on contentFrame is there since the attachment checker feature was last rewritten in bug 498519. I would keep it also for the subject. It looks cheap, because how many times do you click into the subject or the body?

@@ +2011,5 @@
>    var contentFrame = document.getElementById("content-frame");
>    contentFrame.addEventListener("click", CheckForAttachmentNotification, true);
> +  let subjectField = document.getElementById("msgSubject");
> +  subjectField.addEventListener("click", CheckForAttachmentNotification, true);
> +  subjectField.addEventListener("keypress", CheckForAttachmentNotification, true);

On the other hand, keypress listener is expensive, running all the checker on each new character is really not necessary. And it also makes the notification bar flash, e.g. when you write "attach" and then proceed to "attachm", it hides again and shows again when you get to "attachment". And there is no such keypress listener on the body.
Attachment #8344156 - Flags: feedback?(acelists) → feedback-
(In reply to :aceman from comment #6)
> Comment on attachment 8344156 [details] [diff] [review]
> Patch v1
> Review of attachment 8344156 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +2010,5 @@
> >    document.addEventListener("keypress", awDocumentKeyPress, true);
> >    var contentFrame = document.getElementById("content-frame");
> >    contentFrame.addEventListener("click", CheckForAttachmentNotification, true);
> > +  let subjectField = document.getElementById("msgSubject");
> > +  subjectField.addEventListener("click", CheckForAttachmentNotification, true);
> 
> The click listener on contentFrame is there since the attachment checker
> feature was last rewritten in bug 498519. I would keep it also for the
> subject. It looks cheap, because how many times do you click into the
> subject or the body?

Well, but if users hardly ever click into subject or body, and if we run CheckForAttachmentNotification anyway every half second on timer, than what's the benefit of having that extra listener? Due to its frequency, check from timer will always almost exactly co-occur with check from click listener, with a maximum time difference of a quarter second between the two checks. Nobody will notice if the notification comes or goes a quarter of a second earlier, but every listener comes with a cost. And why just would we only listen for clicks which are unlikely anyway? This listener is useless (unless shown otherwise), and should be removed. I actually think it's just an unintended leftover from a previous pre-timer concept.
Also, how would a click possibly change the text content of the body or subject? So if clicks don't even change text content, and if timer listens for all body & subject content changes every half second, what does the "click" listener change/effect at all?
Then we need to ask mkmelin and bwinton, the author of bug 498519.
Flags: needinfo?(bwinton)
Attached patch Patch v2 (obsolete) — Splinter Review
Is this acceptable?
Attachment #8344156 - Attachment is obsolete: true
Attachment #8344223 - Flags: feedback?(acelists)
Comment on attachment 8344223 [details] [diff] [review]
Patch v2

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

OK, if this is what is necessary. I could find any ill effects now. Thanks.
Attachment #8344223 - Flags: feedback?(acelists) → feedback+
I could not :)
Comment on attachment 8344223 [details] [diff] [review]
Patch v2

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

This is going in the right direction, but on the wrong path ;)

It's good to observe subject with MutationObserver, but we should not follow the NSA model and observe the entire composition window when we're just after suspect "subject"... ;)

::: mail/components/compose/content/MsgComposeCommands.js
@@ -2002,5 @@
>    var identityList = document.getElementById("msgIdentity");
>  
>    document.addEventListener("keypress", awDocumentKeyPress, true);
> -  var contentFrame = document.getElementById("content-frame");
> -  contentFrame.addEventListener("click", CheckForAttachmentNotification, true);

Even though my understanding of the timer was not fully adequate in my previous comments, I still maintain that the click listener is not needed, so this change (removing click listener) looks good to me: All changes of body text will be caught 500 ms after the DOM mutation takes place, via MutationObserver [1] which for each mutation will trigger the one-off timer (TYPE_ONE_SHOT).

[1] https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

@@ +3402,5 @@
>  
>  function MsgComposeCloseWindow(recycleIt)
>  {
> +  CheckForAttachmentNotification.shouldFire = true; // Clearing any attachment notifications left
> +  CheckForAttachmentNotification(true);

I'm not sure about this change, but I don't know enough yet about reminder internals to judge.
First to clear (via shouldFire??) and then to check for attachment notification again looks strange.

@@ +4599,5 @@
>    enableInlineSpellCheck(getPref("mail.spellcheck.inline"));
> +  // Add the notifier to the entire compose window
> +  // so that the notification is triggered even if
> +  // the attachment keywords exist in the subject.
> +  gAttachmentNotifier.init(document);

Imo, this is calling for trouble. If document represents the XUL window, then this will observe and trigger CheckForAttachmentNotification() for the following DOM mutations on *all* elements of the *entire* compose window:

4547       attributes: true,
4548       childList: true,
4549       characterData: true,
4550       subtree: true,

That's a net far too wide and you'll never know what you might catch. Think of included child nodes where users can add text: Perhaps it will trigger when users enter "Attachment master <foo@bar.com>" into a recipient field...? Or addons which might add other nodes which would also be observed.

But the general idea looks good to me, we could and probably should use MutationObserver for the subject, too. Afasics, MutationObserver's "observe" method can observe *any* target node (see documentation [2]), not just documents, so you just want to observe the subject node in addition to the document.

[2] https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver#observe%28%29

Here's a draft idea of how to observe the subject (maybe there are more elegant solutions, but this should work):

4536   init: function gAN_init(aDocument) {

Change that line to

4536   init: function gAN_init(aDocument, aSubjectNode) {

Then observe the body and subject, which in principle needs something like this:

this._obs.observe(aDocument, {...
this._obs.observe(aSubjectNode, {...

However, probably you can't observe more than one node with the same MutationObserver, so you might have to create an additional observer for subject:

this._obsSubject = new MutationObserver(function gAN_handleSubjectMutations(aMutations) {...

this._obsSubject.observe(aSubjectNode, {...

Something along those lines. This is untested, just draft ideas from my reading of the code. No guarantees. Please do correct me where I'm wrong, I want to learn :)

(what's aMutations used for btw? don't see that being used anywhere)
Attachment #8344223 - Flags: feedback?(acelists)
Attachment #8344223 - Flags: feedback-
Attachment #8344223 - Flags: feedback+
(In reply to Thomas D. from comment #13)
> Comment on attachment 8344223 [details] [diff] [review]
> Patch v2
> [...]
> But the general idea looks good to me, we could and probably should use
> MutationObserver for the subject, too. Afasics, MutationObserver's "observe"
> method can observe *any* target node (see documentation [2]), not just
> documents, so you just want to observe the subject node in addition to the
> document.

That's supposed to say: "...in addition to the editor.document" (which is NOT the entire composition document).

And of course, to try the idea of comment 13, you'll also need to modify the call accordingly:

Something like

let subjectNode=getElementById...
4602   gAttachmentNotifier.init(editor.document, subjectNode);
I would agree with only attaching the listener to body and subject field. However, the listener wants a document object to listen to. So the quick workaround was to use window.document. If anybody can hint how to also listen to the subject field, that would be great.
Comment on attachment 8344223 [details] [diff] [review]
Patch v2

I agree with Thomas D., however it is not so easy to find the technical solution for the problem. Maybe mkmelin will know how to attach the listener only to the wanted fields.
Attachment #8344223 - Flags: feedback?(acelists) → feedback?(mkmelin+mozilla)
(In reply to :aceman from comment #15)
> I would agree with only attaching the listener to body and subject field.
> However, the listener wants a document object to listen to.

Unless the documentation is wrong, that's not true, as I already explained in Comment 13 including reference to documentation:

From https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver#observe%28%29:

> observe()
> 
> Registers the MutationObserver instance to receive notifications of DOM mutations on the specified
> node.
>
> void observe(
>  Node target,
>  MutationObserverInit options
> );

No hints that MutationObserver.observe function is limited to documents. It's only the customized function made by :bwinton where the home-made *variable name* suggests that aDocument should be passed...

init: function gAN_init(aDocument)

...which was the only thing to pass at the time, and the MutationObserverInit options used in :bwinton's function are adjusted to that, too.

> If anybody can hint how to also listen to the subject field, that would be great.

Now we're adding subject for observation, and I have already hinted/outlined in Comment 13 how to make gAttachmentNotifier.init function accept two arguments so that we can pass two arguments when we call that:

4536   init: function gAN_init(aDocument, aSubjectNode) 

After that, just create *two* MutationObservers:

this._obsDocument = new MutationObserver(...
this._obsSubject = new MutationObserver(... verify which MutationObserverInit options are needed for subject, but I'd just try the same because you never know how addons might add nested nodes.

Then let each observer observe its own node:

this._obsDocument.observe(aDocument, {...
this._obsSubject.observe(aSubjectNode, {...

I'd think it's impossible that mutations on subject and body can happen simultaneously, so there shouldn't be a problem about both observers calling the same function, CheckForAttachmentNotification.
Even if subject and body would call that function with just a tiny difference in time, it will work because previous runs are cancelled before starting new runs. If they would call the same function at exactly the same millisecond in time, which I think is not possible, I'm not sure what would happen.

There might be more elegant ways, but I don't see technical problems with that solution. Pls let me know if there are.
Comment on attachment 8344223 [details] [diff] [review]
Patch v2

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

Yeah you can probably be more specific about which nodes to observe, like Thomas D. wrote
Attachment #8344223 - Flags: feedback?(mkmelin+mozilla)
Another aspect which we should probably consider/verify:

For replies and forwarding of messages, the existing subject should not be checked. (If somebody sends me mail with subject "Your documents attached", and I reply, it doesn't mean that *I* want to add attachments, so it shouldn't show the notification).

Edit as new, not sure, perhaps it's ok to check subject there bc that's a fresh composition created by user himself.
Good idea. Maybe we can access which value from Components.interfaces.nsIMsgCompType is the current composition using.
Attached patch Patch v3 (obsolete) — Splinter Review
Okay, so implemented the observer.
But, unless we enter the body, subject keywords aren't doing the magic. So, if it isn't too expensive, can we add the keypress event listener for the subject field?
Attachment #8344223 - Attachment is obsolete: true
Attachment #8344813 - Flags: review?(mkmelin+mozilla)
Attachment #8344813 - Flags: feedback?(bugzilla2007)
Attachment #8344813 - Flags: feedback?(acelists)
Attached patch Patch v3.1 (obsolete) — Splinter Review
Adding an event listener for the subject field for the use case:
if the user only adds the subject to the mail and adds nothing to the body, the notification will not be shown unless the user explicitly enters the body, which, will not happen in this case.

Moreover, there are ways of changing the text in a field which don't involve key presses so adding the "input" type of event listener instead of "keypress" type.
Thanks to Neil sir for this thought.

I hope this is acceptable now.
Attachment #8344813 - Attachment is obsolete: true
Attachment #8344813 - Flags: review?(mkmelin+mozilla)
Attachment #8344813 - Flags: feedback?(bugzilla2007)
Attachment #8344813 - Flags: feedback?(acelists)
Attachment #8345287 - Flags: feedback?(bugzilla2007)
Attachment #8345287 - Flags: feedback?(acelists)
Comment on attachment 8345287 [details] [diff] [review]
Patch v3.1

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1932,5 @@
> +    // Prepend the subject to see if the subject contains any attachment
> +    // keyword too, after making sure that the message isn't a reply or a
> +    // forwarded message (Bug 944643).
> +    let subject = document.getElementById("msgSubject").value;
> +    if (subject && !subject.contains("Fwd:") && !subject.contains("Re:"))

So can't this be retrieved from the composition type instead of checking the text for hardcoded values?

@@ +2018,5 @@
>    document.addEventListener("keypress", awDocumentKeyPress, true);
> +  // Add an input event listener for the subject field since there
> +  // are ways of changing its value without key presses.
> +  document.getElementById("msgSubject")
> +          .addEventListener("input", CheckForAttachmentNotification, true);

Any idea why the observer doesn't work on the field the same way it works on the body?
Comment on attachment 8345287 [details] [diff] [review]
Patch v3.1

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

If we want this to be polished, this still needs a lot of creative thinking about different scenarios and edge cases.
- Input listener will not be needed and too resourceful if we make mutationobserver work (see my suggestion below), and either of them should be on timer function. 
- There's a bug in the existing implementation already (not introduced by this patch) that the status of auto-reminder is not preserved between close and reopen of draft (probably save in draft source as we did for manual reminder)
- Hardcoded Re: and Fwd: looks bad and needs finetuning, too, for polishing. User changes even of Re:-subjects should be monitored. We should go through all scenarios incl. edit as new and define how we expect them to work.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1932,5 @@
> +    // Prepend the subject to see if the subject contains any attachment
> +    // keyword too, after making sure that the message isn't a reply or a
> +    // forwarded message (Bug 944643).
> +    let subject = document.getElementById("msgSubject").value;
> +    if (subject && !subject.contains("Fwd:") && !subject.contains("Re:"))

As aceman said, hardcoded values will probably be a problem. If anything, they should be variables for the localized version. But that's still not very good. Composition type might be better.

There's something else which is not fully correct about this if-condition:

You exclude the subject from checking just based on the fact that it is an answer/forward. But ideally it should only be ignored if it's still the *original* subject; whereas any *changes* to the subject should be monitored. E.g., when the original subject is "Call for papers", we should ignore "Re: Call for papers"; but if user expands that subject to become "Re: Call for papers - see attachment", that change should be monitored and the notification should show.

For more correctness, another nit: when the notification shows bc. I've changed subject and added a keyword, when I save the draft without closing notification and reopen, the notification should be restored; but if I closed the notification before saving draft, it should not come back. That's a bug in the existing implementation already:

1 type attachment keyword, close notification;
2 type another attachment keyword; close notification;
3 close draft; reopen draft;
-> attachment reminder returns but should not, we should always restore exactly the state at the time of saving.

I suspect that to get that right, we'll ultimately need another flag in the X-Mozilla... header where we put attachmentreminder=1|0):
autoattachmentreminder=...

But that still won't get it right because in the live version, we're actually watching keywords and ignored keywords continue to be ignored and strictly speaking that status should also be preserved upon close/reopen of draft. Which sounds like a new bug, if anything. Perfection is a can of worms; perhaps we can get away with less...

For a basic version of this feature, we might get away with just ignoring any replies and forwarded subjects even when they are changed, and notifiying too little is imo/arguably better than notifying too much. But in the medium/long run we should try to improve the algorithm to get it right, if possible. I don't have time right now to think about all the details, but I feel this needs more thoughts about the various scenarios and edge cases.

@@ +2018,5 @@
>    document.addEventListener("keypress", awDocumentKeyPress, true);
> +  // Add an input event listener for the subject field since there
> +  // are ways of changing its value without key presses.
> +  document.getElementById("msgSubject")
> +          .addEventListener("input", CheckForAttachmentNotification, true);

If you type several words fast without interruption, does input event fire on every letter or just once for the whole string of words?

the input listener should not be necessary if we get mutationobserver to work (and I think I know how). But even if we kept input listener, we would have to call timer function (as in the mutation observer) and not checkforattachmentnotification directly, because that will trigger the check too often, cause flickering of notification etc.

@@ +4537,5 @@
>  {
>    _obs: null,
> +  _subjectObs: null,
> +
> +  init: function gAN_init(aDocument, aElement) {

This is confusing. Both arguments are just nodes, so a neutral notation would be (aNode1, aNode1) or (aElement1, aElement2). But actually, that's not true either. This function has not been designed to just be called with any nodes. Our inner functions are specifically named and designed for editor.document and msgSubject (e.g. setting the right observer parameters for each of these particular nodes). So either we keep the whole function neutral including inside, or the whole function specific. I think specific is better, so I think this function should be something like
init: function gAN_init(aEditorDocument,aSubjectNode)

@@ +4560,5 @@
>        subtree: true,
>      });
> +    this._subjectObs.observe(aElement, {
> +      attributes: true,
> +      characterData: true,

MutationObserver for subject probably fails because these options just check the toplevel of <textbox id="msgSubject">, but the actual subject text, as seen in DomInspector, is several nested nodes deeper down in the DOM tree:

textbox id=msgSubject
- xul:hbox
  - html:input
    - div
      - #text  <-- this is where the subject string really is, so that's the grandchild node of textbox which we need to observe...

Iow, just include childList and subtree in observer options and it should work (shouldn't subtree have camelCase?). We might not need them both, but I'd rather add both because you never know if addons might insert children or subtrees for colored subject words or whatever and we'd still want to check changes to all of these, probably.
Attachment #8345287 - Flags: feedback?(bugzilla2007) → feedback-
FTR:

I understand this is how MutationObserver currently works:
It will fire for every character that you type, because that's a mutation.
for each character, it first cancels any existing timer call and then calls CheckForAttachmentNotification with a 500ms timer delay.
When you type characters fast without interruption, it will fire for each character but at the same time cancel and restart 500ms timer for each new character. The net effect is that you will only see notification if you pause typing for 500ms.
(In reply to Thomas D. from comment #24)
> Comment on attachment 8345287 [details] [diff] [review]
> Patch v3.1
> 
> Review of attachment 8345287 [details] [diff] [review]:

> MutationObserver for subject probably fails because these options just check
> the toplevel of <textbox id="msgSubject">, but the actual subject text, as
> seen in DomInspector, is several nested nodes deeper down in the DOM tree:
> 
> textbox id=msgSubject
> - xul:hbox
>   - html:input
>     - div
>       - #text  <-- this is where the subject string really is, so that's the
> grandchild node of textbox which we need to observe...
> 
> Iow, just include childList and subtree in observer options and it should
> work (shouldn't subtree have camelCase?). We might not need them both, but
> I'd rather add both because you never know if addons might insert children
> or subtrees for colored subject words or whatever and we'd still want to
> check changes to all of these, probably.

Yes, I had tried this and it didn't work.
(In reply to Thomas D. from comment #25)
> FTR:
> 
> I understand this is how MutationObserver currently works:
> It will fire for every character that you type, because that's a mutation.
> for each character, it first cancels any existing timer call and then calls
> CheckForAttachmentNotification with a 500ms timer delay.
> When you type characters fast without interruption, it will fire for each
> character but at the same time cancel and restart 500ms timer for each new
> character. The net effect is that you will only see notification if you
> pause typing for 500ms.

Then, what is the problem with event listener? Provided we are assuming that the user isn't going to play with the subject field :P

And, I confirmed this with mkmelin that event listener for the subject field isn't expensive.
(In reply to Suyash Agarwal (:sshagarwal) will be away from 14th December from comment #27)
> (In reply to Thomas D. from comment #25)
> > FTR:
> > 
> > I understand this is how MutationObserver currently works:
> > It will fire for every character that you type, because that's a mutation.
> > for each character, it first cancels any existing timer call and then calls
> > CheckForAttachmentNotification with a 500ms timer delay.
> > When you type characters fast without interruption, it will fire for each
> > character but at the same time cancel and restart 500ms timer for each new
> > character. The net effect is that you will only see notification if you
> > pause typing for 500ms.
> 
> Then, what is the problem with event listener? Provided we are assuming that
> the user isn't going to play with the subject field :P

The problem with *your* event listener was that it bypasses the timer.

> And, I confirmed this with mkmelin that event listener for the subject field
> isn't expensive.

It's not so much a problem of performance, it's UX-consistency:
The timer is there for a reason, namely to avoid flickering bar for premature matches.
Say user writes "www.doctors.com": Without timer, notification bar will appear instantly after "www.doc" because ".doc" is an attachment keyword. After typing the rest of the word ("tors.com"), notification will disappear again. That's undesirable and unprofessional. Timer avoids that because most people will just type www.doctors.com in one go and the timer makes it so that only after you pause for 500ms, words will be checked (so in this case, the bar would correctly not appear at all, instead of playing hide and seek with user).

So if we really give up on subject mutation observer (I don't understand why it shouldn't work?), you need copy an adapted version of timer function (as seen in gAttachmentNotifier) into your listener, so that CheckForAttachmentNotification gets called timer-based (practically after each word) and not immediately/repeatedly (for each character).
And to find the bug why it doesn't work for subject, you should find out how come that gAttachmentNotifier fires when just tabbing into body as you reported.
(In reply to Thomas D. from comment #28)
> (In reply to Suyash Agarwal (:sshagarwal) will be away from 14th December
> from comment #27)
> > And, I confirmed this with mkmelin that event listener for the subject field
> > isn't expensive.

The event listener itself might not be expensive wrt performance, but calling CheckForAttachmentNotification (without timer-delay) is more expensive, because for each character typed in subject it will check the subject AND the entire body for keywords. I suppose it does make a difference if for typing "attachment" in subject, whole body check is done only *once* (with listener + timer before check), or 10x (without timer), for each of the 10 characters of "attachment".
Attached patch Patch v4 (obsolete) — Splinter Review
Okay so this works.
Actually I wanted to write a lot taking something from every comment, but then decided to keep it short :)
gSubjectChanged: I couldn't get to the actual code source where gContentChanged gets set to true for subject field changes so I am setting this variable in a related function (I hope this is acceptable). Actually I wanted to have such variables for every field so that we have better control on every field and can revert the entire window properly but that is a different thing.

I couldn't get MutationObserver working for the subject field, possibly because of anonymous nodes. 

One last note, since we are monitoring whether the subject field is changed or not, I thing this rules out the need for checking of replying or forwarding intent of composition, so I left that (check) out. If it is still required please let me know.

Thanks.
Attachment #8345287 - Attachment is obsolete: true
Attachment #8345287 - Flags: feedback?(acelists)
Attachment #8351712 - Flags: feedback?(bugzilla2007)
Attachment #8351712 - Flags: feedback?(acelists)
Comment on attachment 8351712 [details] [diff] [review]
Patch v4

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

Much better, but I don't think this addresses all of my comments above.

For example, what about automatical reminder status persistance?

STR

1) compose new msg
2) type "attachment" into subject
-> reminder notification shows; don't touch it (so it's present at the time of closing draft)
3) save draft
4) close and reopen draft

Actual result
- subject-triggered reminder notification was present at the time of closing, but it's gone after reopening same draft (bug)
- it doesn't return because we only listen for changes but after reopen there's no change of subject

Expected result
- reminder status should be exactly the same as at the time of closing
- at least we should be consistent between automatical reminder regardless if triggered by body and subject (with current patch, body status will be rechecked from scratch and body reminder will reappear, but subject reminder won't).

I still think that this needs much more detail and analysis of workflows to get it right...

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4119,5 @@
>  }
>  
>  function subjectKeyPress(event)
>  {
> +  gSubjectChanged = true;

This looks wrong, because keypresses like tab, cursor keys, page down, enter, home, end etc. will also be considered a change of subject while there's no change. Perhaps you can compare old and new subject string.

@@ +4570,5 @@
>    },
>  
> +  // Timer based function triggered by the inputEventListener
> +  // for the subject field.
> +  subjectObserver: function handleEvent() {

That's a lot better/resource-saving than before and I think it's acceptable (if the current assumptions on what should trigger this are right; not so sure).

I'd still be interested if you tried the subject mutation observer with included observer options of childList AND subtree AND characterData (all of them are needed), and if that really still failed.
Attachment #8351712 - Flags: feedback?(bugzilla2007)
Attached patch Patch v4.2 (obsolete) — Splinter Review
(In reply to Thomas D. (away till 31st January) from comment #32)
> Comment on attachment 8351712 [details] [diff] [review]
> Patch v4

> what about automatical reminder status persistance?
> Expected result
> - reminder status should be exactly the same as at the time of closing
> - at least we should be consistent between automatical reminder regardless
> if triggered by body and subject (with current patch, body status will be
> rechecked from scratch and body reminder will reappear, but subject reminder
> won't).
Done!
> >  function subjectKeyPress(event)
> >  {
> > +  gSubjectChanged = true;
> This looks wrong, because keypresses like tab, cursor keys, page down,
> enter, home, end etc. will also be considered a change of subject while
> there's no change. Perhaps you can compare old and new subject string.
It didn't trigger the notification on arrow key presses, page navigation keys, tabs and home, enter. But tab presses in the field will create a whitespace change which will change the value of gContentChanged, so I am changing gSubjectChanged as well. I think that should be considered, rest is what you decide, I can trim the subject if desired.

> I'd still be interested if you tried the subject mutation observer with
> included observer options of childList AND subtree AND characterData (all of
> them are needed), and if that really still failed.
I tried this, it didn't work. Please let me know if I missed something again.

Thanks.
Attachment #8351712 - Attachment is obsolete: true
Attachment #8351712 - Flags: feedback?(acelists)
Attachment #8354999 - Flags: feedback?(acelists)
(The conversation seems to have progressed quite a bit since I was asked for info.  Do we still need me to chime in?  Is there a specific question I should answer?)
Comment on attachment 8354999 [details] [diff] [review]
Patch v4.2

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

This works fine for me.
I just have some questions.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1927,5 @@
> +    // Prepend the subject to see if the subject contains any attachment
> +    // keyword too, after making sure that the subject has changed. - bug #944643
> +    let subject = GetMsgSubjectElement().value;
> +    if (subject && (gSubjectChanged || gEditingDraft))
> +      mailData = subject + " " + mailData;

I am not sure what gEditingDraft contains. However, this does prevent a replied message with a keyword in subject to be notified until the subject is written into. However, when you save an unchanged reply as Draft and reopen the notification comes up even when the subject was never changed. Is that intentional?

@@ +4565,5 @@
>    shutdown: function gAN_shutdown() {
>      if (this._obs)
>        this._obs.disconnect();
>  
>      this._obs = null;

Do we need to remove the timer/listener on subject here, like the listener for body is?

@@ +4572,5 @@
> +  // Timer based function triggered by the inputEventListener
> +  // for the subject field.
> +  subjectObserver: function handleEvent() {
> +    gAttachmentNotifier.timer.cancel();
> +    gAttachmentNotifier.timer.initWithCallback(gAttachmentNotifier.event, 500,

Why isn't this set up in the init function? Was that already tried?
Attachment #8354999 - Flags: feedback?(acelists) → feedback+
Attached patch Patch v5 (obsolete) — Splinter Review
(In reply to :aceman from comment #35)
> Comment on attachment 8354999 [details] [diff] [review]
> Patch v4.2
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +1927,5 @@
> > +    if (subject && (gSubjectChanged || gEditingDraft))
> > +      mailData = subject + " " + mailData;
> 
> I am not sure what gEditingDraft contains. 
I found it is true for editing drafts :)
> However, when you save an unchanged reply as Draft and
> reopen the notification comes up even when the subject was never changed. Is
> that intentional?
I don't think that will be a good thing to happen, so I have tried to fix it in this new patch.
> @@ +4565,5 @@
> >    shutdown: function gAN_shutdown() {
> >      if (this._obs)
> >        this._obs.disconnect();
> Do we need to remove the timer/listener on subject here, like the listener
> for body is?
I think _obs.disconnect() will cancel the timer(?). If that's not the case I think we can just put a call to timer.cancel() in the shutdown method to take care of that. If that's the case please let me know.
> @@ +4572,5 @@
> > +  // Timer based function triggered by the inputEventListener
> > +  // for the subject field.
> > +  subjectObserver: function handleEvent() {
> > +    gAttachmentNotifier.timer.cancel();
> > +    gAttachmentNotifier.timer.initWithCallback(gAttachmentNotifier.event, 500,
> 
> Why isn't this set up in the init function? Was that already tried?
Ya, I tried it and it didn't work (or I couldn't put it to work :( )
But this is fine isn't it? :) It works the same way.

Thanks.
Attachment #8354999 - Attachment is obsolete: true
Attachment #8359657 - Flags: ui-review?(bwinton)
Attachment #8359657 - Flags: review?(mkmelin+mozilla)
Attachment #8359657 - Flags: feedback?(acelists)
(In reply to Blake Winton (:bwinton) from comment #34)
> (The conversation seems to have progressed quite a bit since I was asked for
> info.  Do we still need me to chime in?  Is there a specific question I
> should answer?)

Ya, I think the issue was that I wasn't/am not able to put gAttachmentNotifier work for subject too.
Either the issue is anonymous ids or something else. But I put a substitute :) now, if it is still desired that the new subject input observer should work the same way as the body observer works (Mutation observer) please let me know what fields I am supposed to provide, I tried them all (childList, subTree.. etc. etc. on document.getElmentById("msgSubject")).

If there is something else that this needinfo was about, aceman would know :)

Thanks.
(In reply to Suyash Agarwal (:sshagarwal) from comment #36)
> > @@ +4565,5 @@
> > >    shutdown: function gAN_shutdown() {
> > >      if (this._obs)
> > >        this._obs.disconnect();
> > Do we need to remove the timer/listener on subject here, like the listener
> > for body is?
> I think _obs.disconnect() will cancel the timer(?). If that's not the case I
> think we can just put a call to timer.cancel() in the shutdown method to
> take care of that. If that's the case please let me know.
Yes, I want to somehow stop the timer for the subjectObserver because the body observer is stopped here. They are a bit different so if .cancel() is what is needed for subjectObserver instead of .disconnect() then that is fine.

> > @@ +4572,5 @@
> > > +  // Timer based function triggered by the inputEventListener
> > > +  // for the subject field.
> > > +  subjectObserver: function handleEvent() {
> > > +    gAttachmentNotifier.timer.cancel();
> > > +    gAttachmentNotifier.timer.initWithCallback(gAttachmentNotifier.event, 500,
> > 
> > Why isn't this set up in the init function? Was that already tried?
> Ya, I tried it and it didn't work (or I couldn't put it to work :( )
> But this is fine isn't it? :) It works the same way.
I find it inconsistent that one observer is started from .init and the other one is started from completely other part of code (ComposeStartup).
(In reply to :aceman from comment #38)
> (In reply to Suyash Agarwal (:sshagarwal) from comment #36)
> > I think _obs.disconnect() will cancel the timer(?). If that's not the case I
> > think we can just put a call to timer.cancel() in the shutdown method to
> > take care of that. If that's the case please let me know.
> Yes, I want to somehow stop the timer for the subjectObserver because the
> body observer is stopped here. They are a bit different so if .cancel() is
> what is needed for subjectObserver instead of .disconnect() then that is
> fine.
Done.
> > Ya, I tried it and it didn't work (or I couldn't put it to work :( )
> > But this is fine isn't it? :) It works the same way.
> I find it inconsistent that one observer is started from .init and the other
> one is started from completely other part of code (ComposeStartup).

Okay, so I can put the event listener for the subject field in gAttachmentNotifier.init(). Will that suffice?
Comment on attachment 8359657 [details] [diff] [review]
Patch v5

(It seems like you're going to be posting a new patch in the near future, so I'm clearing my ui-review request from this one.  But ping me when the new patch is posted, and I promise I'll be faster!)
Attachment #8359657 - Flags: ui-review?(bwinton)
Flags: needinfo?(bwinton)
Attached patch Patch v5.2 (obsolete) — Splinter Review
Okay so I have made the assumed changes.
Thanks.
Attachment #8359657 - Attachment is obsolete: true
Attachment #8359657 - Flags: review?(mkmelin+mozilla)
Attachment #8359657 - Flags: feedback?(acelists)
Attachment #8361036 - Flags: ui-review?(bwinton)
Attachment #8361036 - Flags: review?(mkmelin+mozilla)
Attachment #8361036 - Flags: feedback?(acelists)
Comment on attachment 8361036 [details] [diff] [review]
Patch v5.2

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

I read Mutation Observers are limited to observing the DOM serialized state, so indeed you can't use them for text fields.

Would be good with a test for this too, xref bug 939700.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1934,5 @@
>      if (fwdIndex > 0)
>        mailData = mailData.substring(0, fwdIndex);
>  
> +    // Prepend the subject to see if the subject contains any attachment
> +    // keywords too, after making sure that the subject has changed. - bug #944643

no need to mention bug numbers

@@ +1939,5 @@
> +    let subject = GetMsgSubjectElement().value;
> +    if (subject && (gSubjectChanged ||
> +       (gEditingDraft &&
> +         !(gComposeType == nsIMsgCompType.Reply ||
> +         gComposeType == nsIMsgCompType.ReplyToSender ||

This list isn't correct, probably we should just check for New, NewsPost and MailToUrl
http://mxr.mozilla.org/comm-central/source/mailnews/compose/public/nsIMsgComposeParams.idl#17
Attachment #8361036 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Patch v5.5 (obsolete) — Splinter Review
Okay, so changed from checking for negatives to positives.
Attachment #8361036 - Attachment is obsolete: true
Attachment #8361036 - Flags: ui-review?(bwinton)
Attachment #8361036 - Flags: feedback?(acelists)
Attachment #8362219 - Flags: ui-review?(bwinton)
Attachment #8362219 - Flags: review+
Attachment #8362219 - Flags: feedback?(acelists)
Comment on attachment 8362219 [details] [diff] [review]
Patch v5.5

I like it!

Thank you for the patch, Suyash!  :)
Attachment #8362219 - Flags: ui-review?(bwinton) → ui-review+
Aryx, please push both patches to Try server when it is no longer burning. Mozmill tests are enough, all platforms. Thanks
Attachment #8369658 - Flags: feedback?(archaeopteryx)
Comment on attachment 8362219 [details] [diff] [review]
Patch v5.5

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

Looks good to me and I added tests in a separate patch.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1933,5 @@
>      let fwdIndex = mailData.indexOf(fwdText);
>      if (fwdIndex > 0)
>        mailData = mailData.substring(0, fwdIndex);
>  
> +    // Prepend the subject to see if the subject contains any attachment

This part was bitrotted for me, there is a forward_header_originalmessage block inserted since.

@@ +4559,5 @@
>    init: function gAN_init(aDocument) {
> +    // Add an input event listener for the subject field since there
> +    // are ways of changing its value without key presses.
> +    GetMsgSubjectElement().addEventListener("input",
> +      this.subjectObserver, true);

Please put this under the this.shutdown() block below.

@@ +4581,5 @@
>  
>    shutdown: function gAN_shutdown() {
>      if (this._obs)
>        this._obs.disconnect();
> +    gAttachmentNotifier.timer.cancel();

Is existence of this._obs indicating both observers are running? If yes, put both cancellings into a block controlled by the if().
Attachment #8362219 - Flags: feedback?(acelists) → feedback+
Patch 5.5 fails to apply, please provide an updated version.
Flags: needinfo?(syshagarwal)
(In reply to :aceman from comment #46)
> Comment on attachment 8362219 [details] [diff] [review]
> Patch v5.5
> @@ +4559,5 @@
> >    init: function gAN_init(aDocument) {
> > +    // Add an input event listener for the subject field since there
> > +    // are ways of changing its value without key presses.
> > +    GetMsgSubjectElement().addEventListener("input",
> > +      this.subjectObserver, true);
> 
> Please put this under the this.shutdown() block below.
But I want it to start when the init function is called.
> @@ +4581,5 @@
> >  
> >    shutdown: function gAN_shutdown() {
> >      if (this._obs)
> >        this._obs.disconnect();
> > +    gAttachmentNotifier.timer.cancel();
> 
> Is existence of this._obs indicating both observers are running? 
> If yes, put both cancellings into a block controlled by the if().
No, I don't think so. Its just that whenever gAttachmentNotifier.shutdown() is called, I want the subject observer to cancel itself as well. That's it.

So, please let me know if these changes still need to be made.
Attached patch Patch v6 (obsolete) — Splinter Review
So, just the ordering of the code remains. Otherwise, the patch is ready to be pushed to try with the tests.

Thanks.
Attachment #8362219 - Attachment is obsolete: true
Attachment #8369967 - Flags: ui-review+
Attachment #8369967 - Flags: review+
Attachment #8369967 - Flags: feedback+
Flags: needinfo?(syshagarwal)
(In reply to Suyash Agarwal (:sshagarwal) from comment #48)
> (In reply to :aceman from comment #46)
> > Comment on attachment 8362219 [details] [diff] [review]
> > Patch v5.5
> > @@ +4559,5 @@
> > >    init: function gAN_init(aDocument) {
> > > +    // Add an input event listener for the subject field since there
> > > +    // are ways of changing its value without key presses.
> > > +    GetMsgSubjectElement().addEventListener("input",
> > > +      this.subjectObserver, true);
> > 
> > Please put this under the this.shutdown() block below.
> But I want it to start when the init function is called.
Yes, but the init function first cleans up, then starts the observers. I'd think starting the subject observer should be below the cleanup together with the start of the body observer.

> > @@ +4581,5 @@
> > >  
> > >    shutdown: function gAN_shutdown() {
> > >      if (this._obs)
> > >        this._obs.disconnect();
> > > +    gAttachmentNotifier.timer.cancel();
> > 
> > Is existence of this._obs indicating both observers are running? 
> > If yes, put both cancellings into a block controlled by the if().
> No, I don't think so. Its just that whenever gAttachmentNotifier.shutdown()
> is called, I want the subject observer to cancel itself as well. That's it.

How do you know if there is any thing to cancel? If you do not use this._obs, which variable indicates the timer is scheduled?

I think the timer is started from
this._obs = new MutationObserver(function gAN_handleMutations(aMutations) {
gAttachmentNotifier.timer.initWithCallback()}

So it is bound to (this._obs != null).
(In reply to :aceman from comment #50)
> Yes, but the init function first cleans up, then starts the observers. I'd
> think starting the subject observer should be below the cleanup together
> with the start of the body observer.
Ya, my mistake. Moved it at the end of init().
> How do you know if there is any thing to cancel? If you do not use
> this._obs, which variable indicates the timer is scheduled?
> 
> I think the timer is started from
> this._obs = new MutationObserver(function gAN_handleMutations(aMutations) {
> gAttachmentNotifier.timer.initWithCallback()}
> 
> So it is bound to (this._obs != null).

I tried to check at a number of different places, but _obs was null everywhere. Moreover, I think, timer is different, in shutdown(), we are just disconnecting this._obs (stopping the mutation observer so that callback is not invoked), so if we are initiating the timer with timer.initWithCallback() via subjectObserver, I don't think this._obs is involved in any way; so if the timer was invoked via subjectObserver, this._obs can be cannot be null (I think so) and we will need to handle the timer for this separately.

Rest I am not sure, maybe the value of _obs at different places in the method can give an idea (which is coming out to be null for me).

Thanks.
(In reply to Archaeopteryx [:aryx] from comment #51)
> Pushed to Try as
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=8c92a15039c1

Looks like the tests passed. I did some invasive header removal in keyboard-helpers.js (looked like unneeded code) so wanted to see if it does not break any other tests.
Attachment #8369658 - Flags: feedback?(archaeopteryx) → review?(mkmelin+mozilla)
Aren't they orange on Linux?
Yes, but those failures do have their own bugs and happen on TB-trunk too (since today). Also standard8 commented in those bugs about the cause of the failures so it is a real problem not caused by our patches that are only in TB-try.
Whiteboard: [gs][good first bug][has code starting point] → [gs][good first bug]
Great! Thanks for the tests :)
Attachment #8369658 - Flags: review?(mkmelin+mozilla) → review+
(In discussion with :aceman from comment #50)
Does my comment #52 answer your queries or changes are to be made in this patch?
If not, please let me know; if yes, we can close this down :)

Thanks.
I think you need to upload the final version after comment 52. I do not fell strongly about the rest of my nits and mkmelin is fine with the patch as is. So after the final upload it can be checked in.
Attached patch Patch v7Splinter Review
Okay, made the changes as summarized in comment 52.
Attachment #8369967 - Attachment is obsolete: true
Attachment #8375696 - Flags: ui-review+
Attachment #8375696 - Flags: review+
Attachment #8375696 - Flags: feedback+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d2c11a99b1e1
https://hg.mozilla.org/comm-central/rev/bd6fd97114aa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
Blocks: 994655
Summary: Automatical attachment reminder should check for attachment keywords in message subject, too → Automatic attachment reminder should check for attachment keywords in message subject, too
You need to log in before you can comment on or make changes to this bug.