openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Phill has proposed merging lp:~phill-ridout/openlp/bug1416703 into lp:openlp.

Requested reviews:
  Raoul Snyman (raoul-snyman)
Related bugs:
  Bug #1416703 in OpenLP: "Exception when saving a service"
  https://bugs.launchpad.net/openlp/+bug/1416703

For more details, see:
https://code.launchpad.net/~phill-ridout/openlp/bug1416703/+merge/250531

Fixes bug 1416703 by implementing the ability to provide a function to do the conversion.

One test written (covers whole function) and potential bug fixed. Was this the one you were thinking of? 

Add this to your merge proposal:
--------------------------------
lp:~phill-ridout/openlp/bug1416703 (revision 2504)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/973/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/898/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/841/
[SUCCESS] http://ci.openlp.org/job/Branch-04a-Windows_Functional_Tests/731/
[SUCCESS] http://ci.openlp.org/job/Branch-04b-Windows_Interface_Tests/330/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/467/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/338/

Process finished with exit code 0


-- 
Your team OpenLP Core is subscribed to branch lp:openlp.

=== modified file 'openlp/core/common/settings.py'
--- openlp/core/common/settings.py	2015-02-17 19:50:18 +0000
+++ openlp/core/common/settings.py	2015-02-21 13:19:37 +0000
@@ -25,7 +25,6 @@
 import datetime
 import logging
 import os
-import sys
 
 from PyQt4 import QtCore, QtGui
 
@@ -45,6 +44,21 @@
         X11_BYPASS_DEFAULT = False
 
 
+def recent_files_conv(value):
+    """
+    If the value is not a list convert it to a list
+    :param value: Value to convert
+    :return: value as a List
+    """
+    if isinstance(value, list):
+        return value
+    elif isinstance(value, str):
+        return [value]
+    elif isinstance(value, bytes):
+        return [value.decode()]
+    return []
+
+
 class Settings(QtCore.QSettings):
     """
     Class to wrap QSettings.
@@ -66,10 +80,9 @@
         The first entry is the *old key*; it will be removed.
 
         The second entry is the *new key*; we will add it to the config. If this is just an empty string, we just remove
-        the old key.
-
-        The last entry is a list containing two-pair tuples. If the list is empty, no conversion is made. Otherwise each
-        pair describes how to convert the old setting's value::
+        the old key. The last entry is a list containing two-pair tuples. If the list is empty, no conversion is made.
+        If the first value is callable i.e. a function, the function will be called with the old setting's value.
+        Otherwise each pair describes how to convert the old setting's value::
 
             (SlideLimits.Wrap, True)
 
@@ -334,7 +347,7 @@
         ('general/language', 'core/language', []),
         ('general/last version test', 'core/last version test', []),
         ('general/loop delay', 'core/loop delay', []),
-        ('general/recent files', 'core/recent files', []),
+        ('general/recent files', 'core/recent files', [(recent_files_conv, None)]),
         ('general/save prompt', 'core/save prompt', []),
         ('general/screen blank', 'core/screen blank', []),
         ('general/show splash', 'core/show splash', []),
@@ -359,7 +372,7 @@
 
         :param default_values: A dict with setting keys and their default values.
         """
-        Settings.__default_settings__ = dict(list(default_values.items()) + list(Settings.__default_settings__.items()))
+        Settings.__default_settings__.update(default_values)
 
     @staticmethod
     def set_filename(ini_file):
@@ -416,7 +429,9 @@
                     for new, old in rules:
                         # If the value matches with the condition (rule), then use the provided value. This is used to
                         # convert values. E. g. an old value 1 results in True, and 0 in False.
-                        if old == old_value:
+                        if callable(new):
+                            old_value = new(old_value)
+                        elif old == old_value:
                             old_value = new
                             break
                     self.setValue(new_key, old_value)

=== modified file 'tests/functional/openlp_core_common/test_settings.py'
--- tests/functional/openlp_core_common/test_settings.py	2015-01-18 13:39:21 +0000
+++ tests/functional/openlp_core_common/test_settings.py	2015-02-21 13:19:37 +0000
@@ -25,6 +25,8 @@
 from unittest import TestCase
 
 from openlp.core.common import Settings
+from openlp.core.common.settings import recent_files_conv
+from tests.functional import patch
 from tests.helpers.testmixin import TestMixin
 
 
@@ -45,6 +47,25 @@
         """
         self.destroy_settings()
 
+    def recent_files_conv_test(self):
+        """
+        Test that recent_files_conv, converts various possible types of values correctly.
+        """
+        # GIVEN: A list of possible value types and the expected results
+        possible_values = [(['multiple', 'values'], ['multiple', 'values']),
+                           (['single value'], ['single value']),
+                           ('string value', ['string value']),
+                           (b'bytes value', ['bytes value']),
+                           ([], []),
+                           (None, [])]
+
+        # WHEN: Calling recent_files_conv with the possible values
+        for value, expected_result in possible_values:
+            actual_result = recent_files_conv(value)
+
+            # THEN: The actual result should be the same as the expected result
+            self.assertEqual(actual_result, expected_result)
+
     def settings_basic_test(self):
         """
         Test the Settings creation and its default usage
@@ -120,3 +141,19 @@
 
         # THEN: An exception with the non-existing key should be thrown
         self.assertEqual(str(cm.exception), "'core/does not exists'", 'We should get an exception')
+
+    def extend_default_settings_test(self):
+        """
+        Test that the extend_default_settings method extends the default settings
+        """
+        # GIVEN: A patched __default_settings__ dictionary
+        with patch.dict(Settings.__default_settings__,
+                        {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 3}, True):
+
+            # WHEN: Calling extend_default_settings
+            Settings.extend_default_settings({'test/setting 3': 4, 'test/extended 1': 1, 'test/extended 2': 2})
+
+            # THEN: The _default_settings__ dictionary_ should have the new keys
+            self.assertEqual(
+                Settings.__default_settings__, {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 4,
+                                                'test/extended 1': 1, 'test/extended 2': 2})