openlp-core team mailing list archive

[Tomas Groth <tomasgroth@xxxxxxxx>]

Tomas Groth has proposed merging lp:~tomasgroth/openlp/bugfixes18 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1438563 in OpenLP: "Saving theme with unicode letters in the name causes traceback"
  https://bugs.launchpad.net/openlp/+bug/1438563
  Bug #1439671 in OpenLP: "Edit button on preview toolbar disappears when editing custom"
  https://bugs.launchpad.net/openlp/+bug/1439671

For more details, see:
https://code.launchpad.net/~tomasgroth/openlp/bugfixes18/+merge/255147

Mark a custom slide edited from preview as coming from plugin. Fixes bug 1439671.
Use html.escape instead of the deprecated cgi.escape
Fix support for special characters in theme names. Fixes bug 1438563.
Fix another case of traceback when playing media with no players available/enabled (bug 1422761).
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~tomasgroth/openlp/bugfixes18 into lp:openlp.

=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2015-02-04 16:43:04 +0000
+++ openlp/core/common/__init__.py	2015-04-02 21:03:11 +0000
@@ -66,8 +66,9 @@
     try:
         if not os.path.exists(directory):
             os.makedirs(directory)
-    except IOError:
-        pass
+    except IOError as e:
+        if not do_not_log:
+            log.exception('failed to check if directory exists or create directory')
 
 
 def get_frozen_path(frozen_option, non_frozen_option):

=== modified file 'openlp/core/lib/__init__.py'
--- openlp/core/lib/__init__.py	2015-02-13 21:47:06 +0000
+++ openlp/core/lib/__init__.py	2015-04-02 21:03:11 +0000
@@ -90,7 +90,7 @@
     file_handle = None
     content = None
     try:
-        file_handle = open(text_file, 'r')
+        file_handle = open(text_file, 'r', encoding='utf-8')
         if not file_handle.read(3) == '\xEF\xBB\xBF':
             # no BOM was found
             file_handle.seek(0)

=== modified file 'openlp/core/ui/maindisplay.py'
--- openlp/core/ui/maindisplay.py	2015-01-29 20:58:14 +0000
+++ openlp/core/ui/maindisplay.py	2015-04-02 21:03:11 +0000
@@ -29,7 +29,7 @@
 
 """
 
-import cgi
+import html
 import logging
 
 from PyQt4 import QtCore, QtGui, QtWebKit, QtOpenGL
@@ -273,7 +273,7 @@
         """
         # First we convert <>& marks to html variants, then apply
         # formattingtags, finally we double all backslashes for JavaScript.
-        text_prepared = expand_tags(cgi.escape(text)).replace('\\', '\\\\').replace('\"', '\\\"')
+        text_prepared = expand_tags(html.escape(text)).replace('\\', '\\\\').replace('\"', '\\\"')
         if self.height() != self.screen['size'].height() or not self.isVisible():
             shrink = True
             js = 'show_alert("%s", "%s")' % (text_prepared, 'top')

=== modified file 'openlp/core/ui/media/mediacontroller.py'
--- openlp/core/ui/media/mediacontroller.py	2015-03-16 21:32:56 +0000
+++ openlp/core/ui/media/mediacontroller.py	2015-04-02 21:03:11 +0000
@@ -523,6 +523,8 @@
         if controller.media_info.file_info.isFile():
             suffix = '*.%s' % controller.media_info.file_info.suffix().lower()
             for title in used_players:
+                if not title:
+                    continue
                 player = self.media_players[title]
                 if suffix in player.video_extensions_list:
                     if not controller.media_info.is_background or controller.media_info.is_background and \

=== modified file 'openlp/core/ui/thememanager.py'
--- openlp/core/ui/thememanager.py	2015-02-28 07:04:41 +0000
+++ openlp/core/ui/thememanager.py	2015-04-02 21:03:11 +0000
@@ -354,8 +354,12 @@
         delete_file(os.path.join(self.path, thumb))
         delete_file(os.path.join(self.thumb_path, thumb))
         try:
-            encoding = get_filesystem_encoding()
-            shutil.rmtree(os.path.join(self.path, theme).encode(encoding))
+            # Windows is always unicode, so no need to encode filenames
+            if is_win():
+                shutil.rmtree(os.path.join(self.path, theme))
+            else:
+                encoding = get_filesystem_encoding()
+                shutil.rmtree(os.path.join(self.path, theme).encode(encoding))
         except OSError as os_error:
             shutil.Error = os_error
             self.log_exception('Error deleting theme %s' % theme)
@@ -567,7 +571,7 @@
                 check_directory_exists(os.path.dirname(full_name))
                 if os.path.splitext(name)[1].lower() == '.xml':
                     file_xml = str(theme_zip.read(name), 'utf-8')
-                    out_file = open(full_name, 'w')
+                    out_file = open(full_name, 'w', encoding='utf-8')
                     out_file.write(file_xml)
                 else:
                     out_file = open(full_name, 'wb')
@@ -645,8 +649,8 @@
             delete_file(self.old_background_image)
         out_file = None
         try:
-            out_file = open(theme_file, 'w')
-            out_file.write(theme_pretty_xml.decode('UTF-8'))
+            out_file = open(theme_file, 'w', encoding='utf-8')
+            out_file.write(theme_pretty_xml.decode('utf-8'))
         except IOError:
             self.log_exception('Saving theme to file failed')
         finally:

=== modified file 'openlp/plugins/custom/lib/mediaitem.py'
--- openlp/plugins/custom/lib/mediaitem.py	2015-01-18 13:39:21 +0000
+++ openlp/plugins/custom/lib/mediaitem.py	2015-04-02 21:03:11 +0000
@@ -158,6 +158,10 @@
                 self.remote_triggered = None
                 self.remote_custom = 1
                 if item:
+                    if preview:
+                        # A custom slide can only be edited if it comes from plugin,
+                        # so we set it again for the new item.
+                        item.from_plugin = True
                     return item
         return None
 

=== modified file 'openlp/plugins/songs/lib/openlyricsxml.py'
--- openlp/plugins/songs/lib/openlyricsxml.py	2015-01-21 20:35:36 +0000
+++ openlp/plugins/songs/lib/openlyricsxml.py	2015-04-02 21:03:11 +0000
@@ -55,7 +55,7 @@
         </lyrics>
     </song>
 """
-import cgi
+import html
 import logging
 import re
 
@@ -318,7 +318,7 @@
             if 'lang' in verse[0]:
                 verse_element.set('lang', verse[0]['lang'])
             # Create a list with all "optional" verses.
-            optional_verses = cgi.escape(verse[1])
+            optional_verses = html.escape(verse[1])
             optional_verses = optional_verses.split('\n[---]\n')
             start_tags = ''
             end_tags = ''

=== modified file 'tests/functional/openlp_core_lib/test_lib.py'
--- tests/functional/openlp_core_lib/test_lib.py	2015-01-18 13:39:21 +0000
+++ tests/functional/openlp_core_lib/test_lib.py	2015-04-02 21:03:11 +0000
@@ -176,7 +176,7 @@
 
             # THEN: None should be returned
             mocked_isfile.assert_called_with(filename)
-            mocked_open.assert_called_with(filename, 'r')
+            mocked_open.assert_called_with(filename, 'r', encoding='utf-8')
             self.assertIsNone(result, 'None should be returned if the file cannot be opened')
 
     def get_text_file_string_decode_error_test(self):

=== modified file 'tests/functional/openlp_core_ui/test_thememanager.py'
--- tests/functional/openlp_core_ui/test_thememanager.py	2015-02-19 18:57:05 +0000
+++ tests/functional/openlp_core_ui/test_thememanager.py	2015-04-02 21:03:11 +0000
@@ -29,6 +29,7 @@
 from tempfile import mkdtemp
 
 from PyQt4 import QtGui
+from tempfile import mkdtemp
 
 from openlp.core.ui import ThemeManager
 from openlp.core.common import Registry
@@ -44,6 +45,13 @@
         Set up the tests
         """
         Registry.create()
+        self.temp_folder = mkdtemp()
+
+    def tearDown(self):
+        """
+        Clean up
+        """
+        shutil.rmtree(self.temp_folder)
 
     def export_theme_test(self):
         """
@@ -131,6 +139,27 @@
             # THEN: The mocked_copyfile should not have been called
             self.assertTrue(mocked_copyfile.called, 'shutil.copyfile should be called')
 
+    def write_theme_special_char_name_test(self):
+        """
+        Test that we can save themes with special characters in the name
+        """
+        # GIVEN: A new theme manager instance, with mocked theme and thememanager-attributes.
+        theme_manager = ThemeManager(None)
+        theme_manager.old_background_image = None
+        theme_manager.generate_and_save_image = MagicMock()
+        theme_manager.path = self.temp_folder
+        mocked_theme = MagicMock()
+        mocked_theme.theme_name = 'theme 愛 name'
+        mocked_theme.extract_formatted_xml = MagicMock()
+        mocked_theme.extract_formatted_xml.return_value = 'fake theme 愛 XML'.encode()
+
+        # WHEN: Calling _write_theme with a theme with a name with special characters in it
+        theme_manager._write_theme(mocked_theme, None, None)
+
+        # THEN: It should have been created
+        self.assertTrue(os.path.exists(os.path.join(self.temp_folder, 'theme 愛 name', 'theme 愛 name.xml')),
+                        'Theme with special characters should have been created!')
+
     def over_write_message_box_yes_test(self):
         """
         Test that theme_manager.over_write_message_box returns True when the user clicks yes.