736055 - Filelink URL insertions need some adjustments and tests

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jonathan Protzenko [:protz]]

Hey, FileLink is now working perfectly, congrats! Here's two minor remarks.

1) Since the whole thing is about uploading large files, I think we definitely want a progress bar, or at least a percentage in the status bar, so that we know how long it will take until the file is fully uploaded.

2) If I start by FileLink'ing a file before writing anything in the compose window, there's no way for me to insert text *before* the inserted HTML. Is that intentional? Maybe we should insert a <p> before just so that the user can type text before the HTML that's inserted?

Congrats on the great feature, I love it already :=).

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey protz:

Glad it's working for you! :)

1)  Yeah, that was something I wanted too.  This was something that got pushed off until "milestone 3", and when I finally looked at it, I realized that it couldn't be done without some pretty massive changes to the interfaces we'd added (since we primary use nsIRequestObserver for feedback on the state of uploads, which only gives us start and stop events).

2)  Absolutely.  URL placement logic is currently kinda broken, and is where I'll be focusing my time now (along with tests! :D).

Cheers,

-Mike

Comment 2

[Mike Conley (:mconley) (:⚙️)]

With regards to the progress bar idea, it's something that could be implemented in the future, once we fix up what's on Aurora.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Regarding the progress bar, I've filed bug 736169 as an enhancement bug.

I'll use this bug to track improvements on URL insertion.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

The list of attachment URL behaviour bugs is likely going to shift rapidly in this patch, so instead of the nightmare of updating a bullet list of sub-issues within Bugzilla comments, I'm going to try something new.

I'm going to track the sub-issues in this Etherpad:  https://etherpad.mozilla.org/FilelinkURLBugs

Once the issues have more or less stabilised, I'll put the final list into this bug for posterity.

We'll see if this works.

Comment 5

[Mark Banner (:standard8)]

Not tracking Filelink issues until nearer the end of the aurora cycle.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 [checked-in]

This patch does the following:

1)  Wraps up forward content into a moz-forward-container classed DIV for both HTML and plaintext mail
2)  Ensures that forwards start with a BR, allowing users to insert text before the moz-forward-container node
3)  Smarter URL insertion - it's a bit complicated, but it takes care of the following scenarios for both plaintext and HTML:
  * Inserting into empty composers, with and without signatures
  * Inserting into composers with text, with and without signatures
  * Inserting into replies (above quote) with and without additional text entered, with and without signatures
  * Inserting into replies (below quote) with and without additional text entered, with and without signatures
  * Inserting into forwards with and without additional text entered, with and without signatures.
4)  Fixes a bug where we'd always insert a moz-signature node into a composer, even if the signature was empty.

Tests are being attached separately.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP tests v1

I've got some tests for this patch as well.  They still need some cleanup, which I'm working on right now.  But I thought I'd post them for posterity.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 607998 [details] [diff] [review]
Patch v1 [checked-in]

David:

Would it be possible for you to look at this today?  If so, these fixes can get into EarlyBird before Ludo's test day tomorrow.

-Mike

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #8)
> Comment on attachment 607998 [details] [diff] [review]
> Patch v1
> 
> David:
> 
> Would it be possible for you to look at this today?  If so, these fixes can
> get into EarlyBird before Ludo's test day tomorrow.
> 
> -Mike

David:

So I just spoke to Ludo, and based on what jhopkins told me about our EarlyBird building schedule, even if we landed these right now, Ludo wouldn't be able to use them for his test day.

So it's not critical that this patch lands in the next few hours or anything.

-Mike

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Tests v1

Ok, cleaned up my tests a bit.  Let me know if this looks alright.

Comment 11

[David :Bienvenu]

Comment on attachment 607998 [details] [diff] [review]
Patch v1 [checked-in]

looks good.

yikes! :
-      if (!sigOnTop && !aSignature.IsEmpty())
+      if (!sigOnTop && !aSignature.IsEmpty()) {
         textEditor->InsertLineBreak();
         InsertDivWrappedTextAtSelection(aSignature,
                                         NS_LITERAL_STRING("moz-signature"));
+      }

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment #607998 [details] [diff] checked in to:

comm-central as http://hg.mozilla.org/comm-central/rev/35bca187e3cc
comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/cbb93c0755c6

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Jim:

Do you have cycles to review my tests?  Or should I redirect?

-Mike

Comment 14

[Jim Porter (:squib)]

Comment on attachment 608034 [details] [diff] [review]
Tests v1

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

Unfortunately, I'm not going to have time to do a full review of this, as I'll be busy for the next week or so. Sorry about that! (However, I did notice one issue when glancing over the code.)

::: mail/test/mozmill/shared-modules/test-compose-helpers.js
@@ +93,2 @@
>   */
> +function open_compose_new_mail(aController, aShiftKey) {

aShiftKey isn't used in this function. Did you mean to use it, or should we remove that argument?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 608034 [details] [diff] [review]
Tests v1

Jim:

No problem - I'll redirect.  Thanks for the note!

Blake:

Got any cycles to look at this?

-Mike

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 608034 [details] [diff] [review]
Tests v1

>+++ b/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
>@@ -0,0 +1,756 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */

I still love the MPL2!  :)

>+/**
>+ * Tests Filelink URL insertion behaviours in compose windows.
>+ */
>+
>+let Cu = Components.utils;
>+let Cc = Components.classes;
>+let Ci = Components.interfaces;

I kinda like the
let { classes: Cc, interfaces: Ci, utils: Cu }  = Components;
formulation of this.  (But I won't insist on changing.  :)

>+function collectFiles(aFiles) {
>+function get_compose_body(aController) {
>+function assert_previous_text(aStart, aText) {
>+function type_in_composer(aController, aText) {
>+function assert_next_brs(aStart, aNum) {
>+function assert_previous_brs(aStart, aNum) {

These seem kinda general.  How do you feel about moving them into a helper file or two.

(Oh, also, those last two might take a string or function, so that you can use them for more than just "br"s.

>+/**
>+ * Helper function that sets up the mock file picker for a series of files,
>+ * spawns a reply window for the first message in the gFolder, optionally
>+ * types some strings into the compose window, and then attaches some
>+ * Filelinks.
>+ *
>+ * @param aFiles an array of filename strings for files located beneath
>+ *               the test directory.
>+ * @param aText an array of strings to type into the compose window. Each
>+ *              string is followed by pressing the RETURN key, except for
>+ *              the final string.  Pass an empty array if you don't want
>+ *              anything typed.
>+ */
>+function prepare_some_attachments_and_reply(aFiles, aText) {
[snip…]
>+function prepare_some_attachments_and_forward(aFiles, aText) {

So, if you're going to type some stuff in, and then attach some files, wouldn't it make more sense to have the stuff you type in come before the files you attach?

>+function test_inserts_linebreak_on_empty_compose() {
>+  const kSigPrefKey = "mail.identity.id1.htmlSigText";
>+  let oldSig = Services.prefs.getCharPref(kSigPrefKey);
>+  Services.prefs.setCharPref(kSigPrefKey, "");
>+
>+  subtest_inserts_linebreak_on_empty_compose();
>+  Services.prefs.setBoolPref(kHtmlPrefKey, false);
>+  subtest_inserts_linebreak_on_empty_compose();
>+  Services.prefs.setBoolPref(kHtmlPrefKey, true);

Does this also automatically assume that kHtmlPrefKey starts off as true?
Should we save/restore it, and force it to be true, just in case?

>+function test_adding_filelinks_to_written_message() {
>+  const kSigPrefKey = "mail.identity.id1.htmlSigText";
>+  let oldSig = Services.prefs.getCharPref(kSigPrefKey);
>+  Services.prefs.setCharPref(kSigPrefKey, "");
>+
>+  subtest_adding_filelinks_to_written_message();
>+  Services.prefs.setBoolPref(kHtmlPrefKey, false);
>+  subtest_adding_filelinks_to_written_message();
>+  Services.prefs.setBoolPref(kHtmlPrefKey, true);
>+
>+  Services.prefs.setCharPref(kSigPrefKey, oldSig);

I'm also seeing this pattern (save the sig, call a test, set the html pref, call the test, reset the html pref, reset the sig) a whole bunch, and I think it might be worth it to generalize it a little more.

>+++ b/mail/test/mozmill/shared-modules/test-compose-helpers.js
>@@ -79,20 +79,24 @@ function installInto(module) {
> /**
>  * Opens the compose window by starting a new message
>  *
>+ * @param aController the controller for the mail:3pane from which to spawn
>+ *                    the compose window.  If left blank, defaults to mc.
>+ *
>  * @return The loaded window of type "msgcompose" wrapped in a MozmillController
>  *         that is augmented using augment_controller.
>+ *
>  */
>-function open_compose_new_mail(aController) {
>+function open_compose_new_mail(aController, aShiftKey) {

If you're going to add documentation, perhaps you can document aShiftKey, too?

>   windowHelper.plan_for_new_window("msgcompose");
>   aController.keypress(null, "n", {shiftKey: false, accelKey: true});

And I'm thinking you want to change that "false" to "aShiftKey"…

Other than that, looks good!  r=me!

Thanks,
Blake.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Tests v2 (carrying over r+ from bwinton)

(In reply to Jim Porter (:squib) from comment #14)
> ::: mail/test/mozmill/shared-modules/test-compose-helpers.js
> @@ +93,2 @@
> >   */
> > +function open_compose_new_mail(aController, aShiftKey) {
> 
> aShiftKey isn't used in this function. Did you mean to use it, or should we
> remove that argument?

Whoops!  Accidental leftover from some experimentation.  Removed.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #16)

> >+++ b/mail/test/mozmill/shared-modules/test-compose-helpers.js
> >+function open_compose_new_mail(aController, aShiftKey) {
> 
> If you're going to add documentation, perhaps you can document aShiftKey,
> too?
> ...
> And I'm thinking you want to change that "false" to "aShiftKey"…

See above - accidental fragment.  It's been removed.

> >+++ b/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
> >@@ -0,0 +1,756 @@
> >+/* This Source Code Form is subject to the terms of the Mozilla Public
> >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> I still love the MPL2!  :)

Me too!

> 
> >+/**
> >+ * Tests Filelink URL insertion behaviours in compose windows.
> >+ */
> >+
> >+let Cu = Components.utils;
> >+let Cc = Components.classes;
> >+let Ci = Components.interfaces;
> 
> I kinda like the
> let { classes: Cc, interfaces: Ci, utils: Cu }  = Components;
> formulation of this.  (But I won't insist on changing.  :)

Actually, it turns out that Mozmill is magical, and I don't need to define Cu/Cc/Ci because they've already been defined.  So I've just removed those definitions.

> 
> >+function collectFiles(aFiles) {
> >+function get_compose_body(aController) {
> >+function assert_previous_text(aStart, aText) {
> >+function type_in_composer(aController, aText) {
> >+function assert_next_brs(aStart, aNum) {
> >+function assert_previous_brs(aStart, aNum) {
> 
> These seem kinda general.  How do you feel about moving them into a helper
> file or two.

I've moved get_compose_body, type_in_composer and assert_previous_text to the compose helpers, and assert_next_brs / assert_previous_brs to dom-helpers.

I've left collectFiles behind, because it assumes it's defined within the file that's calling it - which is necessary because it uses the file it's defined in to base it's search for the files that it's collecting.

> (Oh, also, those last two might take a string or function, so that you can
> use them for more than just "br"s.

Done!

> 
> >+/**
> >+ * Helper function that sets up the mock file picker for a series of files,
> >+ * spawns a reply window for the first message in the gFolder, optionally
> >+ * types some strings into the compose window, and then attaches some
> >+ * Filelinks.
> >+ *
> >+ * @param aFiles an array of filename strings for files located beneath
> >+ *               the test directory.
> >+ * @param aText an array of strings to type into the compose window. Each
> >+ *              string is followed by pressing the RETURN key, except for
> >+ *              the final string.  Pass an empty array if you don't want
> >+ *              anything typed.
> >+ */
> >+function prepare_some_attachments_and_reply(aFiles, aText) {
> [snip…]
> >+function prepare_some_attachments_and_forward(aFiles, aText) {
> 
> So, if you're going to type some stuff in, and then attach some files,
> wouldn't it make more sense to have the stuff you type in come before the
> files you attach?

Good point!  Fixed.

> 
> >+function test_inserts_linebreak_on_empty_compose() {
> >+  const kSigPrefKey = "mail.identity.id1.htmlSigText";
> >+  let oldSig = Services.prefs.getCharPref(kSigPrefKey);
> >+  Services.prefs.setCharPref(kSigPrefKey, "");
> >+
> >+  subtest_inserts_linebreak_on_empty_compose();
> >+  Services.prefs.setBoolPref(kHtmlPrefKey, false);
> >+  subtest_inserts_linebreak_on_empty_compose();
> >+  Services.prefs.setBoolPref(kHtmlPrefKey, true);
> 
> Does this also automatically assume that kHtmlPrefKey starts off as true?
> Should we save/restore it, and force it to be true, just in case?

Another good point!  We now save / restore the kHtmlPrefKey pref, and the signature too while I was at it.

> 
> >+function test_adding_filelinks_to_written_message() {
> >+  const kSigPrefKey = "mail.identity.id1.htmlSigText";
> >+  let oldSig = Services.prefs.getCharPref(kSigPrefKey);
> >+  Services.prefs.setCharPref(kSigPrefKey, "");
> >+
> >+  subtest_adding_filelinks_to_written_message();
> >+  Services.prefs.setBoolPref(kHtmlPrefKey, false);
> >+  subtest_adding_filelinks_to_written_message();
> >+  Services.prefs.setBoolPref(kHtmlPrefKey, true);
> >+
> >+  Services.prefs.setCharPref(kSigPrefKey, oldSig);
> 
> I'm also seeing this pattern (save the sig, call a test, set the html pref,
> call the test, reset the html pref, reset the sig) a whole bunch, and I
> think it might be worth it to generalize it a little more.

Good catch.  Generalized to a function "try_with_and_without_signature", and I've renamed the original function with that name to the more specific "try_with_and_without_signature_in_reply_or_fwd".

> Other than that, looks good!  r=me!

Cool, thanks!  I'm going to push this to try, and if it looks like I haven't injected any oranges, I'll go ahead and push to comm-central.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Tests v3 (carrying over r+ from bwinton)

Whoops - forgot to document that new function.  Also, gave it a more descriptive name (try_without_signature).

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 610697 [details] [diff] [review]
Tests v3 (carrying over r+ from bwinton)

I don't see any risk in landing these at all, since they're really only tests, and don't affect core functionality.  A push to try didn't reveal any test breakages.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Mozmill tests were pushed to comm-central as http://hg.mozilla.org/comm-central/rev/9382c5e02e62

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/659db89dd3e3