Closed Bug 1099866 Opened 6 years ago Closed 6 years ago

Not checking for attachments creates UI problems in Compose window

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
major

Tracking

(thunderbird36 fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird36 --- fixed

People

(Reporter: u382332, Assigned: aceman)

References

Details

(Keywords: regression, Whiteboard: [regression from bug 938829])

Attachments

(1 file, 2 obsolete files)

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.
Severity: normal → major
Confirmed on current trunk.
Additional Info:
The single address line persists until you again set "Check for missing Attachments"
AND restart TB
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can reproduce this on linux and will work on it. It is caused by bug 938829.
Assignee: nobody → acelists
Blocks: 938829
Component: Preferences → Message Compose Window
Keywords: regression
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [regression from bug 938829]
The Contacts sidebar is not needed to reproduce this bug. Just disabling notification in Options is enough for me.
Status: NEW → ASSIGNED
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Flags: needinfo?(archaeopteryx)
Comment on attachment 8523556 [details] [diff] [review]
patch

OK, looks like it is working.
Attachment #8523556 - Flags: review?(mkmelin+mozilla)
Patch seems to work fine in a local build with and without the pref setting
Duplicate of this bug: 1095963
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?
Attachment #8523556 - Flags: review?(mkmelin+mozilla)
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).
(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.
Attached patch patch v2 (obsolete) — Splinter Review
OK.
Attachment #8523556 - Attachment is obsolete: true
Attachment #8525578 - Flags: review?(mkmelin+mozilla)
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 []
Attachment #8525578 - Flags: review?(mkmelin+mozilla) → review+
(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 _.
So I wanted to say that I'll remove the g which is preserved wrongly, but I'd keep _ as I added it intentionally.
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).
Attached patch patch v2.1Splinter Review
Thanks.
Attachment #8525578 - Attachment is obsolete: true
Attachment #8526354 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.