789865 - Port front-end parts from bug 440794 (Initial sendInBackground support) to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Description

[Tony Mechelynck [:tonymec]]

Setting preference mailnews.sendInBackground has no effect in SeaMonkey, even after a restart.

See bug 440794 attachment 368247 [details] [diff] [review] and bug 440794 attachment 369842 [details] [diff] [review]. The backend parts are common to Tb & Sm but we still need the frontend parts.

Philip: I'm setting [good first bug] but I haven't checked if it qualifies. You may want to add the other fields (or, if too hard, clear the Whiteboard).

Comment 1

[Tony Mechelynck [:tonymec]]

The frontend parts from these two attachments are in the same file, and a lot of what the second one does consists of just undoing what the first one had changed. :-/

Comment 2

[Tony Mechelynck [:tonymec]]

Attachment: patch v0

Port the frontend part which weren't reverting each other.

IanN (reviewer): I cannot test that this compiles. I noticed a difference in the last hunk and allowed for it, but maybe there are other coding differences between Tb & Sm which have escaped my mind.

Comment 3

[Philip Chee]

Quick nits:
> +              .getBoolPref("mailnews.sendInBackground");
Can use Services.prefs.getBoolPref("mailnews.sendInBackground");

Comment 4

[Tony Mechelynck [:tonymec]]

(In reply to Philip Chee from comment #3)
> Quick nits:
> > +              .getBoolPref("mailnews.sendInBackground");
> Can use Services.prefs.getBoolPref("mailnews.sendInBackground");

Ah, I had a vague idea that it might be possible, but didn't know how. Do you know where to bone up on those "Services.blabla" things or should I try searching MDN?

Comment 5

[Tony Mechelynck [:tonymec]]

Attachment: patch v0.1, using Services.prefs.getBoolPref()

Comment 6

[Tony Mechelynck [:tonymec]]

(In reply to Tony Mechelynck [:tonymec] from comment #4)
> [...] Do
> you know where to bone up on those "Services.blabla" things or should I try
> searching MDN?

Gotcha: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm

Not that I understand everything in it, but well…

Comment 7

[Philip Chee]

> Do you know where to bone up on those "Services.blabla" things

I just look at the following to see what's available
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm

>Gotcha: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm
This is usually out of date, at least for trunk.

Comment 8

[Tony Mechelynck [:tonymec]]

(In reply to Philip Chee from comment #7)
> > Do you know where to bone up on those "Services.blabla" things
> 
> I just look at the following to see what's available
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Services.jsm
> 
> >Gotcha: https://developer.mozilla.org/en-US/docs/JavaScript_code_modules/Services.jsm
> This is usually out of date, at least for trunk.

Well, at least it's a small source file (currently 72 lines, not thousands). Ah la la… Apparently there isn't much software where the developers are expected to write the documentation together with the code, or even before (see http://vimdoc.sourceforge.net/htmldoc/develop.html#design-documented for the outstanding exception <shrug />)

Comment 9

[Tony Mechelynck [:tonymec]]

Attachment: patch v0.2, with better metadata

Comment 10

[Ian Neal]

Comment on attachment 659713 [details] [diff] [review]
patch v0.2, with better metadata

> function SendMessage()
> {
>+  let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground");
>+  GenericSendMessage(sendInBackground ?
>+                     nsIMsgCompDeliverMode.Background :
>+                     nsIMsgCompDeliverMode.Now);
This could probably be done over 2 rather than 3 lines.
> }
> 
> function SendMessageWithCheck()
> {
>     var warn = getPref("mail.warn_on_send_accel_key");
> 
>     if (warn) {
>         var checkValue = {value:false};
>@@ -1916,18 +1922,20 @@ function SendMessageWithCheck()
>         if (buttonPressed != 0) {
>             return;
>         }
>         if (checkValue.value) {
>             Services.prefs.setBoolPref("mail.warn_on_send_accel_key", false);
>         }
>     }
> 
>+  let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground");
>+  GenericSendMessage(Services.io.offline ? nsIMsgCompDeliverMode.Later :
>+                       (sendInBackground ? nsIMsgCompDeliverMode.Background :
>+                                           nsIMsgCompDeliverMode.Now));
I would prefer that you did something like:
if (Services.io.offline)
  SendMessageLater();
else
  SendMessage();

> }
> 
> function SendMessageLater()
> {
>   GenericSendMessage(nsIMsgCompDeliverMode.Later);
> }
r- as I would like to see the new patch.

Comment 11

[Tony Mechelynck [:tonymec]]

(In reply to Ian Neal from comment #10)
> Comment on attachment 659713 [details] [diff] [review]
> patch v0.2, with better metadata
> 
> > function SendMessage()
> > {
> >+  let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground");
> >+  GenericSendMessage(sendInBackground ?
> >+                     nsIMsgCompDeliverMode.Background :
> >+                     nsIMsgCompDeliverMode.Now);
> This could probably be done over 2 rather than 3 lines.

Attachment 369842 [details] [diff] new line 1886 does it in 3 lines but no prob., I'll try 2 lines in the new version of the patch so we can compare.

[...]
> >+  let sendInBackground = Services.prefs.getBoolPref("mailnews.sendInBackground");
> >+  GenericSendMessage(Services.io.offline ? nsIMsgCompDeliverMode.Later :
> >+                       (sendInBackground ? nsIMsgCompDeliverMode.Background :
> >+                                           nsIMsgCompDeliverMode.Now));
> I would prefer that you did something like:
> if (Services.io.offline)
>   SendMessageLater();
> else
>   SendMessage();
> 
yes, looks clearer; though it could be marginally slower (one more level of function call). Probably not to worry.

> > }
> > 
> > function SendMessageLater()
> > {
> >   GenericSendMessage(nsIMsgCompDeliverMode.Later);
> > }
> r- as I would like to see the new patch.

OK, patch forthcoming. r- is better than no answer. :-)

Comment 12

[Tony Mechelynck [:tonymec]]

Attachment: patch v1.0, addressing review comments on v0.2

Question: is ui-review necessary, considering that, though there are user-facing changes, they involve hardly any UI?

Comment 13

[Ian Neal]

Comment on attachment 663332 [details] [diff] [review]
patch v1.0, addressing review comments on v0.2

Shouldn't need a ui-review

Comment 14

[Karsten Düsterloh]

Comment on attachment 663332 [details] [diff] [review]
patch v1.0, addressing review comments on v0.2

Nice!

>+    if (Services.io.offline)
>+        SendMessageLater();
>+    else
>+        SendMessage();

To get the indentation hell cleaned up one day, I'd prefer if you already use the correct two-spaces-per-level indent for touched code.

Comment 15

[Tony Mechelynck [:tonymec]]

(In reply to Karsten Düsterloh from comment #14)
[...]
> To get the indentation hell cleaned up one day, I'd prefer if you already
> use the correct two-spaces-per-level indent for touched code.

I was trying to stay consistent with neighbouring code (visible in the same hunk just before the modified lines).

I'll prepare a modified patch with the changes indented at two spaces per level.

Shall I add a followup patch, reformatting the whole file with the correct indents, or would you rather leave unmodified lines unchanged?

Comment 16

[Tony Mechelynck [:tonymec]]

Attachment: patch v1.1 carrying forward r=IanN sr=Mnyromyr

- indenting by twos at lines 1930 and 1932
- at lines 1675, 1676 and 1903 I kept corresponding words aligned as in the previous version.

Comment 17

[Tony Mechelynck [:tonymec]]

Patch is ready for checkin to comm-central.

Please leave the bug open, a followup patch may or may not be needed depending on Karsten's answer to comment #15.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/17c2cee4815c

Comment 19

[Karsten Düsterloh]

(In reply to Tony Mechelynck [:tonymec] from comment #15)
> > To get the indentation hell cleaned up one day, I'd prefer if you already
> > use the correct two-spaces-per-level indent for touched code.
> 
> I was trying to stay consistent with neighbouring code (visible in the same
> hunk just before the modified lines).

That's good in general. :)

> Shall I add a followup patch, reformatting the whole file with the correct
> indents, or would you rather leave unmodified lines unchanged?

Yes, please leave unmodified lines as they are.
Unnecessary whitespace changes only clutter the edit history of the file.