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.
This commit is contained in:
Michael James Gratton 2017-01-02 10:23:35 +11:00
parent 57f10446a9
commit 17fda41b85
8 changed files with 341 additions and 99 deletions

View file

@ -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;
}
/**

View file

@ -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
)

View file

@ -0,0 +1,39 @@
/*
* Copyright 2016 Michael Gratton <mike@vee.net>
*
* 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<V> : 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();
}
}
}

View file

@ -0,0 +1,166 @@
/*
* Copyright 2016 Michael Gratton <mike@vee.net>
*
* 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<ComposerWebView> {
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 = "<p>para</p>";
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 + "<br><br>");
} catch (Error err) {
print("Error: %s\n", err.message);
assert_not_reached();
}
}
public void get_text() {
load_body_fixture("<p>para</p>");
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("<p>pre</p> <blockquote><p>quote</p></blockquote> <p>post</p>");
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("<p>pre</p> <blockquote><p>quote1</p> <blockquote><p>quote2</p></blockquote></blockquote> <p>post</p>");
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("""
<p>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.</p>
""");
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("""
<blockquote><p>A long, long, long, long, long, long line. Well, longer than MAX_BREAKABLE_LEN at least.</p></blockquote>
<p>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.</p>""");
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 &lt;mike@vee.net&gt; wrote:<br>
<blockquote type="cite">long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,&nbsp;long,
</blockquote><br>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,&nbsp;<div style="white-space: pre;">
</div>
""");
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 <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,
""");
} 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();
}
}
}

View file

@ -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<AsyncResult> async_results = new AsyncQueue<AsyncResult>();
class ComposerPageStateTest : ClientWebViewTestCase<ComposerWebView> {
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("<p>para</p>");
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("<p>pre</p> <blockquote><p>quote</p></blockquote> <p>post</p>");
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("<p>pre</p> <blockquote><p>quote1</p> <blockquote><p>quote2</p></blockquote></blockquote> <p>post</p>");
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;
}
}

View file

@ -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");

View file

@ -18,12 +18,14 @@
*
* Author:
* Julien Peeters <contact@julienpeeters.fr>
* Michael Gratton <mike@vee.net>
*/
public abstract class Gee.TestCase : Object {
private GLib.TestSuite suite;
private Adaptor[] adaptors = new Adaptor[0];
private AsyncQueue<AsyncResult> async_results = new AsyncQueue<AsyncResult>();
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; }

View file

@ -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 &nbsp into U+00A0.
return text.replace(new RegExp(" ", "g"), " ");
};
var geary = new ComposerPageState();
window.onload = function() {