openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #23357
[Merge] lp:~googol/openlp/bug-1240037 into lp:openlp
Andreas Preikschat has proposed merging lp:~googol/openlp/bug-1240037 into lp:openlp.
Requested reviews:
OpenLP Core (openlp-core)
Related bugs:
Bug #1240037 in OpenLP: "Error occured when moving directory"
https://bugs.launchpad.net/openlp/+bug/1240037
For more details, see:
https://code.launchpad.net/~googol/openlp/bug-1240037/+merge/217335
Hello,
- Fixed bug #1240037
- Fixed a few bugs in the CategoryActionList class (recursion, ...)
- Removed depreciated has_key() method.
- Changed behaviour of the remove() method to match the behaviour of the list.remove() method
- refactored defaultShortcuts to default_shortcuts
Note: It seems that the CategoryActionList class provides methods we never used. I thought about removing these, but as I wrote tests now, I decided against removing them.
lp:~googol/openlp/bug-1240037 (revision 2377)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/406/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/362/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/307/
[SUCCESS] http://ci.openlp.org/job/Branch-04-Windows_Tests/268/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/185/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/60/
--
https://code.launchpad.net/~googol/openlp/bug-1240037/+merge/217335
Your team OpenLP Core is requested to review the proposed merge of lp:~googol/openlp/bug-1240037 into lp:openlp.
=== modified file 'openlp/core/ui/mainwindow.py'
--- openlp/core/ui/mainwindow.py 2014-04-12 20:19:22 +0000
+++ openlp/core/ui/mainwindow.py 2014-04-26 09:39:44 +0000
@@ -1334,7 +1334,7 @@
if self.copy_data:
log.info('Copying data to new path')
try:
- self.showStatusMessage(
+ self.show_status_message(
translate('OpenLP.MainWindow', 'Copying OpenLP data to new data directory location - %s '
'- Please wait for copy to finish').replace('%s', self.new_data_path))
dir_util.copy_tree(old_data_path, self.new_data_path)
@@ -1364,8 +1364,7 @@
args = []
for a in self.arguments:
args.extend([a])
- for arg in args:
- filename = arg
+ for filename in args:
if not isinstance(filename, str):
filename = str(filename, sys.getfilesystemencoding())
if filename.endswith(('.osz', '.oszl')):
=== modified file 'openlp/core/ui/shortcutlistform.py'
--- openlp/core/ui/shortcutlistform.py 2014-03-20 19:10:31 +0000
+++ openlp/core/ui/shortcutlistform.py 2014-04-26 09:39:44 +0000
@@ -244,10 +244,10 @@
self.primary_push_button.setChecked(False)
self.alternate_push_button.setChecked(False)
else:
- if action.defaultShortcuts:
- primary_label_text = action.defaultShortcuts[0].toString()
- if len(action.defaultShortcuts) == 2:
- alternate_label_text = action.defaultShortcuts[1].toString()
+ if action.default_shortcuts:
+ primary_label_text = action.default_shortcuts[0].toString()
+ if len(action.default_shortcuts) == 2:
+ alternate_label_text = action.default_shortcuts[1].toString()
shortcuts = self._action_shortcuts(action)
# We do not want to loose pending changes, that is why we have to keep the text when, this function has not
# been triggered by a signal.
@@ -292,7 +292,7 @@
self._adjust_button(self.alternate_push_button, False, text='')
for category in self.action_list.categories:
for action in category.actions:
- self.changed_actions[action] = action.defaultShortcuts
+ self.changed_actions[action] = action.default_shortcuts
self.refresh_shortcut_list()
def on_default_radio_button_clicked(self, toggled):
@@ -306,7 +306,7 @@
if action is None:
return
temp_shortcuts = self._action_shortcuts(action)
- self.changed_actions[action] = action.defaultShortcuts
+ self.changed_actions[action] = action.default_shortcuts
self.refresh_shortcut_list()
primary_button_text = ''
alternate_button_text = ''
@@ -357,8 +357,8 @@
return
shortcuts = self._action_shortcuts(action)
new_shortcuts = []
- if action.defaultShortcuts:
- new_shortcuts.append(action.defaultShortcuts[0])
+ if action.default_shortcuts:
+ new_shortcuts.append(action.default_shortcuts[0])
# We have to check if the primary default shortcut is available. But we only have to check, if the action
# has a default primary shortcut (an "empty" shortcut is always valid and if the action does not have a
# default primary shortcut, then the alternative shortcut (not the default one) will become primary
@@ -383,8 +383,8 @@
new_shortcuts = []
if shortcuts:
new_shortcuts.append(shortcuts[0])
- if len(action.defaultShortcuts) == 2:
- new_shortcuts.append(action.defaultShortcuts[1])
+ if len(action.default_shortcuts) == 2:
+ new_shortcuts.append(action.default_shortcuts[1])
if len(new_shortcuts) == 2:
if not self._validiate_shortcut(action, new_shortcuts[1]):
return
=== modified file 'openlp/core/utils/actions.py'
--- openlp/core/utils/actions.py 2014-03-20 19:10:31 +0000
+++ openlp/core/utils/actions.py 2014-04-26 09:39:44 +0000
@@ -69,16 +69,16 @@
"""
Implement the __getitem__() method to make this class a dictionary type
"""
- for weight, action in self.actions:
- if action.text() == key:
- return action
- raise KeyError('Action "%s" does not exist.' % key)
+ return self.actions[key][1]
- def __contains__(self, item):
+ def __contains__(self, key):
"""
Implement the __contains__() method to make this class a dictionary type
"""
- return item in self
+ for weight, action in self.actions:
+ if action == key:
+ return True
+ return False
def __len__(self):
"""
@@ -103,23 +103,14 @@
self.index += 1
return self.actions[self.index - 1][1]
- def has_key(self, key):
- """
- Implement the has_key() method to make this class a dictionary type
- """
- for weight, action in self.actions:
- if action.text() == key:
- return True
- return False
-
- def append(self, name):
+ def append(self, action):
"""
Append an action
"""
weight = 0
if self.actions:
weight = self.actions[-1][0] + 1
- self.add(name, weight)
+ self.add(action, weight)
def add(self, action, weight=0):
"""
@@ -128,14 +119,15 @@
self.actions.append((weight, action))
self.actions.sort(key=lambda act: act[0])
- def remove(self, remove_action):
+ def remove(self, action):
"""
Remove an action
"""
- for action in self.actions:
- if action[1] == remove_action:
- self.actions.remove(action)
+ for item in self.actions:
+ if item[1] == action:
+ self.actions.remove(item)
return
+ raise ValueError('Action "%s" does not exist.' % action)
class CategoryList(object):
@@ -270,7 +262,7 @@
settings = Settings()
settings.beginGroup('shortcuts')
# Get the default shortcut from the config.
- action.defaultShortcuts = settings.get_default_value(action.objectName())
+ action.default_shortcuts = settings.get_default_value(action.objectName())
if weight is None:
self.categories[category].actions.append(action)
else:
=== modified file 'tests/functional/openlp_core_utils/test_actions.py'
--- tests/functional/openlp_core_utils/test_actions.py 2014-04-02 19:35:09 +0000
+++ tests/functional/openlp_core_utils/test_actions.py 2014-04-26 09:39:44 +0000
@@ -35,9 +35,123 @@
from openlp.core.common import Settings
from openlp.core.utils import ActionList
+from openlp.core.utils.actions import CategoryActionList
+from tests.functional import MagicMock
from tests.helpers.testmixin import TestMixin
+class TestCategoryActionList(TestCase):
+ def setUp(self):
+ """
+ Create an instance and a few example actions.
+ """
+ self.action1 = MagicMock()
+ self.action1.text.return_value = 'first'
+ self.action2 = MagicMock()
+ self.action2.text.return_value = 'second'
+ self.list = CategoryActionList()
+
+ def tearDown(self):
+ """
+ Clean up
+ """
+ del self.list
+
+ def get_test(self):
+ """
+ Test the __getitem__() method
+ """
+ # GIVEN: The list.
+ self.list.append(self.action1)
+
+ # WHEN: Add an action.
+ returned_action = self.list[0]
+
+ # THEN: Check if the correct action was returned.
+ self.assertEqual(self.action1, returned_action)
+
+ # THEN: Test if an exception is raised when trying to access a non existing item.
+ self.assertRaises(IndexError, self.list.__getitem__, 1)
+
+ def contains_test(self):
+ """
+ Test the __contains__() method
+ """
+ # GIVEN: The list.
+ # WHEN: Add an action
+ self.list.append(self.action1)
+
+ # THEN: The actions should (not) be in the list.
+ self.assertTrue(self.action1 in self.list)
+ self.assertFalse(self.action2 in self.list)
+
+ def len_test(self):
+ """
+ Test the __len__ method
+ """
+ # GIVEN: The list.
+ # WHEN: Do nothing.
+ # THEN: Check the length.
+ self.assertEqual(len(self.list), 0, "The length should be 0.")
+
+ # GIVEN: The list.
+ # WHEN: Append an action.
+ self.list.append(self.action1)
+
+ # THEN: Check the length.
+ self.assertEqual(len(self.list), 1, "The length should be 1.")
+
+ def append_test(self):
+ """
+ Test the append() method
+ """
+ # GIVEN: The list.
+ # WHEN: Append an action.
+ self.list.append(self.action1)
+ self.list.append(self.action2)
+
+ # THEN: Check if the actions are in the list and check if they have the correct weights.
+ self.assertTrue(self.action1 in self.list)
+ self.assertTrue(self.action2 in self.list)
+ self.assertEqual(self.list[0], self.action1)
+ self.assertEqual(self.list[1], self.action2)
+
+ def add_test(self):
+ """
+ Test the add() method
+ """
+ # GIVEN: The list and weights.
+ action1_weight = 42
+ action2_weight = 41
+
+ # WHEN: Add actions and their weights.
+ self.list.add(self.action1, action1_weight)
+ self.list.add(self.action2, action2_weight)
+
+ # THEN: Check if they were added and have the specified weights.
+ self.assertTrue(self.action1 in self.list)
+ self.assertTrue(self.action2 in self.list)
+ # Now check if action1 is second and action2 is first (due to their weights).
+ self.assertEqual(self.list[0], self.action2)
+ self.assertEqual(self.list[1], self.action1)
+
+ def remove_test(self):
+ """
+ Test the remove() method
+ """
+ # GIVEN: The list
+ self.list.append(self.action1)
+
+ # WHEN: Delete an item from the list.
+ self.list.remove(self.action1)
+
+ # THEN: Now the element should not be in the list anymore.
+ self.assertFalse(self.action1 in self.list)
+
+ # THEN: Check if an exception is raised when trying to remove a not present action.
+ self.assertRaises(ValueError, self.list.remove, self.action2)
+
+
class TestActionList(TestCase, TestMixin):
"""
Test the ActionList class
Follow ups
