[geary/wip/309-signature-not-updated] Fix signature not being updated when composer first opened without one



commit 12c6bbca5638284e4b69a289ad10369c35dbb023
Author: Michael Gratton <mike vee net>
Date:   Mon Mar 11 11:59:11 2019 +1100

    Fix signature not being updated when composer first opened without one
    
    The ComposerPageState JS object assumed that if no signature was present
    when first loaded, that none ever would be. This broke changing the
    signature when the composer was opened for an account without one, and
    the from account was changed to an account with a sig.
    
    Instead of including the signature as part of the loaded body, always
    include just a skeleton signature DIV and ensure the signature is loaded
    dynamically after the body has been loaded. Update code and tests to
    match this assumption, and add a unit test for updating the sig.
    
    Fixes #309

 src/client/composer/composer-web-view.vala       |  9 +----
 src/client/composer/composer-widget.vala         | 37 +++++++++++--------
 test/client/composer/composer-web-view-test.vala | 47 ++++++++++++++++++------
 test/js/composer-page-state-test.vala            | 14 +++----
 ui/composer-web-view.css                         |  4 ++
 ui/composer-web-view.js                          | 19 ++++++++--
 6 files changed, 81 insertions(+), 49 deletions(-)
---
diff --git a/src/client/composer/composer-web-view.vala b/src/client/composer/composer-web-view.vala
index c308dcd4..2e7f8147 100644
--- a/src/client/composer/composer-web-view.vala
+++ b/src/client/composer/composer-web-view.vala
@@ -133,7 +133,6 @@ public class ComposerWebView : ClientWebView {
      * Loads a message HTML body into the view.
      */
     public new void load_html(string body,
-                              string signature,
                               string quote,
                               bool top_posting,
                               bool is_draft) {
@@ -142,9 +141,7 @@ public class ComposerWebView : ClientWebView {
         const string BODY_PRE = """
 <div id="geary-body" dir="auto">""";
         const string BODY_POST = """</div>
-""";
-        const string SIGNATURE = """
-<div id="geary-signature" dir="auto">%s</div>
+<div id="geary-signature" class="geary-no-display" dir="auto"></div>
 """;
         const string QUOTE = """
 <div id="geary-quote" dir="auto"><br />%s</div>
@@ -171,10 +168,6 @@ public class ComposerWebView : ClientWebView {
             html.append(CURSOR);
             html.append(BODY_POST);
 
-            if (!Geary.String.is_empty(signature)) {
-                html.append_printf(SIGNATURE, signature);
-            }
-
             if (top_posting && !Geary.String.is_empty(quote)) {
                 html.append_printf(QUOTE, quote);
             }
diff --git a/src/client/composer/composer-widget.vala b/src/client/composer/composer-widget.vala
index cb6d9506..cd39fa94 100644
--- a/src/client/composer/composer-widget.vala
+++ b/src/client/composer/composer-widget.vala
@@ -461,6 +461,7 @@ public class ComposerWidget : Gtk.EventBox, Geary.BaseInterface {
         this.editor = new ComposerWebView(config);
         this.editor.set_hexpand(true);
         this.editor.set_vexpand(true);
+        this.editor.content_loaded.connect(on_editor_content_loaded);
         this.editor.show();
 
         this.body_container.add(this.editor);
@@ -628,10 +629,8 @@ public class ComposerWidget : Gtk.EventBox, Geary.BaseInterface {
         update_attachments_view();
         update_pending_attachments(this.pending_include, true);
 
-        string signature = yield load_signature(cancellable);
         this.editor.load_html(
             this.body_html,
-            signature,
             referred_quote,
             this.top_posting,
             is_referred_draft
@@ -2164,41 +2163,43 @@ public class ComposerWidget : Gtk.EventBox, Geary.BaseInterface {
 
             if (selected.account != this.account) {
                 this.account = selected.account;
-                this.load_signature.begin(null, (obj, res) => {
-                        this.editor.update_signature(this.load_signature.end(res));
-                    });
+                this.update_signature.begin(null);
                 load_entry_completions();
                 this.reopen_draft_manager_async.begin();
             }
         }
     }
 
-    private async string load_signature(Cancellable? cancellable = null) {
-        string account_sig = "";
-
+    private async void update_signature(Cancellable? cancellable = null) {
+        string sig = "";
         if (this.account.information.use_signature) {
-            account_sig = account.information.signature;
-            if (Geary.String.is_empty_or_whitespace(account_sig)) {
+            sig = account.information.signature;
+            if (Geary.String.is_empty_or_whitespace(sig)) {
                 // No signature is specified in the settings, so use
                 // ~/.signature
                 File signature_file = File.new_for_path(Environment.get_home_dir()).get_child(".signature");
                 try {
                     uint8[] data;
                     yield signature_file.load_contents_async(cancellable, out data, null);
-                    account_sig = (string) data;
+                    sig = (string) data;
                 } catch (Error error) {
                     if (!(error is IOError.NOT_FOUND)) {
                         debug("Error reading signature file %s: %s", signature_file.get_path(), 
error.message);
                     }
                 }
             }
-
-            account_sig = (!Geary.String.is_empty_or_whitespace(account_sig))
-                ? Geary.HTML.smart_escape(account_sig)
-                : "";
         }
 
-        return account_sig;
+        // Still want to update the signature even if it is empty,
+        // since when changing the selected from account, if the
+        // previously selected account had a sig but the newly
+        // selected account does not, the old sig gets cleared out.
+        if (Geary.String.is_empty_or_whitespace(sig)) {
+            // Clear out multiple spaces etc so smart_escape
+            // doesn't create &nbsp;'s
+            sig = "";
+        }
+        this.editor.update_signature(Geary.HTML.smart_escape(sig));
     }
 
     private async ComposerLinkPopover new_link_popover(ComposerLinkPopover.Type type,
@@ -2230,6 +2231,10 @@ public class ComposerWidget : Gtk.EventBox, Geary.BaseInterface {
         get_action(GearyApplication.ACTION_REDO).set_enabled(can_redo);
     }
 
+    private void on_editor_content_loaded() {
+        this.update_signature.begin(null);
+    }
+
     private void on_draft_id_changed() {
         draft_id_changed(this.draft_manager.current_draft_id);
     }
diff --git a/test/client/composer/composer-web-view-test.vala 
b/test/client/composer/composer-web-view-test.vala
index 4bb36939..ef5b97e0 100644
--- a/test/client/composer/composer-web-view-test.vala
+++ b/test/client/composer/composer-web-view-test.vala
@@ -7,7 +7,6 @@
 
 public class ComposerWebViewTest : ClientWebViewTestCase<ComposerWebView> {
 
-    private const string BODY_TEMPLATE = """<div id="geary-body" 
dir="auto">%s<div><br></div><div><br></div></div>""";
 
     public ComposerWebViewTest() {
         base("ComposerWebViewTest");
@@ -23,6 +22,7 @@ public class ComposerWebViewTest : ClientWebViewTestCase<ComposerWebView> {
         add_test("get_text_with_named_link", get_text_with_named_link);
         add_test("get_text_with_url_link", get_text_with_named_link);
         add_test("get_text_with_surrounding_nbsps", get_text_with_surrounding_nbsps);
+        add_test("update_signature", update_signature);
     }
 
     public void load_resources() throws Error {
@@ -45,17 +45,12 @@ public class ComposerWebViewTest : ClientWebViewTestCase<ComposerWebView> {
         assert(new ComposerWebView.EditContext("0,,,12").font_size == 12);
     }
 
-    public void get_html() throws Error {
-        string html = "<p>para</p>";
-        load_body_fixture(html);
+    public void get_html() throws GLib.Error {
+        string BODY = "<p>para</p>";
+        load_body_fixture(BODY);
         this.test_view.get_html.begin((obj, ret) => { async_complete(ret); });
-        try {
-            assert(this.test_view.get_html.end(async_result()) ==
-                   BODY_TEMPLATE.printf(html));
-        } catch (Error err) {
-            print("Error: %s\n", err.message);
-            assert_not_reached();
-        }
+        string html = this.test_view.get_html.end(async_result());
+        assert_string(ComposerPageStateTest.COMPLETE_BODY_TEMPLATE.printf(BODY), html);
     }
 
     public void get_text() throws Error {
@@ -210,12 +205,40 @@ long, long, long, long, long, long, long, long, long, long,
         }
     }
 
+    public void update_signature() throws GLib.Error {
+        const string BODY = "<p>para</p>";
+        load_body_fixture(BODY);
+        string html = "";
+
+        const string SIG1 = "signature text 1";
+        this.test_view.update_signature(SIG1);
+        this.test_view.get_html.begin((obj, ret) => { async_complete(ret); });
+        html = this.test_view.get_html.end(async_result());
+        assert_true(BODY in html, "Body not present");
+        assert_true(SIG1 in html, "Signature 1 not present");
+
+        const string SIG2 = "signature text 2";
+        this.test_view.update_signature(SIG2);
+        this.test_view.get_html.begin((obj, ret) => { async_complete(ret); });
+        html = this.test_view.get_html.end(async_result());
+        assert_true(BODY in html, "Body not present");
+        assert_false(SIG1 in html, "Signature 1 still present");
+        assert_true(SIG2 in html, "Signature 2 not present");
+
+        this.test_view.update_signature("");
+        this.test_view.get_html.begin((obj, ret) => { async_complete(ret); });
+        html = this.test_view.get_html.end(async_result());
+        assert_true(BODY in html, "Body not present");
+        assert_false(SIG1 in html, "Signature 1 still present");
+        assert_false(SIG2 in html, "Signature 2 still present");
+    }
+
     protected override ComposerWebView set_up_test_view() {
         return new ComposerWebView(this.config);
     }
 
     protected override void load_body_fixture(string html = "") {
-        this.test_view.load_html(html, "", "", false, false);
+        this.test_view.load_html(html, "", false, false);
         while (this.test_view.is_loading) {
             Gtk.main_iteration();
         }
diff --git a/test/js/composer-page-state-test.vala b/test/js/composer-page-state-test.vala
index ad8098fd..52afd678 100644
--- a/test/js/composer-page-state-test.vala
+++ b/test/js/composer-page-state-test.vala
@@ -7,8 +7,9 @@
 
 class ComposerPageStateTest : ClientWebViewTestCase<ComposerWebView> {
 
-    private const string COMPLETE_BODY_TEMPLATE = """<div id="geary-body" 
dir="auto">%s<div><br></div><div><br></div></div>""";
-    private const string CLEAN_BODY_TEMPLATE = "%s<div><br></div><div><br></div>";
+    public const string COMPLETE_BODY_TEMPLATE =
+        """<div id="geary-body" dir="auto">%s<div><br></div><div><br></div></div><div id="geary-signature" 
dir="auto"></div>""";
+    public const string CLEAN_BODY_TEMPLATE = "%s<div><br></div><div><br></div>";
 
     public ComposerPageStateTest() {
         base("ComposerPageStateTest");
@@ -89,7 +90,6 @@ class ComposerPageStateTest : ClientWebViewTestCase<ComposerWebView> {
 
 some text
 """,
-                          "-- <br>sig",
                           "<p>outerquote text</p>",
                           true
             );
@@ -103,9 +103,6 @@ some text
             assert(!WebKitUtil.to_bool(run_javascript(
                 @"geary.containsAttachmentKeyword(\"innerquote\", \"subject text\");"
             )));
-            assert(!WebKitUtil.to_bool(run_javascript(
-                @"geary.containsAttachmentKeyword(\"sig\", \"subject text\");"
-            )));
             assert(!WebKitUtil.to_bool(run_javascript(
                 @"geary.containsAttachmentKeyword(\"outerquote\", \"subject text\");"
             )));
@@ -292,14 +289,13 @@ unknown://example6.com
     }
 
     protected override void load_body_fixture(string body = "") {
-        load_body_fixture_full(body, "", "", true);
+        load_body_fixture_full(body, "", true);
     }
 
     protected void load_body_fixture_full(string body,
-                                          string sig,
                                           string quote,
                                           bool top_posting) {
-        this.test_view.load_html(body, sig, quote, top_posting, false);
+        this.test_view.load_html(body, quote, top_posting, false);
         while (this.test_view.is_loading) {
             Gtk.main_iteration();
         }
diff --git a/ui/composer-web-view.css b/ui/composer-web-view.css
index 83c36e90..a5344c10 100644
--- a/ui/composer-web-view.css
+++ b/ui/composer-web-view.css
@@ -24,6 +24,10 @@ body.plain a {
   cursor: text;
 }
 
+body > *.geary-no-display {
+  display: none !important;
+}
+
 body > div#geary-body {
   margin: 0 !important;
   border: 0 !important;
diff --git a/ui/composer-web-view.js b/ui/composer-web-view.js
index 31fda273..876e4cb5 100644
--- a/ui/composer-web-view.js
+++ b/ui/composer-web-view.js
@@ -133,6 +133,7 @@ ComposerPageState.prototype = {
         if (!enabled) {
             this.stopBodyObserver();
         }
+
         this.bodyPart.contentEditable = true;
         if (this.signaturePart != null) {
             this.signaturePart.contentEditable = true;
@@ -140,6 +141,7 @@ ComposerPageState.prototype = {
         if (this.quotePart != null) {
             this.quotePart.contentEditable = true;
         }
+
         if (enabled) {
             // Enable modification observation only after the document
             // has been set editable as WebKit will alter some attrs
@@ -208,8 +210,13 @@ ComposerPageState.prototype = {
     },
     updateSignature: function(signature) {
         if (this.signaturePart != null) {
-            console.log(signature);
-            this.signaturePart.innerHTML = signature;
+            if (signature.trim()) {
+                this.signaturePart.innerHTML = signature;
+                this.signaturePart.classList.remove("geary-no-display");
+            } else {
+                this.signaturePart.innerHTML = "";
+                this.signaturePart.classList.add("geary-no-display");
+            }
         }
     },
     deleteQuotedMessage: function() {
@@ -315,13 +322,17 @@ ComposerPageState.prototype = {
 
         if (this.signaturePart != null) {
             parent.appendChild(
-                ComposerPageState.cleanPart(this.signaturePart.cloneNode(true), false)
+                ComposerPageState.cleanPart(
+                    this.signaturePart.cloneNode(true), false
+                )
             );
         }
 
         if (this.quotePart != null) {
             parent.appendChild(
-                ComposerPageState.cleanPart(this.quotePart.cloneNode(true), false)
+                ComposerPageState.cleanPart(
+                    this.quotePart.cloneNode(true), false
+                )
             );
         }
 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]