Merge lp:~stolowski/unity-lens-shopping/markup-cleaner-fix into lp:unity-lens-shopping

[Paweł Stołowski]

Fixed regular expression for capturing html tags; fixed replace_cb logic and added unit tests for MarkupCleaner.

Fixed regular expression for capturing html tags; fixed replace_cb logic and added unit tests for MarkupCleaner.

[Michal Hruby (mhr3)]

40 + internal static const string HTML_MARKUP_RE = "(</?)\\s*([^>]*?)\\s*(/?>)|(\\&(?!(([a-z]+)|(#\\d+));))";

> (?!(([a-z]+) -> perhaps (?!(\S{1,6})) instead? (the entities can be uppercase and have digits)

Pls add `<a href="wooo"><small>Click me!</small></a>` to the unit tests.

review: Needs Fixing

[lp:~stolowski/unity-lens-shopping/markup-cleaner-fix]

28. By Paweł Stołowski on 2012-10-01

Also support entities with numbers. Added test case for SMALL tag surrounded by A tag.

[Michal Hruby (mhr3)]

> (?!(([a-z]+) -> perhaps (?!(\S{1,6})) instead? (the entities can be uppercase and have digits)

As discussed on IRC pango/glib recognizes only 5 named entities, will throw an error otherwise (amp, gt, lt, apos, quote) .

[lp:~stolowski/unity-lens-shopping/markup-cleaner-fix]

29. By Paweł Stołowski on 2012-10-01

It turns out pango supports a limited set of entities, so filter all named entitites except for amp, lt, gt, quot, apos.

30. By Paweł Stołowski on 2012-10-01

Use named position in the regex for better readability.

31. By Paweł Stołowski on 2012-10-01

Lowercase entity names. Unknown entities are converted to raw string instead of discarding.

[Michal Hruby (mhr3)]

Great now! +1

review: Approve

=== modified file 'Makefile.am'
--- Makefile.am	2012-07-25 08:13:14 +0000
+++ Makefile.am	2012-10-01 15:11:29 +0000
@@ -1,4 +1,4 @@
-SUBDIRS = src data po
+SUBDIRS = src data po tests/unit
 
 #
 # Install the shopping.lens
 
=== modified file 'configure.ac'
--- configure.ac	2012-09-26 15:33:10 +0000
+++ configure.ac	2012-10-01 15:11:29 +0000
@@ -121,6 +121,7 @@
   src/config.vala
   vapi/Makefile
   po/Makefile.in
+  tests/unit/Makefile
 ])
 AC_OUTPUT
 
 
=== modified file 'src/markup-cleaner.vala'
--- src/markup-cleaner.vala	2012-09-18 09:31:25 +0000
+++ src/markup-cleaner.vala	2012-10-01 15:11:29 +0000
@@ -23,37 +23,71 @@
   {
     /**
      * Regex for capturing HTML tags. It's closely related to the logic in replace_cb,
-     * so be careful with any changes to it.
-     * First three parts of the regex capture <..> tags; there are three variants of it, because
-     * we want to capture the actual tag name, without <> and /, so <foo/>, </foo> and <foo> are
-     * treated separately in the regex.
-     * The last part captures &, unless it's &amp; already.
+     * so be careful with any changes to it; don't forget to run unit tests!
+     * First part of the regex captures <..> tags.
+     * The last part captures '&' unless it's an entity.
      */
-    internal static const string HTML_MARKUP_RE = "<(.*?)\\s*/>|</\\s*(.*?)>|<(.*?)>|(\\&(?!amp;))";
+
+    internal static const string HTML_MARKUP_RE = "(?'OPENING_BRACKET'</?)\\s*(?'TAG_NAME'[^>]*?)\\s*(?'CLOSING_BRACKET'/?>)|(?'ENTITY'\\&(?'ENTITY_NAME'[a-zA-Z0-9]+);)|(?'AMPERSAND'\\&(?!(#\\d+)))";
 
     internal static bool replace_cb (MatchInfo match, StringBuilder result)
     {
-      string? entire_substring = match.fetch (0);
-      if (match.get_match_count () > 1 && entire_substring != null)
-      {
-        if (entire_substring == "&")
+      string? entire_substring = match.fetch (0).down ();
+
+      /* html <..> tags */
+      if (match.get_match_count () == 4 && entire_substring != null)
+      {
+        string? opening_bracket = match.fetch_named ("OPENING_BRACKET");
+        string? html_tag_name = match.fetch_named ("TAG_NAME").down ();
+        string? closing_bracket = match.fetch_named ("CLOSING_BRACKET");
+
+        if (html_tag_name == "br")
+        {
+          result.append ("\n");
+        }
+        else if (html_tag_name == "b" ||
+                 html_tag_name == "i" ||
+                 html_tag_name == "u" ||
+                 html_tag_name == "s" ||
+                 html_tag_name == "tt" ||
+                 html_tag_name == "big" ||
+                 html_tag_name == "small" ||
+                 html_tag_name == "sup" ||
+                 html_tag_name == "sub")
+        {
+          result.append (entire_substring);
+        }
+        else if (html_tag_name == "strike")
+        {
+          result.append (opening_bracket + "s" + closing_bracket);
+        }
+      }
+      else if (match.get_match_count () == 6)
+      {
+        string entity_name = match.fetch_named ("ENTITY_NAME").down ();
+        if (entity_name == "amp" ||
+            entity_name == "quot" ||
+            entity_name == "apos" ||
+            entity_name == "lt" ||
+            entity_name == "gt")
+        {
+          result.append (match.fetch_named ("ENTITY").down ());
+        }
+        else if (entity_name == "nbsp")
+        {
+          result.append (" ");
+        }
+        else //append raw entity name with &amp; prefix
         {
           result.append ("&amp;");
-        }
-        else /* html <..> tags */
-        {
-          string? html_tag_name = match.fetch (1).down ();
-          if (html_tag_name == "br")
-          {
-            result.append ("\n");
-          }
-          else if (html_tag_name == "b")
-          {
-            result.append (entire_substring);
-          }
-        }
-        /* else - ignore all other html tags */
-      }
+          result.append (entity_name);
+        }
+      }
+      else if (match.get_match_count () == 7 && match.fetch_named ("AMPERSAND") == "&")
+      {
+        result.append ("&amp;");
+      }
+      /* else - ignore all other html tags */
       return false;
     }
     
 
=== added directory 'tests/unit'
=== added file 'tests/unit/Makefile.am'
--- tests/unit/Makefile.am	1970-01-01 00:00:00 +0000
+++ tests/unit/Makefile.am	2012-10-01 15:11:29 +0000
@@ -0,0 +1,28 @@
+check_PROGRAMS = test-markup-cleaner
+TESTS = $(check_PROGRAMS)
+
+test_markup_cleaner_SOURCES = \
+	test-markup-cleaner.vala \
+	$(top_srcdir)/src/markup-cleaner.vala
+	$(NULL)
+
+test_markup_cleaner_CPPFLAGS = \
+  -w \
+  $(LENS_DAEMON_CFLAGS) \
+  -I$(srcdir) \
+  $(NULL)
+
+AM_VALAFLAGS = \
+	-C \
+	--vapidir $(top_srcdir)/vapi \
+	--pkg glib-2.0 \
+	--pkg gio-2.0 \
+	--pkg gio-unix-2.0 \
+      	--target-glib=2.26 \
+	$(NULL)
+
+test_markup_cleaner_LDADD = \
+	$(LENS_DAEMON_LIBS) \
+	$(test_libs)
+
+CLEANFILES = *.stamp
 
=== added file 'tests/unit/test-markup-cleaner.vala'
--- tests/unit/test-markup-cleaner.vala	1970-01-01 00:00:00 +0000
+++ tests/unit/test-markup-cleaner.vala	2012-10-01 15:11:29 +0000
@@ -0,0 +1,184 @@
+/*
+ * Copyright (C) 2012 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by Pawel Stolowski <[e-mail]>
+ *
+ */
+
+using Unity.ShoppingLens;
+
+public class Main
+{
+  public static int main (string[] args)
+  {
+    Test.init (ref args);
+
+    Test.add_data_func ("/Unit/NoMarkup", test_no_markup);
+    Test.add_data_func ("/Unit/BrTagSupport", test_br_tag_support);
+    Test.add_data_func ("/Unit/BTagSupport", test_b_tag_support);
+    Test.add_data_func ("/Unit/ITagSupport", test_i_tag_support);
+    Test.add_data_func ("/Unit/UTagSupport", test_u_tag_support);
+    Test.add_data_func ("/Unit/TtTagSupport", test_tt_tag_support);
+    Test.add_data_func ("/Unit/STagSupport", test_s_tag_support);
+    Test.add_data_func ("/Unit/StrikeTagSupport", test_strike_tag_support);
+    Test.add_data_func ("/Unit/BigTagSupport", test_big_tag_support);
+    Test.add_data_func ("/Unit/SmallTagSupport", test_small_tag_support);
+    Test.add_data_func ("/Unit/SubTagSupport", test_sub_tag_support);
+    Test.add_data_func ("/Unit/SupTagSupport", test_sup_tag_support);
+    Test.add_data_func ("/Unit/UnsupportedTags", test_unsupported_tags);
+    Test.add_data_func ("/Unit/NestedTags", test_nested_tags);
+    Test.add_data_func ("/Unit/AmpEntitySupport", test_amp_entity);
+    Test.add_data_func ("/Unit/NbspEntitySupport", test_nbsp_entity);
+    Test.add_data_func ("/Unit/EntitiesArePreserved", test_basic_entities_are_preserved);
+    Test.add_data_func ("/Unit/UnsupportedEntitiesAreRaw", test_unsupported_entities_are_raw);
+    Test.add_data_func ("/Unit/NumericEntitiesArePreserved", test_num_entities_are_preserved);
+
+    Test.run ();
+    return 0;
+  }
+
+  internal static void test_no_markup ()
+  {
+    string input = "This is a\ntest";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "This is a\ntest");
+  }
+
+  internal static void test_br_tag_support ()
+  {
+    string input = "This is<br/> a<br />test<br>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "This is\n a\ntest\n");
+  }
+
+  internal static void test_b_tag_support ()
+  {
+    string input = "<B>T</b>his <b>is</b> a <B>test</B>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<b>T</b>his <b>is</b> a <b>test</b>");
+  }
+
+  internal static void test_i_tag_support ()
+  {
+    string input = "<I>T</i>his <i>is</i> a <I>test</I>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<i>T</i>his <i>is</i> a <i>test</i>");
+  }
+
+  internal static void test_u_tag_support ()
+  {
+    string input = "<U>T</u>his <u>is</u> a <U>test</U>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<u>T</u>his <u>is</u> a <u>test</u>");
+  }
+
+  internal static void test_tt_tag_support ()
+  {
+    string input = "<TT>T</TT>his <tt>is</tt> a <tT>test</Tt>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<tt>T</tt>his <tt>is</tt> a <tt>test</tt>");
+  }
+
+  internal static void test_s_tag_support ()
+  {
+    string input = "<S>T</s>his <s>is</s> a <S>test</S>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<s>T</s>his <s>is</s> a <s>test</s>");
+  }
+
+  internal static void test_strike_tag_support ()
+  {
+    string input = "<STRIKE>T</STRIKE>his <strike>is</strike> a <STRike>test</Strike>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<s>T</s>his <s>is</s> a <s>test</s>");
+  }
+
+  internal static void test_small_tag_support ()
+  {
+    string input = "<SMALL>T</SMALL>his <small>is</small> a <SMall>test</SmaLL>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<small>T</small>his <small>is</small> a <small>test</small>");
+  }
+
+  internal static void test_big_tag_support ()
+  {
+    string input = "<BIG>T></BIG>his <big>is</big> a <bIG>test</BiG>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<big>T></big>his <big>is</big> a <big>test</big>");
+  }
+
+  internal static void test_sub_tag_support ()
+  {
+    string input = "<SUB>T</SUB>his <sub>is</sub> a <suB>test</SuB>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<sub>T</sub>his <sub>is</sub> a <sub>test</sub>");
+  }
+
+  internal static void test_sup_tag_support ()
+  {
+    string input = "<SUP>T</SUP>his <sup>is</sup> a <suP>test</SuP>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<sup>T</sup>his <sup>is</sup> a <sup>test</sup>");
+  }
+
+  internal static void test_unsupported_tags ()
+  {
+    string input = "<foo>This</bar> is a <a href=\"http://foo.com\">test</a>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "This is a test");
+  }
+
+  internal static void test_nested_tags ()
+  {
+    string input = "<a href=\"wooo\"><small>Click me!</small></a>";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "<small>Click me!</small>");
+  }
+
+  internal static void test_amp_entity ()
+  {
+    string input = "Foo & Bar";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "Foo &amp; Bar");
+  }
+
+  internal static void test_nbsp_entity ()
+  {
+    string input = "Foo&nbsp;Bar";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "Foo Bar");
+  }
+
+  internal static void test_basic_entities_are_preserved ()
+  {
+    string input = "Foo &amp; bar &lt; &GT; &Quot; &apos;";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "Foo &amp; bar &lt; &gt; &quot; &apos;");
+  }
+
+  internal static void test_unsupported_entities_are_raw ()
+  {
+    string input = "Foo &frac14; bar &not;";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "Foo &amp;frac14 bar &amp;not");
+  }
+
+  internal static void test_num_entities_are_preserved ()
+  {
+    string input = "Foo&#160;bar &#8364;";
+    string result = MarkupCleaner.html_to_pango_markup (input);
+    assert (result == "Foo&#160;bar &#8364;");
+  }
+}