From 17fda41b85d07877908b85b864d707a1bdfd9fa6 Mon Sep 17 00:00:00 2001 From: Michael James Gratton Date: Mon, 2 Jan 2017 10:23:35 +1100 Subject: [PATCH] Fix non-breaking spaces breaking formatting in sent messages. This ensures that non-breaking space chars (not HTML entities) are removed from text obtainined from the composer, and moves the F=F text formatting from JS back to Vala, to minimimse the JS footprint and return to using the old (working) version again. * src/client/composer/composer-web-view.vala (ClientWebView::get_text): Restore the old F=F formatting code previously in webkit-util, apply it to plain text obtained from the composer. * test/client/components/client-web-view-test-case.vala: New base class for tests involving ClientWebView. * test/client/composer/composer-web-view-test.vala: New tests for ComposerWebView::get_html and ::get_text. * test/js/composer-page-state-test.vala: Reworked to use ClientWebViewTestCase, updated tests now that JS is returning QUOTE_MARKER-delinated text, not F=F text. * test/testcase.vala (TestCase): Move ::async_complete and ::async_result from ComposerPageStateTest so all test cases can test async code. * test/CMakeLists.txt: Add new source files. * test/main.vala (main): Add new test. * ui/composer-web-view.js: Update doc comments, remove F=F code, break out non-breaking space replacement so it can be tested. --- src/client/composer/composer-web-view.vala | 46 ++++- test/CMakeLists.txt | 2 + .../components/client-web-view-test-case.vala | 39 ++++ .../composer/composer-web-view-test.vala | 166 ++++++++++++++++++ test/js/composer-page-state-test.vala | 73 ++++---- test/main.vala | 1 + test/testcase.vala | 15 ++ ui/composer-web-view.js | 98 ++++------- 8 files changed, 341 insertions(+), 99 deletions(-) create mode 100644 test/client/components/client-web-view-test-case.vala create mode 100644 test/client/composer/composer-web-view-test.vala diff --git a/src/client/composer/composer-web-view.vala b/src/client/composer/composer-web-view.vala index 50abc1a5..22de6bda 100644 --- a/src/client/composer/composer-web-view.vala +++ b/src/client/composer/composer-web-view.vala @@ -204,13 +204,55 @@ public class ComposerWebView : ClientWebView { } /** - * Returns the editor content as a plain text string. + * Returns the editor text as RFC 3676 format=flowed text. */ public async string? get_text() throws Error { WebKit.JavascriptResult result = yield this.run_javascript( "geary.getText();", null ); - return WebKitUtil.to_string(result); + + string body_text = WebKitUtil.to_string(result); + string[] lines = body_text.split("\n"); + GLib.StringBuilder flowed = new GLib.StringBuilder.sized(body_text.length); + foreach (string line in lines) { + // Strip trailing whitespace, so it doesn't look like a + // flowed line. But the signature separator "-- " is + // special, so leave that alone. + if (line != "-- ") + line = line.chomp(); + int quote_level = 0; + while (line[quote_level] == Geary.RFC822.Utils.QUOTE_MARKER) + quote_level += 1; + line = line[quote_level:line.length]; + string prefix = quote_level > 0 ? string.nfill(quote_level, '>') + " " : ""; + int max_len = 72 - prefix.length; + + do { + int start_ind = 0; + if (quote_level == 0 && + (line.has_prefix(">") || line.has_prefix("From"))) { + line = " " + line; + start_ind = 1; + } + + int cut_ind = line.length; + if (cut_ind > max_len) { + string beg = line[0:max_len]; + cut_ind = beg.last_index_of(" ", start_ind) + 1; + if (cut_ind == 0) { + cut_ind = line.index_of(" ", start_ind) + 1; + if (cut_ind == 0) + cut_ind = line.length; + if (cut_ind > 998 - prefix.length) + cut_ind = 998 - prefix.length; + } + } + flowed.append(prefix + line[0:cut_ind] + "\n"); + line = line[cut_ind:line.length]; + } while (line.length > 0); + } + + return flowed.str; } /** diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6e2e7a2e..87a977ed 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -13,6 +13,8 @@ set(TEST_SRC engine/util-html-test.vala client/application/geary-configuration-test.vala + client/components/client-web-view-test-case.vala + client/composer/composer-web-view-test.vala js/composer-page-state-test.vala ) diff --git a/test/client/components/client-web-view-test-case.vala b/test/client/components/client-web-view-test-case.vala new file mode 100644 index 00000000..636a6ada --- /dev/null +++ b/test/client/components/client-web-view-test-case.vala @@ -0,0 +1,39 @@ +/* + * Copyright 2016 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +// Defined by CMake build script. +extern const string _BUILD_ROOT_DIR; + +public abstract class ClientWebViewTestCase : Gee.TestCase { + + protected V test_view = null; + + public ClientWebViewTestCase(string name) { + base(name); + } + + public override void set_up() { + ClientWebView.init_web_context(File.new_for_path(_BUILD_ROOT_DIR).get_child("src"), true); + try { + ClientWebView.load_scripts(); + } catch (Error err) { + assert_not_reached(); + } + this.test_view = set_up_test_view(); + } + + protected abstract V set_up_test_view(); + + protected virtual void load_body_fixture(string? html = null) { + ClientWebView client_view = (ClientWebView) this.test_view; + client_view.load_html(html); + while (client_view.is_loading) { + Gtk.main_iteration(); + } + } + +} diff --git a/test/client/composer/composer-web-view-test.vala b/test/client/composer/composer-web-view-test.vala new file mode 100644 index 00000000..034488b6 --- /dev/null +++ b/test/client/composer/composer-web-view-test.vala @@ -0,0 +1,166 @@ +/* + * Copyright 2016 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +public class ComposerWebViewTest : ClientWebViewTestCase { + + public ComposerWebViewTest() { + base("ComposerWebViewTest"); + add_test("get_html", get_html); + add_test("get_text", get_text); + add_test("get_text_with_quote", get_text_with_quote); + add_test("get_text_with_nested_quote", get_text_with_nested_quote); + add_test("get_text_with_long_line", get_text_with_long_line); + add_test("get_text_with_long_quote", get_text_with_long_quote); + add_test("get_text_with_nbsp", get_text_with_nbsp); + } + + public void get_html() { + string html = "

para

"; + load_body_fixture(html); + this.test_view.get_html.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_html.end(async_result()) == html + "

"); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + public void get_text() { + load_body_fixture("

para

"); + this.test_view.get_text.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_text.end(async_result()) == "para\n\n\n\n\n"); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + public void get_text_with_quote() { + load_body_fixture("

pre

quote

post

"); + this.test_view.get_text.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_text.end(async_result()) == + "pre\n\n> quote\n> \npost\n\n\n\n\n"); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + public void get_text_with_nested_quote() { + load_body_fixture("

pre

quote1

quote2

post

"); + this.test_view.get_text.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_text.end(async_result()) == + "pre\n\n> quote1\n> \n>> quote2\n>> \npost\n\n\n\n\n"); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + public void get_text_with_long_line() { + load_body_fixture(""" +

A long, long, long, long, long, long para. Well, longer than MAX_BREAKABLE_LEN +at least. Really long, long, long, long, long long, long long, long long, long.

+"""); + this.test_view.get_text.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_text.end(async_result()) == +"""A long, long, long, long, long, long para. Well, longer than +MAX_BREAKABLE_LEN at least. Really long, long, long, long, long long, +long long, long long, long. + + + + +"""); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + public void get_text_with_long_quote() { + load_body_fixture(""" +

A long, long, long, long, long, long line. Well, longer than MAX_BREAKABLE_LEN at least.

+ +

A long, long, long, long, long, long para. Well, longer than MAX_BREAKABLE_LEN +at least. Really long, long, long, long, long long, long long, long long, long.

"""); + this.test_view.get_text.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_text.end(async_result()) == +"""> A long, long, long, long, long, long line. Well, longer than +> MAX_BREAKABLE_LEN at least. +> +A long, long, long, long, long, long para. Well, longer than +MAX_BREAKABLE_LEN at least. Really long, long, long, long, long long, +long long, long long, long. + + + + +"""); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + public void get_text_with_nbsp() { + load_body_fixture("""On Sun, Jan 1, 2017 at 9:55 PM, Michael Gratton <mike@vee.net> wrote:
+
long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, +

long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, long, 
+
+ +"""); + this.test_view.get_text.begin((obj, ret) => { async_complete(ret); }); + try { + assert(this.test_view.get_text.end(async_result()) == +"""On Sun, Jan 1, 2017 at 9:55 PM, Michael Gratton wrote: +> long, long, long, long, long, long, long, long, long, long, long, +> long, long, long, long, long, long, long, long, long, long, long, +> long, long, long, long, long, long, long, long, long, long, long, +> long, long, long, long, long, long, long, long, long, long, long, +> long, long, long, long, long, + +long, long, long, long, long, long, long, long, long, long, long, long, +long, long, long, long, long, long, long, long, long, long, long, long, +long, long, long, long, long, long, long, long, long, long, long, long, +long, long, long, long, long, long, long, long, long, long, long, long, +long, long, long, long, long, long, long, long, long, long, + + + + +"""); + } catch (Error err) { + print("Error: %s\n", err.message); + assert_not_reached(); + } + } + + protected override ComposerWebView set_up_test_view() { + try { + ComposerWebView.load_resources(); + } catch (Error err) { + assert_not_reached(); + } + Configuration config = new Configuration(GearyApplication.APP_ID); + return new ComposerWebView(config); + } + + protected override void load_body_fixture(string? html = null) { + this.test_view.load_html(html, null, 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 485c6955..433d4ba8 100644 --- a/test/js/composer-page-state-test.vala +++ b/test/js/composer-page-state-test.vala @@ -5,13 +5,7 @@ * (version 2.1 or later). See the COPYING file in this distribution. */ -// Defined by CMake build script. -extern const string _BUILD_ROOT_DIR; - -class ComposerPageStateTest : Gee.TestCase { - - private ComposerWebView test_view = null; - private AsyncQueue async_results = new AsyncQueue(); +class ComposerPageStateTest : ClientWebViewTestCase { public ComposerPageStateTest() { base("ComposerPageStateTest"); @@ -21,19 +15,7 @@ class ComposerPageStateTest : Gee.TestCase { add_test("get_text_with_nested_quote", get_text_with_nested_quote); add_test("resolve_nesting", resolve_nesting); add_test("quote_lines", quote_lines); - } - - public override void set_up() { - ClientWebView.init_web_context(File.new_for_path(_BUILD_ROOT_DIR).get_child("src"), true); - try { - ClientWebView.load_scripts(); - ComposerWebView.load_resources(); - } catch (Error err) { - print("\nComposerPageStateTest::set_up: %s\n", err.message); - assert_not_reached(); - } - Configuration config = new Configuration(GearyApplication.APP_ID); - this.test_view = new ComposerWebView(config); + add_test("replace_non_breaking_space", replace_non_breaking_space); } public void get_html() { @@ -53,7 +35,7 @@ class ComposerPageStateTest : Gee.TestCase { public void get_text() { load_body_fixture("

para

"); try { - assert(run_javascript(@"window.geary.getText();") == "para\n\n\n\n\n"); + assert(run_javascript(@"window.geary.getText();") == "para\n\n\n\n"); } catch (Geary.JS.Error err) { print("Geary.JS.Error: %s", err.message); assert_not_reached(); @@ -64,10 +46,11 @@ class ComposerPageStateTest : Gee.TestCase { } public void get_text_with_quote() { + unichar q_marker = Geary.RFC822.Utils.QUOTE_MARKER; load_body_fixture("

pre

quote

post

"); try { assert(run_javascript(@"window.geary.getText();") == - "pre\n\n> quote\n> \npost\n\n\n\n\n"); + @"pre\n\n$(q_marker)quote\n$(q_marker)\npost\n\n\n\n"); } catch (Geary.JS.Error err) { print("Geary.JS.Error: %s", err.message); assert_not_reached(); @@ -78,10 +61,11 @@ class ComposerPageStateTest : Gee.TestCase { } public void get_text_with_nested_quote() { + unichar q_marker = Geary.RFC822.Utils.QUOTE_MARKER; load_body_fixture("

pre

quote1

quote2

post

"); try { assert(run_javascript(@"window.geary.getText();") == - "pre\n\n> quote1\n> \n>> quote2\n>> \npost\n\n\n\n\n"); + @"pre\n\n$(q_marker)quote1\n$(q_marker)\n$(q_marker)$(q_marker)quote2\n$(q_marker)$(q_marker)\npost\n\n\n\n"); } catch (Geary.JS.Error err) { print("Geary.JS.Error: %s", err.message); assert_not_reached(); @@ -144,7 +128,35 @@ class ComposerPageStateTest : Gee.TestCase { } } - protected void load_body_fixture(string? html = null) { + public void replace_non_breaking_space() { + load_body_fixture(); + string single_nbsp = "a b"; + string multiple_nbsp = "a b c"; + try { + assert(run_javascript(@"ComposerPageState.replaceNonBreakingSpace('$(single_nbsp)');") == + "a b"); + assert(run_javascript(@"ComposerPageState.replaceNonBreakingSpace('$(multiple_nbsp)');") == + "a b c"); + } catch (Geary.JS.Error err) { + print("Geary.JS.Error: %s\n", err.message); + assert_not_reached(); + } catch (Error err) { + print("WKError: %s\n", err.message); + assert_not_reached(); + } + } + + protected override ComposerWebView set_up_test_view() { + try { + ComposerWebView.load_resources(); + } catch (Error err) { + assert_not_reached(); + } + Configuration config = new Configuration(GearyApplication.APP_ID); + return new ComposerWebView(config); + } + + protected override void load_body_fixture(string? html = null) { this.test_view.load_html(html, null, false); while (this.test_view.is_loading) { Gtk.main_iteration(); @@ -161,17 +173,4 @@ class ComposerPageStateTest : Gee.TestCase { return WebKitUtil.to_string(result); } - protected void async_complete(AsyncResult result) { - this.async_results.push(result); - } - - protected AsyncResult async_result() { - AsyncResult? result = null; - while (result == null) { - Gtk.main_iteration(); - result = this.async_results.try_pop(); - } - return result; - } - } diff --git a/test/main.vala b/test/main.vala index 5b733be0..c82e5692 100644 --- a/test/main.vala +++ b/test/main.vala @@ -45,6 +45,7 @@ int main(string[] args) { TestSuite client = new TestSuite("client"); + client.add_suite(new ComposerWebViewTest().get_suite()); client.add_suite(new ConfigurationTest().get_suite()); TestSuite js = new TestSuite("js"); diff --git a/test/testcase.vala b/test/testcase.vala index 83a37ae3..c6f77695 100644 --- a/test/testcase.vala +++ b/test/testcase.vala @@ -18,12 +18,14 @@ * * Author: * Julien Peeters + * Michael Gratton */ public abstract class Gee.TestCase : Object { private GLib.TestSuite suite; private Adaptor[] adaptors = new Adaptor[0]; + private AsyncQueue async_results = new AsyncQueue(); public delegate void TestMethod (); @@ -51,6 +53,19 @@ public abstract class Gee.TestCase : Object { return this.suite; } + protected void async_complete(AsyncResult result) { + this.async_results.push(result); + } + + protected AsyncResult async_result() { + AsyncResult? result = null; + while (result == null) { + Gtk.main_iteration(); + result = this.async_results.try_pop(); + } + return result; + } + private class Adaptor { [CCode (notify = false)] public string name { get; private set; } diff --git a/ui/composer-web-view.js b/ui/composer-web-view.js index 324c5c3d..26e16f95 100644 --- a/ui/composer-web-view.js +++ b/ui/composer-web-view.js @@ -66,7 +66,7 @@ ComposerPageState.prototype = { return document.getElementById(ComposerPageState.BODY_ID).innerHTML; }, getText: function() { - return ComposerPageState.htmlToFlowedText( + return ComposerPageState.htmlToQuotedText( document.getElementById(ComposerPageState.BODY_ID) ); }, @@ -93,15 +93,27 @@ ComposerPageState.prototype = { }; /** - * Convert a HTML DOM tree to RFC 3676 format=flowed text. + * Convert a HTML DOM tree to plain text with delineated quotes. * - * This will modify/reset the DOM. + * Lines are delinated using LF. Quoted lines are prefixed with + * `ComposerPageState.QUOTE_MARKER`, where the number of markers + * indicates the depth of nesting of the quote. + * + * This will modify/reset the DOM, since it ultimately requires + * stuffing `QUOTE_MARKER` into existing paragraphs and getting it + * back out in a way that preserves the visual presentation. */ -ComposerPageState.htmlToFlowedText = function(root) { - var savedDoc = root.innerHTML; - var blockquotes = root.querySelectorAll("blockquote"); - var nbq = blockquotes.length; - var bqtexts = new Array(nbq); +ComposerPageState.htmlToQuotedText = function(root) { + // XXX It would be nice to just clone the root and modify that, or + // see if we can implement this some other way so as to not modify + // the DOM at all, but currently unit test show that the results + // are not the same if we work on a clone, likely because of the + // use of HTMLElement::innerText. Need to look into it more. + + let savedDoc = root.innerHTML; + let blockquotes = root.querySelectorAll("blockquote"); + let nbq = blockquotes.length; + let bqtexts = new Array(nbq); // Get text of blockquotes and pull them out of DOM. They are // replaced with tokens deliminated with the characters @@ -132,61 +144,14 @@ ComposerPageState.htmlToFlowedText = function(root) { ); } - // Reassemble plain text out of parts, replace non-breaking - // space with regular space - var doctext = ComposerPageState.resolveNesting( - root.innerText, bqtexts - ).replace("\xc2\xa0", " "); + // Reassemble plain text out of parts, and replace non-breaking + // space with regular space. + let text = ComposerPageState.resolveNesting(root.innerText, bqtexts) - // Reassemble DOM + // Reassemble DOM now we have the plain text root.innerHTML = savedDoc; - // Wrap, space stuff, quote - var lines = doctext.split("\n"); - flowed = []; - for (let line of lines) { - // Strip trailing whitespace, so it doesn't look like a flowed - // line. But the signature separator "-- " is special, so - // leave that alone. - if (line != "-- ") { - line = line.trimRight(); - } - let quoteLevel = 0; - while (line[quoteLevel] == ComposerPageState.QUOTE_MARKER) { - quoteLevel += 1; - } - line = line.substr(quoteLevel, line.length); - let prefix = quoteLevel > 0 ? '>'.repeat(quoteLevel) + " " : ""; - let maxLen = 72 - prefix.length; - - do { - let startInd = 0; - if (quoteLevel == 0 && - (line.startsWith(">") || line.startsWith("From"))) { - line = " " + line; - startInd = 1; - } - - let cutInd = line.length; - if (cutInd > maxLen) { - let beg = line.substr(0, maxLen); - cutInd = beg.lastIndexOf(" ", startInd) + 1; - if (cutInd == 0) { - cutInd = line.indexOf(" ", startInd) + 1; - if (cutInd == 0) { - cutInd = line.length; - } - if (cutInd > 998 - prefix.length) { - cutInd = 998 - prefix.length; - } - } - } - flowed.push(prefix + line.substr(0, cutInd) + "\n"); - line = line.substr(cutInd, line.length); - } while (line.length > 0); - } - - return flowed.join(""); + return ComposerPageState.replaceNonBreakingSpace(text); }; ComposerPageState.resolveNesting = function(text, values) { @@ -219,6 +184,9 @@ ComposerPageState.resolveNesting = function(text, values) { }); }; +/** + * Prefixes each NL-delineated line with `ComposerPageState.QUOTE_MARKER`. + */ ComposerPageState.quoteLines = function(text) { let lines = text.split("\n"); for (let i = 0; i < lines.length; i++) @@ -226,6 +194,16 @@ ComposerPageState.quoteLines = function(text) { return lines.join("\n"); }; +/** + * Converts all non-breaking space chars to plain spaces. + */ +ComposerPageState.replaceNonBreakingSpace = function(text) { + // XXX this is a separate function for unit testing - since when + // running as a unit test, HTMLElement.innerText appears to not + // convert   into U+00A0. + return text.replace(new RegExp(" ", "g"), " "); +}; + var geary = new ComposerPageState(); window.onload = function() {