[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-18 Thread noreply
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343465
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-18 Thread Phill
Review: Approve

Good, thanks for you patience
-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343465
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Tim Bentley
Review: Approve

Looks good to me.
-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343465
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna
Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-newline-bug into 
lp:openlp.

Requested reviews:
  Phill (phill-ridout)
  Tim Bentley (trb143)
Related bugs:
  Bug #1727517 in OpenLP: "Unicode control chars causes song importer to crash"
  https://bugs.launchpad.net/openlp/+bug/1727517

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343465

Fix the fix for 1727517
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2018-02-24 16:10:02 +
+++ openlp/core/common/__init__.py	2018-04-17 19:37:10 +
@@ -44,7 +44,7 @@
 
 FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
 SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
-CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
+CONTROL_CHARS = re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
 IMAGES_FILTER = None
 REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
@@ -471,15 +471,15 @@
 log.exception('Error detecting file encoding')
 
 
-def normalize_str(irreg_str):
+def normalize_str(irregular_string):
 """
 Normalize the supplied string. Remove unicode control chars and tidy up white space.
 
-:param str irreg_str: The string to normalize.
+:param str irregular_string: The string to normalize.
 :return: The normalized string
 :rtype: str
 """
-irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
-irreg_str = CONTROL_CHARS.sub('', irreg_str)
-irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
-return WHITESPACE_REGEX.sub(' ', irreg_str)
+irregular_string = irregular_string.translate(REPLACMENT_CHARS_MAP)
+irregular_string = CONTROL_CHARS.sub('', irregular_string)
+irregular_string = NEW_LINE_REGEX.sub('\n', irregular_string)
+return WHITESPACE_REGEX.sub(' ', irregular_string)

=== modified file 'tests/functional/openlp_core/common/test_common.py'
--- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +
+++ tests/functional/openlp_core/common/test_common.py	2018-04-17 19:37:10 +
@@ -25,8 +25,8 @@
 from unittest import TestCase
 from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, is_win, \
-path_to_module, trace_error_handler
+from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, \
+is_win, normalize_str, path_to_module, trace_error_handler
 from openlp.core.common.path import Path
 
 
@@ -211,6 +211,30 @@
 assert is_win() is False, 'is_win() should return False'
 assert is_macosx() is False, 'is_macosx() should return False'
 
+def test_normalize_str_leaves_newlines(self):
+# GIVEN: a string containing newlines
+string = 'something\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: string is unchanged
+assert normalized_string == string
+
+def test_normalize_str_removes_null_byte(self):
+# GIVEN: a string containing a null byte
+string = 'somet\x00hing'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: nullbyte is removed
+assert normalized_string == 'something'
+
+def test_normalize_str_replaces_crlf_with_lf(self):
+# GIVEN: a string containing crlf
+string = 'something\r\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: crlf is replaced with lf
+assert normalized_string == 'something\nelse'
+
 def test_clean_button_text(self):
 """
 Test the clean_button_text() function.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Superseded

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343422
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Tim Bentley
Review: Needs Fixing

See comments inline

lp:~thelinuxguy/openlp/fix-newline-bug (revision 2822)
https://ci.openlp.io/job/Branch-01-Pull/2502/  [SUCCESS]
https://ci.openlp.io/job/Branch-02a-Linux-Tests/2403/  [SUCCESS]
https://ci.openlp.io/job/Branch-02b-macOS-Tests/189/   [FAILURE]
https://ci.openlp.io/job/Branch-03a-Build-Source/101/  [SUCCESS]
https://ci.openlp.io/job/Branch-03b-Build-macOS/94/[SUCCESS]
https://ci.openlp.io/job/Branch-04a-Code-Analysis/1563/[SUCCESS]
https://ci.openlp.io/job/Branch-04b-Test-Coverage/1376/[SUCCESS]


Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-17 10:28:19 +
> @@ -471,15 +471,15 @@
>  log.exception('Error detecting file encoding')
>  
>  
> -def normalize_str(irreg_str):
> +def normalize_str(string):

Field names of string are not meaningful.  irreg_string is more correct

>  """
>  Normalize the supplied string. Remove unicode control chars and tidy up 
> white space.
>  
> -:param str irreg_str: The string to normalize.
> +:param str string: The string to normalize.
>  :return: The normalized string
>  :rtype: str
>  """
> -irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
> -irreg_str = CONTROL_CHARS.sub('', irreg_str)
> -irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
> -return WHITESPACE_REGEX.sub(' ', irreg_str)
> +string = string.translate(REPLACMENT_CHARS_MAP)
> +string = CONTROL_CHARS.sub('', string)
> +string = NEW_LINE_REGEX.sub('\n', string)
> +return WHITESPACE_REGEX.sub(' ', string)


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343422
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna
Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-newline-bug into 
lp:openlp.

Requested reviews:
  Phill (phill-ridout)
  Tim Bentley (trb143)
Related bugs:
  Bug #1727517 in OpenLP: "Unicode control chars causes song importer to crash"
  https://bugs.launchpad.net/openlp/+bug/1727517

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343422

Fix the fix for 1727517
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2018-02-24 16:10:02 +
+++ openlp/core/common/__init__.py	2018-04-17 10:28:19 +
@@ -44,7 +44,7 @@
 
 FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
 SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
-CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
+CONTROL_CHARS = re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
 IMAGES_FILTER = None
 REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
@@ -471,15 +471,15 @@
 log.exception('Error detecting file encoding')
 
 
-def normalize_str(irreg_str):
+def normalize_str(string):
 """
 Normalize the supplied string. Remove unicode control chars and tidy up white space.
 
-:param str irreg_str: The string to normalize.
+:param str string: The string to normalize.
 :return: The normalized string
 :rtype: str
 """
-irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
-irreg_str = CONTROL_CHARS.sub('', irreg_str)
-irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
-return WHITESPACE_REGEX.sub(' ', irreg_str)
+string = string.translate(REPLACMENT_CHARS_MAP)
+string = CONTROL_CHARS.sub('', string)
+string = NEW_LINE_REGEX.sub('\n', string)
+return WHITESPACE_REGEX.sub(' ', string)

=== modified file 'tests/functional/openlp_core/common/test_common.py'
--- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +
+++ tests/functional/openlp_core/common/test_common.py	2018-04-17 10:28:19 +
@@ -25,8 +25,8 @@
 from unittest import TestCase
 from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, is_win, \
-path_to_module, trace_error_handler
+from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, \
+is_win, normalize_str, path_to_module, trace_error_handler
 from openlp.core.common.path import Path
 
 
@@ -211,6 +211,30 @@
 assert is_win() is False, 'is_win() should return False'
 assert is_macosx() is False, 'is_macosx() should return False'
 
+def test_normalize_str_leaves_newlines(self):
+# GIVEN: a string containing newlines
+string = 'something\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: string is unchanged
+assert normalized_string == string
+
+def test_normalize_str_removes_null_byte(self):
+# GIVEN: a string containing a null byte
+string = 'somet\x00hing'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: nullbyte is removed
+assert normalized_string == 'something'
+
+def test_normalize_str_replaces_crlf_with_lf(self):
+# GIVEN: a string containing crlf
+string = 'something\r\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: crlf is replaced with lf
+assert normalized_string == 'something\nelse'
+
 def test_clean_button_text(self):
 """
 Test the clean_button_text() function.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-17 Thread Simon Hanna
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Superseded

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343306
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-16 Thread Phill
> One small typo, and a couple comments that need changing.

Sorry!
-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343306
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-16 Thread Phill
Review: Needs Fixing

One small typo, and a couple comments that need changing.

Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-16 11:33:07 +
> @@ -44,7 +44,7 @@
>  
>  FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
>  SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
> -CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
> +CONTROL_CHARS = re.compile(r'[\x00-\08\x0B\x0C\x0E-\x1F\x7F-\x9F]')

\08 should be \x08

>  INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
>  IMAGES_FILTER = None
>  REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', 
> '\u201c': '"', '\u201d': '"', '\u2026': '...',
> 
> === modified file 'tests/functional/openlp_core/common/test_common.py'
> --- tests/functional/openlp_core/common/test_common.py2017-12-29 
> 09:15:48 +
> +++ tests/functional/openlp_core/common/test_common.py2018-04-16 
> 11:33:07 +
> @@ -211,6 +211,30 @@
>  assert is_win() is False, 'is_win() should return False'
>  assert is_macosx() is False, 'is_macosx() should return False'
>  
> +def test_normalize_str_leaves_newlines(self):
> +# GIVEN: a string containing newlines
> +string = 'something\nelse'
> +# WHEN: normalize is called
> +normalized_string = normalize_str(string)
> +# THEN: string is unchanged
> +assert normalized_string == string
> +
> +def test_normalize_str_removes_null_byte(self):
> +# GIVEN: a string containing newlines

This comment is incorrect

> +string = 'somet\x00hing'
> +# WHEN: normalize is called
> +normalized_string = normalize_str(string)
> +# THEN: string is unchanged

so is this

> +assert normalized_string == 'something'
> +
> +def test_normalize_str_replaces_crlf_with_lf(self):
> +# GIVEN: a string containing crlf
> +string = 'something\r\nelse'
> +# WHEN: normalize is called
> +normalized_string = normalize_str(string)
> +# THEN: crlf is replaced with lf
> +assert normalized_string == 'something\nelse'
> +
>  def test_clean_button_text(self):
>  """
>  Test the clean_button_text() function.


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343306
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-16 Thread Simon Hanna
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Superseded

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343277
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-16 Thread Simon Hanna
I wonder what the reason behind replacing vertial tabs with new lines is...

Looking at where the regex is being used I'm inclined to say the whole thing 
needs to be reworked.

It's also used in the filename cleaning where linefeeds are strange, but then 
again I don't know what the input always is.

I'm changing it to what you request, but you should really take a serious look 
at it. From what I see, you are the last one that touched the invalid file 
regex, I highly doubt this is what you actually want to happen. If it is, it is 
a mystery to me...
-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343277
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-16 Thread Phill
Review: Needs Fixing

I've done some research in to this. Officially the only code points allowed are:

"
[2] Char   ::=  #x9 | #xA | #xD | [#x20-#xD7FF] | 
[#xE000-#xFFFD] | [#x1-#x10]  /* any Unicode character, excluding the 
surrogate blocks, FFFE, and . */
"   ( https://www.w3.org/TR/REC-xml/#charsets)

So going on this our regex should be:

re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')

we probably should filter out the other code points too.

Its worth noting that REPLACMENT_CHARS_MAP replaces the vertical tab and form 
feed chars with "\n\n" before the CONTROL_CHARS regex filters them out!

So in summary please replace the regex with the one in the inline comment. 
(Note: its not tested)


Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-15 14:10:22 +
> @@ -44,7 +44,7 @@
>  
>  FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
>  SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
> -CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
> +CONTROL_CHARS = re.compile(r'[\x00-\08\x0E-\x1F\x7F-\x9F]')

CONTROL_CHARS = re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')

>  INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
>  IMAGES_FILTER = None
>  REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', 
> '\u201c': '"', '\u201d': '"', '\u2026': '...',


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343277
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-15 Thread Simon Hanna
Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-newline-bug into 
lp:openlp.

Requested reviews:
  Phill (phill-ridout)
  Tim Bentley (trb143)
Related bugs:
  Bug #1727517 in OpenLP: "Unicode control chars causes song importer to crash"
  https://bugs.launchpad.net/openlp/+bug/1727517

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343277

Fix the fix for 1727517
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2018-02-24 16:10:02 +
+++ openlp/core/common/__init__.py	2018-04-15 14:10:22 +
@@ -44,7 +44,7 @@
 
 FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
 SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
-CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
+CONTROL_CHARS = re.compile(r'[\x00-\08\x0E-\x1F\x7F-\x9F]')
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
 IMAGES_FILTER = None
 REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
@@ -471,15 +471,15 @@
 log.exception('Error detecting file encoding')
 
 
-def normalize_str(irreg_str):
+def normalize_str(string):
 """
 Normalize the supplied string. Remove unicode control chars and tidy up white space.
 
-:param str irreg_str: The string to normalize.
+:param str string: The string to normalize.
 :return: The normalized string
 :rtype: str
 """
-irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
-irreg_str = CONTROL_CHARS.sub('', irreg_str)
-irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
-return WHITESPACE_REGEX.sub(' ', irreg_str)
+string = string.translate(REPLACMENT_CHARS_MAP)
+string = CONTROL_CHARS.sub('', string)
+string = NEW_LINE_REGEX.sub('\n', string)
+return WHITESPACE_REGEX.sub(' ', string)

=== modified file 'tests/functional/openlp_core/common/test_common.py'
--- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +
+++ tests/functional/openlp_core/common/test_common.py	2018-04-15 14:10:22 +
@@ -25,8 +25,8 @@
 from unittest import TestCase
 from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, is_win, \
-path_to_module, trace_error_handler
+from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, \
+is_win, normalize_str, path_to_module, trace_error_handler
 from openlp.core.common.path import Path
 
 
@@ -211,6 +211,30 @@
 assert is_win() is False, 'is_win() should return False'
 assert is_macosx() is False, 'is_macosx() should return False'
 
+def test_normalize_str_leaves_newlines(self):
+# GIVEN: a string containing newlines
+string = 'something\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: string is unchanged
+assert normalized_string == string
+
+def test_normalize_str_removes_null_byte(self):
+# GIVEN: a string containing newlines
+string = 'somet\x00hing'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: string is unchanged
+assert normalized_string == 'something'
+
+def test_normalize_str_replaces_crlf_with_lf(self):
+# GIVEN: a string containing crlf
+string = 'something\r\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(string)
+# THEN: crlf is replaced with lf
+assert normalized_string == 'something\nelse'
+
 def test_clean_button_text(self):
 """
 Test the clean_button_text() function.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-15 Thread Simon Hanna
The proposal to merge lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp has 
been updated.

Status: Needs review => Superseded

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343256
-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-15 Thread Simon Hanna
will update shortly

Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-14 20:05:07 +
> @@ -44,7 +44,7 @@
>  
>  FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
>  SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
> -CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
> +CONTROL_CHARS = re.compile(r'[\x00-\08\x0E-\x1F\x7F-\x9F]')

I said, I don't know what all these replacement are actually about. I just 
excluded things that sound somewhat sane to me.
Note that you are performing these replacements for all songs for all input 
formats. I have no idea how different formats are specified and what is 
used/allowed and what not. Just blindly replacing stuff doesn't sound right.
For instance replacing tabs with nothing doesn't sound like the correct thing 
to do. Which is why I excluded the whole block of these.

I'd probably also argue, that these replacements should actually be carried out 
in the single importers, where you have knowledge about what you are getting.

>  INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
>  IMAGES_FILTER = None
>  REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', 
> '\u201c': '"', '\u201d': '"', '\u2026': '...',
> @@ -471,15 +471,15 @@
>  log.exception('Error detecting file encoding')
>  
>  
> -def normalize_str(irreg_str):
> +def normalize_str(string):

it's not string but rather str that is the type, so string is perfectly fine

>  """
>  Normalize the supplied string. Remove unicode control chars and tidy up 
> white space.
>  
> -:param str irreg_str: The string to normalize.
> +:param str string: The string to normalize.
>  :return: The normalized string
>  :rtype: str
>  """
> -irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
> -irreg_str = CONTROL_CHARS.sub('', irreg_str)
> -irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
> -return WHITESPACE_REGEX.sub(' ', irreg_str)
> +string = string.translate(REPLACMENT_CHARS_MAP)
> +string = CONTROL_CHARS.sub('', string)
> +string = NEW_LINE_REGEX.sub('\n', string)
> +return WHITESPACE_REGEX.sub(' ', string)
> 
> === modified file 'tests/functional/openlp_core/common/test_common.py'
> --- tests/functional/openlp_core/common/test_common.py2017-12-29 
> 09:15:48 +
> +++ tests/functional/openlp_core/common/test_common.py2018-04-14 
> 20:05:07 +
> @@ -25,8 +25,8 @@
>  from unittest import TestCase
>  from unittest.mock import MagicMock, call, patch
>  
> -from openlp.core.common import clean_button_text, de_hump, extension_loader, 
> is_macosx, is_linux, is_win, \
> -path_to_module, trace_error_handler
> +from openlp.core.common import (clean_button_text, de_hump, 
> extension_loader, is_macosx, is_linux,
> +is_win, normalize_str, path_to_module, 
> trace_error_handler)

I think that using backslashes in imports is a little weird, but I'll change it

>  from openlp.core.common.path import Path
>  
>  
> @@ -211,6 +211,30 @@
>  assert is_win() is False, 'is_win() should return False'
>  assert is_macosx() is False, 'is_macosx() should return False'
>  
> +def test_normalize_str_leaves_newlines(self):
> +# GIVEN: a string containing newlines
> +str = 'something\nelse'

yeah, it's just tests but I'll change them :-) (especially since I mentioned 
that it was a type in one comment :D)

> +# WHEN: normalize is called
> +normalized_string = normalize_str(str)
> +# THEN: string is unchanged
> +assert normalized_string == str
> +
> +def test_normalize_str_removes_null_byte(self):
> +# GIVEN: a string containing newlines
> +str = 'somet\x00hing'
> +# WHEN: normalize is called
> +normalized_string = normalize_str(str)
> +# THEN: string is unchanged
> +assert normalized_string == 'something'
> +
> +def test_normalize_str_replaces_crlf_with_lf(self):
> +# GIVEN: a string containing crlf
> +str = 'something\r\nelse'
> +# WHEN: normalize is called
> +normalized_string = normalize_str(str)
> +# THEN: crlf is replaced with lf
> +assert normalized_string == 'something\nelse'
> +
>  def test_clean_button_text(self):
>  """
>  Test the clean_button_text() function.


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343256
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-15 Thread Phill
Review: Needs Fixing

See in line. One question and a few (more) minor fixes please.

Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-14 20:05:07 +
> @@ -44,7 +44,7 @@
>  
>  FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
>  SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
> -CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
> +CONTROL_CHARS = re.compile(r'[\x00-\08\x0E-\x1F\x7F-\x9F]')

is there any reason for removing 09, 0B, 0C (horizontal and vertical tabs as 
well as form feed)?

>  INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
>  IMAGES_FILTER = None
>  REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', 
> '\u201c': '"', '\u201d': '"', '\u2026': '...',
> 
> === modified file 'tests/functional/openlp_core/common/test_common.py'
> --- tests/functional/openlp_core/common/test_common.py2017-12-29 
> 09:15:48 +
> +++ tests/functional/openlp_core/common/test_common.py2018-04-14 
> 20:05:07 +
> @@ -25,8 +25,8 @@
>  from unittest import TestCase
>  from unittest.mock import MagicMock, call, patch
>  
> -from openlp.core.common import clean_button_text, de_hump, extension_loader, 
> is_macosx, is_linux, is_win, \
> -path_to_module, trace_error_handler
> +from openlp.core.common import (clean_button_text, de_hump, 
> extension_loader, is_macosx, is_linux,
> +is_win, normalize_str, path_to_module, 
> trace_error_handler)

can you remove the brackets from the import statement please, its not our style.

>  from openlp.core.common.path import Path
>  
>  
> @@ -211,6 +211,30 @@
>  assert is_win() is False, 'is_win() should return False'
>  assert is_macosx() is False, 'is_macosx() should return False'
>  
> +def test_normalize_str_leaves_newlines(self):
> +# GIVEN: a string containing newlines
> +str = 'something\nelse'

str is a built-in function, please use another name

> +# WHEN: normalize is called
> +normalized_string = normalize_str(str)
> +# THEN: string is unchanged
> +assert normalized_string == str
> +
> +def test_normalize_str_removes_null_byte(self):
> +# GIVEN: a string containing newlines
> +str = 'somet\x00hing'

and here

> +# WHEN: normalize is called
> +normalized_string = normalize_str(str)
> +# THEN: string is unchanged
> +assert normalized_string == 'something'
> +
> +def test_normalize_str_replaces_crlf_with_lf(self):
> +# GIVEN: a string containing crlf
> +str = 'something\r\nelse'

here too!

> +# WHEN: normalize is called
> +normalized_string = normalize_str(str)
> +# THEN: crlf is replaced with lf
> +assert normalized_string == 'something\nelse'
> +
>  def test_clean_button_text(self):
>  """
>  Test the clean_button_text() function.


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343256
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


Re: [Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-14 Thread Tim Bentley
Review: Needs Fixing

One minor fix as I do not like the rename of the field

Diff comments:

> === modified file 'openlp/core/common/__init__.py'
> --- openlp/core/common/__init__.py2018-02-24 16:10:02 +
> +++ openlp/core/common/__init__.py2018-04-14 20:05:07 +
> @@ -471,15 +471,15 @@
>  log.exception('Error detecting file encoding')
>  
>  
> -def normalize_str(irreg_str):
> +def normalize_str(string):

String is a data type so should not be used as a variable..

>  """
>  Normalize the supplied string. Remove unicode control chars and tidy up 
> white space.
>  
> -:param str irreg_str: The string to normalize.
> +:param str string: The string to normalize.
>  :return: The normalized string
>  :rtype: str
>  """
> -irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
> -irreg_str = CONTROL_CHARS.sub('', irreg_str)
> -irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
> -return WHITESPACE_REGEX.sub(' ', irreg_str)
> +string = string.translate(REPLACMENT_CHARS_MAP)
> +string = CONTROL_CHARS.sub('', string)
> +string = NEW_LINE_REGEX.sub('\n', string)
> +return WHITESPACE_REGEX.sub(' ', string)


-- 
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343256
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp


[Openlp-core] [Merge] lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp

2018-04-14 Thread Simon Hanna
Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-newline-bug into 
lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1727517 in OpenLP: "Unicode control chars causes song importer to crash"
  https://bugs.launchpad.net/openlp/+bug/1727517

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-newline-bug/+merge/343256

Fix the fix for 1727517
-- 
Your team OpenLP Core is requested to review the proposed merge of 
lp:~thelinuxguy/openlp/fix-newline-bug into lp:openlp.
=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2018-02-24 16:10:02 +
+++ openlp/core/common/__init__.py	2018-04-14 20:05:07 +
@@ -44,7 +44,7 @@
 
 FIRST_CAMEL_REGEX = re.compile('(.)([A-Z][a-z]+)')
 SECOND_CAMEL_REGEX = re.compile('([a-z0-9])([A-Z])')
-CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]')
+CONTROL_CHARS = re.compile(r'[\x00-\08\x0E-\x1F\x7F-\x9F]')
 INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]')
 IMAGES_FILTER = None
 REPLACMENT_CHARS_MAP = str.maketrans({'\u2018': '\'', '\u2019': '\'', '\u201c': '"', '\u201d': '"', '\u2026': '...',
@@ -471,15 +471,15 @@
 log.exception('Error detecting file encoding')
 
 
-def normalize_str(irreg_str):
+def normalize_str(string):
 """
 Normalize the supplied string. Remove unicode control chars and tidy up white space.
 
-:param str irreg_str: The string to normalize.
+:param str string: The string to normalize.
 :return: The normalized string
 :rtype: str
 """
-irreg_str = irreg_str.translate(REPLACMENT_CHARS_MAP)
-irreg_str = CONTROL_CHARS.sub('', irreg_str)
-irreg_str = NEW_LINE_REGEX.sub('\n', irreg_str)
-return WHITESPACE_REGEX.sub(' ', irreg_str)
+string = string.translate(REPLACMENT_CHARS_MAP)
+string = CONTROL_CHARS.sub('', string)
+string = NEW_LINE_REGEX.sub('\n', string)
+return WHITESPACE_REGEX.sub(' ', string)

=== modified file 'tests/functional/openlp_core/common/test_common.py'
--- tests/functional/openlp_core/common/test_common.py	2017-12-29 09:15:48 +
+++ tests/functional/openlp_core/common/test_common.py	2018-04-14 20:05:07 +
@@ -25,8 +25,8 @@
 from unittest import TestCase
 from unittest.mock import MagicMock, call, patch
 
-from openlp.core.common import clean_button_text, de_hump, extension_loader, is_macosx, is_linux, is_win, \
-path_to_module, trace_error_handler
+from openlp.core.common import (clean_button_text, de_hump, extension_loader, is_macosx, is_linux,
+is_win, normalize_str, path_to_module, trace_error_handler)
 from openlp.core.common.path import Path
 
 
@@ -211,6 +211,30 @@
 assert is_win() is False, 'is_win() should return False'
 assert is_macosx() is False, 'is_macosx() should return False'
 
+def test_normalize_str_leaves_newlines(self):
+# GIVEN: a string containing newlines
+str = 'something\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(str)
+# THEN: string is unchanged
+assert normalized_string == str
+
+def test_normalize_str_removes_null_byte(self):
+# GIVEN: a string containing newlines
+str = 'somet\x00hing'
+# WHEN: normalize is called
+normalized_string = normalize_str(str)
+# THEN: string is unchanged
+assert normalized_string == 'something'
+
+def test_normalize_str_replaces_crlf_with_lf(self):
+# GIVEN: a string containing crlf
+str = 'something\r\nelse'
+# WHEN: normalize is called
+normalized_string = normalize_str(str)
+# THEN: crlf is replaced with lf
+assert normalized_string == 'something\nelse'
+
 def test_clean_button_text(self):
 """
 Test the clean_button_text() function.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp