The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

SeaMonkey
MailNews: Composition
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tonymec, Assigned: tonymec)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph
Bug Flags:
in-testsuite ?

SeaMonkey Tracking Flags

(seamonkey2.15 fixed)

Details

(Whiteboard: [good first bug][mentor=IanN][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

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).
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. :-/
Component: MailNews: Message Display → MailNews: Composition
Created attachment 659647 [details] [diff] [review]
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.
Assignee: nobody → antoine.mechelynck
Status: NEW → ASSIGNED
Attachment #659647 - Flags: review?(iann_bugzilla)

Comment 3

5 years ago
Quick nits:
> +              .getBoolPref("mailnews.sendInBackground");
Can use Services.prefs.getBoolPref("mailnews.sendInBackground");
Whiteboard: [good first bug] → [good first bug][mentor=IanN][lang=js]
(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?
Created attachment 659677 [details] [diff] [review]
patch v0.1, using Services.prefs.getBoolPref()
Attachment #659647 - Attachment is obsolete: true
Attachment #659647 - Flags: review?(iann_bugzilla)
Attachment #659677 - Flags: review?(iann_bugzilla)
(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

5 years ago
> 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.
(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 />)
Created attachment 659713 [details] [diff] [review]
patch v0.2, with better metadata
Attachment #659677 - Attachment is obsolete: true
Attachment #659677 - Flags: review?(iann_bugzilla)
Attachment #659713 - Flags: review?(iann_bugzilla)

Comment 10

5 years ago
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.
Attachment #659713 - Flags: review?(iann_bugzilla) → review-
(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. :-)
Created attachment 663332 [details] [diff] [review]
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?
Attachment #659713 - Attachment is obsolete: true
Attachment #663332 - Flags: superreview?(mnyromyr)
Attachment #663332 - Flags: review?(iann_bugzilla)

Comment 13

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

Shouldn't need a ui-review
Attachment #663332 - Flags: review?(iann_bugzilla) → review+

Comment 14

5 years ago
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.
Attachment #663332 - Flags: superreview?(mnyromyr) → superreview+
(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?
Created attachment 667261 [details] [diff] [review]
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.
Attachment #663332 - Attachment is obsolete: true
Attachment #667261 - Flags: superreview+
Attachment #667261 - Flags: review+
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.
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=IanN][lang=js] → [leave open] [good first bug][mentor=IanN][lang=js]
https://hg.mozilla.org/comm-central/rev/17c2cee4815c
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [leave open] [good first bug][mentor=IanN][lang=js] → [leave open][good first bug][mentor=IanN][lang=js]
status-seamonkey2.15: --- → fixed

Comment 19

5 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open][good first bug][mentor=IanN][lang=js] → [good first bug][mentor=IanN][lang=js]

Updated

5 years ago
No longer blocks: 360488
Blocks: 799533
You need to log in before you can comment on or make changes to this bug.