From 12c6bbca5638284e4b69a289ad10369c35dbb023 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 11 Mar 2019 11:59:11 +1100 Subject: [PATCH] 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 ++++++++------- .../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 = """
"""; const string BODY_POST = """
-"""; - const string SIGNATURE = """ -
%s
+
"""; const string QUOTE = """

%s
@@ -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  '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 { - private const string BODY_TEMPLATE = """
%s


"""; public ComposerWebViewTest() { base("ComposerWebViewTest"); @@ -23,6 +22,7 @@ public class ComposerWebViewTest : ClientWebViewTestCase { 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 { assert(new ComposerWebView.EditContext("0,,,12").font_size == 12); } - public void get_html() throws Error { - string html = "

para

"; - load_body_fixture(html); + public void get_html() throws GLib.Error { + string BODY = "

para

"; + 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 = "

para

"; + 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 { - private const string COMPLETE_BODY_TEMPLATE = """
%s


"""; - private const string CLEAN_BODY_TEMPLATE = "%s


"; + public const string COMPLETE_BODY_TEMPLATE = + """
%s


"""; + public const string CLEAN_BODY_TEMPLATE = "%s


"; public ComposerPageStateTest() { base("ComposerPageStateTest"); @@ -89,7 +90,6 @@ class ComposerPageStateTest : ClientWebViewTestCase { some text """, - "--
sig", "

outerquote text

", 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 + ) ); }