Merge lp:~hideaki-t/wikkid/unicode_test into lp:wikkid

[Hideaki Takahashi]

I developed non-ASCII character support(path/URI, contents) for wikkid.
I think this contains the solution for Bug #614286(without creole parser changes)

[Tim Penhey (thumper)]

On Sat, 14 May 2011 16:53:24 you wrote:
> Hideaki Takahashi has proposed merging lp:~hideaki-t/wikkid/unicode_test
> into lp:wikkid.
>
> Requested reviews:
> Wikkid Hackers (wikkid)
>
> For more details, see:
> https://code.launchpad.net/~hideaki-t/wikkid/unicode_test/+merge/60980
>
> I developed non-ASCII character support(path/URI, contents) for wikkid.
> I think this contains the solution for Bug #614286(without creole parser
> changes)

Thanks for that. the changes generally look good.

Do you have any suggestions for testing it?

Tim

[Hideaki Takahashi (hideaki-t)]

Hello,
I'm sorry my delay.

Thank you for your review.

Ideally all operations should be tested.

I think the following tests will be performed at least.
 * file/uri operations(non-ASCII)
  * create new file
  * open(view)
  * edit & save
  * display the file name in a list view
 * contentes rendering
  * rendering the contents that contains non-ASCII characters.

Actually I have not unittested that yet. It was regression tested by "make check". and I tested it for the Japanese characters handling using web browser manually.

I propose to create(or add) Japanese version of existing tests. What do you think?
(maybe it is ugly for non japanese speakers if I add Japanese version to existing tests.)

And, I think it should be tested by Latin characters. I just tested it for Japanese characters.

Hideaki

[Aaron Bentley (abentley)]

This doesn't look correct, because the WorkingTree API accepts unicode strings (e.g. in path2id), not bytes or escaped values.

review: Needs Fixing

[Hideaki Takahashi (hideaki-t)]

Thank you for pointing that out. I did not realize that specs.
I'll check and fix the issue.

Hideaki

Unmerged revisions

61. By Hideaki Takahashi on 2011-05-14

revert #50/#51.

pass unicode string to the creole parser.

60. By Hideaki Takahashi on 2011-05-14

support non-ascii contents in preview.

59. By Hideaki Takahashi on 2011-05-14

merge trunk

58. By Hideaki Takahashi on 2011-02-18

merge trunk changes.

57. By Hideaki Takahashi on 2010-10-28

merge trunk changes

56. By Hideaki Takahashi on 2010-10-16

Convert content to utf-8.
Use safe_utf8 function provided by bzrlib to convert to utf-8.

55. By Hideaki Takahashi on 2010-10-15

don't decode string object.

54. By Hideaki Takahashi on 2010-10-15

Support MS Windows environment.
Use os module insted of bzrlib.urlutils for non bzr controled contents.

53. By Hideaki Takahashi on 2010-10-10

At first, convert the path string to unicode.

52. By Hideaki Takahashi on 2010-10-10

support non ascii filename/URI

=== modified file 'wikkid/app.py'
--- wikkid/app.py	2010-11-22 00:00:31 +0000
+++ wikkid/app.py	2011-05-14 04:53:22 +0000
@@ -64,7 +64,7 @@
         request = Request(environ)
 
         # TODO: reject requests that aren't GET or POST
-        path = urllib.unquote(request.path)
+        path = urllib.unquote(request.path).decode('utf-8')
         if path == '/favicon.ico':
             if self.skin.favicon is not None:
                 response = serve_file(self.skin.favicon)

=== modified file 'wikkid/dispatcher.py'
--- wikkid/dispatcher.py	2010-11-12 09:00:42 +0000
+++ wikkid/dispatcher.py	2011-05-14 04:53:22 +0000
@@ -13,7 +13,7 @@
 
 import os
 
-from bzrlib.urlutils import dirname, joinpath
+from bzrlib.urlutils import joinpath
 
 from zope.interface import providedBy
 
@@ -69,7 +69,7 @@
 # themselves with the view registry.
 
 def load_view_modules():
-    curr_dir = os.path.abspath(dirname(__file__))
+    curr_dir = os.path.abspath(os.path.dirname(__file__))
     view_dir = joinpath(curr_dir, 'view')
     py_files = [
         filename for filename in os.listdir(view_dir)

=== modified file 'wikkid/filestore/bzr.py'
--- wikkid/filestore/bzr.py	2010-10-17 01:47:00 +0000
+++ wikkid/filestore/bzr.py	2011-05-14 04:53:22 +0000
@@ -13,10 +13,10 @@
 
 from bzrlib.errors import BinaryFile
 from bzrlib.merge3 import Merge3
-from bzrlib.osutils import split_lines
+from bzrlib.osutils import split_lines, safe_utf8
 from bzrlib.revision import NULL_REVISION
 from bzrlib.textfile import check_text_path
-from bzrlib.urlutils import basename, dirname, joinpath
+from bzrlib.urlutils import basename, dirname, joinpath, escape
 
 from wikkid.errors import FileExists, UpdateConflicts
 from wikkid.filestore.basefile import BaseFile
@@ -66,6 +66,9 @@
             commit_message = 'No description of change given.'
         if parent_revision is None:
             parent_revision = NULL_REVISION
+        # bzr transport expects URL(ASCII only)
+        path = escape(path)
+        content = safe_utf8(content)
         # Firstly we want to lock the tree for writing.
         self.working_tree.lock_write()
         try:
@@ -118,7 +121,6 @@
         t = t.clone(dirname(path))
         t.create_prefix()
         # Put the file there.
-        # TODO: UTF-8 encode text files?
         t.put_bytes(basename(path), content)
         self.working_tree.smart_add([t.local_abspath('.')])
         self.working_tree.commit(
@@ -153,12 +155,12 @@
             if len(new_lines) > 0 and not new_lines[-1].endswith(ending):
                 new_lines[-1] += ending
             merge = Merge3(basis_lines, new_lines, current_lines)
-            result = list(merge.merge_lines()) # or merge_regions or whatever
+            result = ''.join(merge.merge_lines()) # or merge_regions or whatever
             conflicted = ('>>>>>>>' + ending) in result
             if conflicted:
-                raise UpdateConflicts(''.join(result), current_rev)
+                raise UpdateConflicts(result, current_rev)
             else:
-                wt.bzrdir.root_transport.put_bytes(path, ''.join(result))
+                wt.bzrdir.root_transport.put_bytes(path, result)
                 wt.commit(
                     message=commit_message, authors=[author],
                     specific_files=[path])

=== modified file 'wikkid/formatter/creoleformatter.py'
--- wikkid/formatter/creoleformatter.py	2010-05-12 10:39:13 +0000
+++ wikkid/formatter/creoleformatter.py	2011-05-14 04:53:22 +0000
@@ -30,4 +30,5 @@
 
     def format(self, filename, text):
         """Format the text."""
+        text = text.decode('utf-8')
         return HtmlEmitter(Parser(text, self.rules).parse()).emit()

=== modified file 'wikkid/skin/loader.py'
--- wikkid/skin/loader.py	2010-11-05 07:43:31 +0000
+++ wikkid/skin/loader.py	2011-05-14 04:53:22 +0000
@@ -39,7 +39,7 @@
             'missing': self.env.get_template('missing-page.html'),
             'missing-dir' : self.env.get_template('missing-directory.html')
             }
-        module_location = urlutils.dirname(__file__)
+        module_location = os.path.dirname(__file__)
         self.dir_name = urlutils.joinpath(module_location, skin_name)
 
     def get_template(self, template_name):

=== modified file 'wikkid/view/textfile.py'
--- wikkid/view/textfile.py	2011-04-23 08:49:27 +0000
+++ wikkid/view/textfile.py	2011-05-14 04:53:22 +0000
@@ -7,6 +7,7 @@
 """View classes to control the rendering of text pages."""
 
 import logging
+from urllib import quote
 
 from webob.exc import HTTPSeeOther
 
@@ -64,13 +65,13 @@
             self.content = content
             default_format = self.execution_context.default_format
             self.preview_content = format_content(
-                content, self.context.base_name, default_format)
+                content.encode('utf-8'), self.context.base_name, default_format)
         else:
             try:
                 self.context.put_bytes(
                     content, self.user.committer_id, rev_id, description)
 
-                raise HTTPSeeOther(location=self.context.path)
+                raise HTTPSeeOther(location=quote(self.context.path.encode('utf-8')))
             except UpdateConflicts, e:
                 # Show the edit page again.
                 logger = logging.getLogger('wikkid')

=== modified file 'wikkid/view/urls.py'
--- wikkid/view/urls.py	2010-06-16 10:29:35 +0000
+++ wikkid/view/urls.py	2011-05-14 04:53:22 +0000
@@ -32,4 +32,4 @@
     else:
         if path == '/':
             path = ''
-        return '{0}/+{1}'.format(path, view)
+        return u'{0}/+{1}'.format(path, view)