Not checking for attachments creates UI problems in Compose window

RESOLVED FIXED in Thunderbird 36.0

Status

--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: u382332, Assigned: aceman)

Tracking

({regression})

Trunk
Thunderbird 36.0
regression
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird36 fixed)

Details

(Whiteboard: [regression from bug 938829])

Attachments

(1 attachment, 2 obsolete attachments)

20.27 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Severity: normal → major

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
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]
(Assignee)

Comment 3

4 years ago
The Contacts sidebar is not needed to reproduce this bug. Just disabling notification in Options is enough for me.
Status: NEW → ASSIGNED
(Reporter)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8523556 [details] [diff] [review]
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.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 7

4 years ago
Comment on attachment 8523556 [details] [diff] [review]
patch

OK, looks like it is working.
Attachment #8523556 - Flags: review?(mkmelin+mozilla)

Comment 8

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

Updated

4 years ago
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)
(Assignee)

Comment 11

4 years ago
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.
(Assignee)

Comment 13

4 years ago
Created attachment 8525578 [details] [diff] [review]
patch v2

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+
(Assignee)

Comment 15

4 years ago
(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 _.
(Assignee)

Comment 16

4 years ago
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).
(Assignee)

Comment 18

4 years ago
Created attachment 8526354 [details] [diff] [review]
patch v2.1

Thanks.
Attachment #8525578 - Attachment is obsolete: true
Attachment #8526354 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
status-thunderbird36: --- → fixed
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.