1099866 - Not checking for attachments creates UI problems in Compose window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[u382332]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141027150301

Steps to reproduce:

In the Compose window, open Contacts sidebar and close the Compose window to keep the sidebar showing on next open. 

Go to Tools>Options>Composition>General tab>remove check from [b]Check for missing attachments[/b] and click Close.

Close and restart Thunderbird.

Open a new Compose window.


Actual results:

Contacts side bar is not showing and the address lines only show one line and not the default 3 lines or any other number of lines being used.

Error Console:

Timestamp: 11/14/2014 6:03:56 PM
Error: data.slice is not a function
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1992

Timestamp: 11/14/2014 6:03:56 PM
Error: NS_ERROR_NOT_AVAILABLE: Cannot call openModalWindow on a hidden window
Source File: resource://gre/components/nsPrompter.js
Line: 356

Timestamp: 11/14/2014 6:03:56 PM
Error: NS_ERROR_NOT_AVAILABLE: Cannot call openModalWindow on a hidden window
Source File: resource://gre/components/nsPrompter.js
Line: 356

The bottom two are listed twice.

Regression range:

Works no problem-
User Agent: 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Thunderbird/36.0a1
Application Build ID: 	20141107030200

Broken-
User Agent: 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Thunderbird/36.0a1
Application Build ID: 	20141108030207




Expected results:

Change should not affect Compose window UI.

Comment 1

[Joe Sabash [:JoeS1]]

Confirmed on current trunk.
Additional Info:
The single address line persists until you again set "Check for missing Attachments"
AND restart TB

Comment 2

[:aceman]

I can reproduce this on linux and will work on it. It is caused by bug 938829.

Comment 3

[:aceman]

The Contacts sidebar is not needed to reproduce this bug. Just disabling notification in Options is enough for me.

Comment 4

[u382332]

(In reply to :aceman from comment #3)
> The Contacts sidebar is not needed to reproduce this bug. Just disabling
> notification in Options is enough for me.

That is correct. I included the Contacts sidebar in the STR just to make sure it was also known about and could be verified with the fix as well as the address lines.

Comment 5

[:aceman]

Attachment: patch

This should do it. I also add some tests. But I get strange inconsistent results (with AND without the patch), even mozmill crashes on my local setup in this test file. Aryx, please push to try server so I can see what is going on.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=e040883991d8

Comment 7

[:aceman]

Comment on attachment 8523556 [details] [diff] [review]
patch

OK, looks like it is working.

Comment 8

[Joe Sabash [:JoeS1]]

Patch seems to work fine in a local build with and without the pref setting

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 8523556 [details] [diff] [review]
patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +128,5 @@
>    gCharsetTitle = null;
>    gCharsetConvertManager = Components.classes['@mozilla.org/charset-converter-manager;1'].getService(Components.interfaces.nsICharsetConverterManager);
>    gHideMenus = false;
>    gManualAttachmentReminder = false;
> +  gRemindOnKeywords = getPref("mail.compose.attachment_reminder");

I don't particularly like having an extra global just for this.
Wouldn't it be simpler just to not set up gAttachmentNotifier observing to check anything if the pref is set to disabled?

Comment 11

[:aceman]

Well, we use the value in redetectKeywords() that is called from manageAttachmentNotification().  gAttachmentNotifier only manages the detecting of keywords, not the other events causing the notifier to come up.

Would you like to push this variable as a member into gAttachmentNotifier object together with redetectKeywords and CheckForAttachmentKeywords (as they actually belong into it) ?

Also, I can drop introducing that global var, I just found it potentially expensive to call getPref("mail.compose.attachment_reminder") each 0.5 secs. Also, I am not sure what would happen if the value of "mail.compose.attachment_reminder" changed while compose window was up so I made it fixed per session (1 message composition).

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #11)
> Would you like to push this variable as a member into gAttachmentNotifier
> object together with redetectKeywords and CheckForAttachmentKeywords (as
> they actually belong into it) ?

Sounds good to me, if you're up to it.

Comment 13

[:aceman]

Attachment: patch v2

OK.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8525578 [details] [diff] [review]
patch v2

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4516,5 @@
>  const gAttachmentNotifier =
>  {
>    _obs: null,
>  
> +  _gRemindOnKeywords: false,

odd to see a member variable be named with g.
Please rename to "enabled" (_ isn't really needed either, but sometimes useful if it implements some public interface). Isn't that what it's about?

@@ +4564,5 @@
> +   * @return  If async is true, attachmentWorker.message is called with the array
> +   *          of found keywords and this function returns null.
> +   *          If it is false, the array is returned from this function immediately.
> +   */
> +  CheckForAttachmentKeywords: function(async)

please lowerCamelCase this while you're here

@@ +4567,5 @@
> +   */
> +  CheckForAttachmentKeywords: function(async)
> +  {
> +    if (!this._gRemindOnKeywords)
> +      return false;

null or []

@@ +4574,5 @@
> +      // If we know we don't need to show the notification,
> +      // we can skip the expensive checking of keywords in the message.
> +      // but mark it in the .lastMessage that the keywords are unknown.
> +      attachmentWorker.lastMessage = null;
> +      return false;

null or []

Comment 15

[:aceman]

(In reply to Magnus Melin from comment #14)
> Review of attachment 8525578 [details] [diff] [review]:
> -----------------------------------------------------------------
 >  const gAttachmentNotifier =
> >  {
> >    _obs: null,
> >  
> > +  _gRemindOnKeywords: false,
> 
> odd to see a member variable be named with g.
> Please rename to "enabled" (_ isn't really needed either, but sometimes
> useful if it implements some public interface). Isn't that what it's about?
The point of this was that nothing outside of gAttachmentNotifier should see this variable so it is no longer global. And therefore it is internal to the object so it has the _. Some of the the other methods are publicly called from outside so do not have _.

Comment 16

[:aceman]

So I wanted to say that I'll remove the g which is preserved wrongly, but I'd keep _ as I added it intentionally.

Comment 17

[Magnus Melin [:mkmelin]]

I don't have a strong opinion about the _, but generally, it's hard to predict what outsiders may want (if they do anything at all with it).

Comment 18

[:aceman]

Attachment: patch v2.1

Thanks.

Comment 19

[aleth [:aleth]]

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